Test coverage for Comment, Custom Block, Node, Taxonomy Term and User entity cache tags

1: term_cache_tags_test-2188565-1.patch queued for re-testing.

Straight reroll.

#7: agreed.

Done.

Implemented in a generic way, so which allows us to easily add test coverage for all entity types for which we want cache tag test coverage. I talked to Berdir & nielsvm, both agreed it'd be better to add tests for many entity types at once: less patch review/core committer overhead.

This patch adds cache tags test coverage for the following entity types:

• Comment
• Custom Block
• Node
• Taxonomy Term
• User

In addition, I've converted the tests for Menu (that were added in #2179083: Rendered menus (e.g. menu blocks) should set cache tags to inform the page cache) to use the newly added base class.

I tried to also add test coverage for File entities, but entity reference fields to File entities don't render, and hence don't get cache tags. Entity reference thinks no user has access to it, so quite possibly this is related to #2128791: File resource needs an access controller.

Added base classes

1. Drupal\system\Tests\Cache\PageCacheTagsTestBase: the base class that should be used by every cache tags test. Enables page caching and comes with a handy verifyPageCache($path, $hit_or_miss, $tags = FALSE) method.
2. \Drupal\system\Tests\Entity\EntityCacheTagsTestBase, extends PageCacheTagsTestBase: must be subclassed on a per-entity type basis, automatically will test the scenario Berdir described above, and more: create an entity of the "entity_test" type, reference an entity of the type being tested, verify the cache tags of that referencing entity, list all entities referencing our entity of interest, delete the referencing entity, modify the referenced entity, delete the referenced entity, and so on.
3. \Drupal\system\Tests\Entity\EntityWithUriCacheTagsTestBase, extends EntityCacheTagsTestBase: similar to the above, but also tests "the full entity page" for entities that have that (i.e. this must be used if and only if the entity type has a URI where you can see it). I.e. Comment, Node, Taxonomy Term and User use this, but Custom Block and File don't.

This of course has the huge benefit of being able to add more test scenarios to all these entity types at once, by modifying a single base class :)

Fixed bugs

EntityViewBuilder only sets cache tags if the entity is render cacheable, but that's wrong: it should always set cache tags, but only set cache keys/granularity/bin if the entity is render cacheable. Cache tags must always exist, otherwise the page cache cannot be informed!

It seems that just before I posted the patch, the field_entity entity type was renamed to field_config and field_instance to field_instance_config. Great changes, but they broke my patch :)

Testbot also surfaced NodePageCacheTest as being broken (yay!), which was introduced by #1841900: Node deletion should clear page cache. It inappropriately uses the content cache tag to ensure all affected nodes are deleted from the page cache. The test coverage in this patch demonstrates that that is not necessary. The test coverage in this patch is also a superset of what was added there, so removing NodePageCacheTest.

That was very painful to debug, but this should do the trick.

xdebug_break() call… STUPID!

#16: Thanks for the review!

I'm afraid that you reviewed the patch incorrectly though:

• you reviewed MenuTest, where there's no cache tags test anymore, it's been moved to MenuCacheTagsTest. I've applied your remarks to the code there. Block assignment changing is for the upcoming block cache test coverage at #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags.
• you also reviewed NodePageCacheTest, which was removed completely, so all of your remarks (all of which I agree with!) are no longer relevant :)

Also fixed the remarks for EntityTestController.

Finally, also fixed the two failures, which I did fix locally but forgot to stage and therefor got lost :/ Stupid!

Hm, this is interesting, but for the most part, we're essentially testing the same thing over and over again, just differently painted, or am I missing something? Most of what's being tested is generic and the same for all entity types, right? What do we gain from doing it for all entity types? Asking from a test performance standpoint, web tests are pretty slow...

Some entity types override the base entity view builder or storage controller. Therefore they might break caching. We want to ensure that's not the case. Hence it's necessary.

Plus, as we discover entity type-specific edge cases, I will add entity type-specific tests too. #2151459-75: Enable node render caching made me realize that changes to NodeType should also affect Node, so that's one I'm adding in this reroll.

