EntityResourceTestBase::testGet() has some very specific testing of how many cache redirects there should be stored in DynamicPageCacheSubscriber (which uses the render cache behind the scenes).

By testing the caching layer when you needn't, you are making it harder to make changes to said caching layer as seemingly unrelated tests such as this one will go red when you change how the cache works. See #2551419-130: Abstract RenderCache into a separate service that is capable of cache redirects in a non-render array-specific way for an example of how it breaks unrelated patches.

The fix here is to adapt the code introduced in #2877778: Harden test EntityResource + Dynamic Page Cache test coverage to simply check if there are any redirects, not how many there are. This allows us to adjust how many redirects the cache might introduce without having seemingly unrelated test fails occur.

I'm still not happy that it tests whether the response is a redirect by using the code below, but for now I see no other easy way around that.

        if (!isset($cached_data['#cache_redirect'])) {
          $cached_response = $cached_data['#response'];
CommentFileSizeAuthor
#14 interdiff_13-14.txt673 bytesvsujeetkumar
#14 3082053-14.patch1.88 KBvsujeetkumar
#13 3082053-13.patch1.86 KBvsujeetkumar
#2 drupal-3082053-2.patch2.38 KBkristiaanvandeneynde
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kristiaanvandeneynde created an issue. See original summary.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Factorial GmbH commented
Assigned: kristiaanvandeneynde » Unassigned
Status: Active » Needs review
FileSize
drupal-3082053-2.patch2.38 KB

Attached patch simplifies the check to merely see whether there are any cache items. It no longer checks if there are any redirects. I also simplified the query and loop because $cid was not being used.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +API-First Initiative
Related issues: +#2877778: Harden test EntityResource + Dynamic Page Cache test coverage

I'm fine with not testing how many redirects there are. But testing that there are some redirects is necessary, that's what #2877778: Harden test EntityResource + Dynamic Page Cache test coverage introduced, at the request of a core committer.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Factorial GmbH commented

Well, that's implicitly taken care of, no? We check that the DB returns one or more results, knowing that some of them might be redirects. That's what the if (!isset($cached_data['#cache_redirect'])) { further down is for.

The whole reason we don't want to specifically check for the amount of redirects is because it's up to the cache to decide whether it needs to add them. So we should only make sure that if there are redirects, we handle them, which the code still does.

If we don't commit the patch as attached, this test will always go red when an optimization of RenderCache or a replacement (in my case) happens that manages to remove the need for redirects in some scenarios. And no-one likes it when tests from totally unrelated sections of the codebase go red when you're in the process of optimizing another layer, right?

The "Proposed resolution" from the original issue literally says: "Because there's always a cache redirect, assert the number of cache items." But that statement is dangerous because you cannot know this will always be the case. We still keep the $found_cached_response check and judging from the original issue, that one was the most important one.

Edit: Even the current RenderCache will not issue a redirect if the pre-bubbling and post-bubbling element have the same cache ID. So it's really dangerous to assume that there will always be redirects.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Well, that's implicitly taken care of, no? We check that the DB returns one or more results, knowing that some of them might be redirects. That's what the if (!isset($cached_data['#cache_redirect'])) { further down is for.

That is … very implicit. Tests should be explicit.

And no-one likes it when tests from totally unrelated sections of the codebase go red when you're in the process of optimizing another layer, right?

Except it's not "totally unrelated" when we're specifically asserting the storage characteristics to guarantee predictable performance.

But that statement is dangerous because you cannot know this will always be the case.

Well, actually, we do. That's guaranteed to be the case in Dynamic Page Cache today. That's what you propose to change.

Even the current RenderCache will not issue a redirect if the pre-bubbling and post-bubbling element have the same cache ID. So it's really dangerous to assume that there will always be redirects.

… but that can only ever occur in cached render fragments, not for cached REST/JSON:API responses. Because those responses always need to have access checking, so it means that at a minimum the user.permissions cache context will be present in the post-bubbling scenario. So this caveat you added only applies to the general case, not to the Dynamic Page Cache case.

I'm fine with removing this, but then we need to make the test more clear. That's all.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Factorial GmbH commented

I'm fine with removing this, but then we need to make the test more clear. That's all.

How would you suggest we change the test?

I agree with #5 that DPC will have redirects (for now at least) and that it wouldn't be an issue for this test specifically. But I still feel that we have too many tests in core that are testing behavior from other layers that is already covered in dedicated tests. Consider #4 more of a general concern about these types of tests rather than this one in specific.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

I took another fresh look at this. I think you're right, actually :)

But here's one suggestion to at least retain the same spirit in testing:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -548,19 +548,18 @@ public function testGet() {
-      $this->assertCount(2, $cache_items);
...
+      $this->assertNotEmpty($cache_items);

Can't we change this to

$this->assertLessThanOrEqual(2, count($cache_items));

?

That way we at least continue to test the upper bound, but we did remove the lower bound, which is what you care about :)

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

joachim’s picture

joachim CreditAttribution: joachim at Factorial GmbH commented
Status: Needs review » Needs work

Setting to needs work based on #8.

vsujeetkumar’s picture

vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3082053-13.patch1.86 KB

Re-roll patch created for 9.3.x.

vsujeetkumar’s picture

vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commented
FileSize
3082053-14.patch1.88 KB
interdiff_13-14.txt673 bytes

Address #8, Please have a look.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ at Calibrate commented
Status: Needs review » Reviewed & tested by the community

With Wim's approval in #8 this looks good to go, #14 still applies (with some offset, but that should be fine?).

I agree that it is important that we test the upper bound of the amount of cache redirects, and the test still does that right now.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed f8b4d7a105 to 10.1.x and 8d95ff2f1d to 10.0.x and 36cfac4274 to 9.5.x. Thanks!

Backported to 9.5.x to keep tests aligned.

  • alexpott committed f8b4d7a on 10.1.x 
    Issue #3082053 by vsujeetkumar, kristiaanvandeneynde, Wim Leers:...

  • alexpott committed 8d95ff2 on 10.0.x 
    Issue #3082053 by vsujeetkumar, kristiaanvandeneynde, Wim Leers:...

  • alexpott committed 36cfac4 on 9.5.x 
    Issue #3082053 by vsujeetkumar, kristiaanvandeneynde, Wim Leers:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.