Having done that, I thought of also ensuring that when you change field settings or field instance settings, the affected entities' render cache entries should be invalidated. That was NOT yet the case. In this reroll, I fixed that, and there's test coverage for it.

Do we need more test coverage for things that make each entity type special?

Yes! See the above example.

Most things are handled by the generic referencedEntities() implementation and everything being entity references now, so I guess my own argument is now saying that we don't need to test fun stuff like user signatures on comments, because it's from the author, or inline information about the comment parent, as that's an entity reference too. Cool :)

Agreed, but that's a different case of "specialness" when compared to the above example, wouldn't you agree?

As far as access goes, as long as it's by-role, we should be good. Wondering if there are things in core that are not that we need to think about. "'view own unpublished content'" could be an interesting case, that is essentially per-user for unpublished content?

Agreed. Where per-user content is rendered, it's the responsibility of the thing that does per-user rendering to set #cache = FALSE.

As you can see, I'm trying everything to find possible flaws ;)

That's exactly what this patch needs! :)

1. I can remove it for the example you cited, but the static::createEntity() call needs to remain, because we need late static binding for it. I don't think there will be many (if any) tests overriding createReferenceTestEntities(), but I figured I'd keep the door open.
2. :) Consistency & DRY FTW!
3. Because I can't. I really, really tried. See the comment. Also see TestBase::getInfo() for another such example.
4. Good find! How did you get PHPUnit to report that to you?
5. Fixed "improper" function calls. And no, because we create a referencing and a non-referencing entity. Modifying the non-referencing entity must only affect the non-referencing entity's page cache entry. Modifying the referencing entity must affect both the referencing entity and the listing of referencing entities. Etc. Just like *you* described/requested in #7… :) Am I missing something?
   However, thanks to your critical remark here, I noticed that I wasn't actually modifying the non-referencing entity yet and verifying its effects. As of this reroll, this patch does that too.
6. Great catch, fixed!

The two failures for #21 are unrelated to this patch.

1. I want those methods to be static, because I don't want them setting things on the object. Therefor, I must use late static binding. I tried again to do without, but I can't. The very first example listed at http://php.net/lsb is precisely the problem I'd encounter otherwise.
3. Huh!? PHP wouldn't run at all while I did what you're doing in that interdiff!? But then again, you've removed the static keyword, which I want to be there for the above reason. If I try what you did, but restore the static keyword, things break again.
5. I'm still having trouble grokking what your concern is :) I do see that it doesn't really matter whether the "listing page" contains only referencing entities or simply all entities. But what's the value of changing what I have ("list referencing entities") in the patch today? The reason I went for "list all referencing entities" is simple: it's precisely what the "taxonomy term page" does (but only for entities of type Node), and therefor it's a simple-to-grok listing use case, so I just went with that.

Nitpick fixed. Regarding that render cache invalidation for taxonomy terms: agreed, that's only a theoretical addition/improvement for the functionality in Drupal core, but I think it's very conceivable that taxonomy term pages in the real world might list the name of the vocabulary they're part of, and in that case it's definitely necessary.
This actually points me to a case where this is not yet correctly happening: custom_block entities, whose bundle entity type is custom_block_type. So far, I'd only added the additional test cases to the testEntityUri() test case (which is not run for custom blocks, since they don't have a URI), and not yet also to the testReferencedEntity() test case. I fixed that now.
In doing so, I noticed that I could implement this in a generic way in Drupal\Core\Entity\Entity, so now the changes in Vocabulary are gone, NodeType became a bit simpler, and no more changes to CustomBlockType are necessary.

Anybody can think of yet more test cases? :)

It seems this fatal error is nigh impossible to debug… The review log says this:

…
Breakpoint media query tests 2 passes, 0 fails, 0 exceptions
PHP Fatal error:  Call to a member function getBundleOf() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Entity/Entity.php on line 329
Comment route enhancer test 3 passes, 0 fails, 0 exceptions
…

Neither the preceding nor following test name triggers the PHP fatal error. So… then how are we supposed to figure out what's causing the test failure?

I'm adding debug output that will hopefully help…

So, #26 was utterly useless. To make debugging failed tests like that less miserable, we have #2189317: Get a PHPUnit job type running that is separate and independent from the Simpletest Job type, fix the d.o integration so that we can run the PHPUnit tests independently.

Much thanks to Berdir for the golden tip: just run all PHPUnit tests locally, because most fatal errors are PHPUnit tests. I did it, and I found the cause immediately. The fix is simple. This is an interdiff against #24, not #26.

Berdir wanted confirmation from somebody else that the way the base classes added here use static methods to enforce a strict contract is acceptable. We discussed it with alexpott in IRC, and he thinks that if there's a comment explaining this, we're fine. Thus: adding the necessary comments :)

Straight reroll, #1987694: Convert entity_test callbacks to a new style controller broke this.

#32: This is nothing extreme, \Drupal\simpletest\TestBase::setUp() does exactly the same already.

Can we please not focus on a tiny implementation detail in tests?

Sorry, I meant getInfo():

From TestBase:

  public static function getInfo() {
    // PHP does not allow us to declare this method as abstract public static,
    // so we simply throw an exception here if this has not been implemented by
    // a child class.
    throw new \RuntimeException(String::format('@class must implement \Drupal\simpletest\TestBase::getInfo().', array(
      '@class' => get_called_class(),
    )));
  }

So:

• it wants to be abstract, but can't, because of PHP limitations
• it most definitely is static!

I want to make it as easy as possible to RTBC/commit this issue. That's what I'm trying to do in this reroll.

There was a very slight amount of scope creep: three content cache tags were removed because they're now obsolete, but since this issue focuses on test coverage, they shouldn't be removed here. Only one removal was necessary, two were optional. (See interdiff to find out what's been restored.)

Now it's solely about test coverage for the Comment, Custom Block, Node, Taxonomy Term and User entity cache tags, with simple fixes in:

• core/lib/Drupal/Core/Entity/Entity (to invalidate render cache of affected entities when modifying a bundle entity type — NodeType already had this, but now it's generalized)
• Drupal/Core/Entity/EntityViewBuilder (to always set cache tags)
• Drupal/field/Entity/FieldConfig (to invalidate render cache of affected entities)
• Drupal/field/Entity/FieldInstanceConfig (to invalidate render cache of affected entities)
• Drupal/taxonomy/VocabularyStorageController (to not invalidate the content cache tag, because that'd cause one test failure, since it invalidates too much)

#39: from #30:

Berdir wanted confirmation from somebody else that the way the base classes added here use static methods to enforce a strict contract is acceptable. We discussed it with alexpott in IRC, and he thinks that if there's a comment explaining this, we're fine. Thus: adding the necessary comments :)

All statics removed.

Back to RTBC now that all statics have been removed.

This blocks #2124957: Replace 'content' cache tag with 'rendered' and use it sparingly, which is critical. Hence marking this critical.

I'm happy to say the answer is simple: "It's just stupid git diff context" :)

Because in both cases, it's actually postSave!

--- a/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
+++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
@@ -366,6 +366,13 @@ protected function preSaveUpdated(EntityStorageControllerInterface $storage_cont
  public function postSave(EntityStorageControllerInterface $storage_controller, $update = TRUE) {

--- a/core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php
+++ b/core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php
@@ -361,6 +361,13 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
  public function postSave(EntityStorageControllerInterface $storage_controller, $update = TRUE) {

The context that git provides is just misleading!

catch remarked in IRC that the cache invalidation in FieldConfig::postSave() had a small logic flaw: it should only invalidate when updating an existing field_config, because when you create a new field_config, it doesn't affect any entity, it only will once there is a field_instance_config for it that associates it with a specific bundle.

Trivial fix. Tests locally are green.

YES! Thanks! :)

Now we can continue with other subissues of #2124957: Replace 'content' cache tag with 'rendered' and use it sparingly! :)

#52: I spent *a lot* of time trying to achieve precisely that — I avoid using WebTestBase like the plague… if I can.

IIRC/AFAICT the problem is that Drupal's page cache sits *on top* of HttpKernel. Therefor, code like

$subrequest = Request::create('/node/1', 'GET');¬
$response = $this->container->get('http_kernel')->handle($subrequest);

does not trigger the page cache, which is part of what's being tested here. If you can show me the light, I will follow it.