Fix test performance of Drupal\system\Tests\Cache\PageCacheTagsIntegrationTest

I have been playing with this while down with a bug. I am stuck on creating the cache tags for 'config:search.settings'

Oh I'll leave for you then.

@smustgrave, leaving it to me is not a good idea. When I said I was stuck I meant it. I do not know how to fix the remaining test errors.

smustgrave → credited catch → .

Crediting @catch for helping with the user-role cache tag issue. https://drupal.slack.com/archives/C04CHUX484T/p1701362984050229

A few more modules may be removed from the install list, please find details at https://git.drupalcode.org/project/drupal/-/merge_requests/5625#note_238407 .

@marvil07 , thanks for pointing out that the list of modules could be reduced. I did my own testing as well and adjusted the list.

but I am not quite sure how they can be used without declaring them; maybe it they are enabled indirectly?

As a functional test using BrowserTestBase this uses the moduleInstaller which will install modules that are declared as dependencies.

Removal of additional modules seems good.

This is kind of defeating the purpose 😅 The purpose was to ensure that Page Cache works well in real scenarios, and that's best served by the Standard install profile (or Umami at this point).

However, if there's a significant speed-up, I'd agree this is worth it — a fine trade-off. 😊 👍

What's missing from the issue summary: the actual speed-up is of GitLab CI test runs?

@marvil07 before we open a new MR can we agree if the test coverage is being lost by changing this.

And if we need to update can use the existing MR as it's pointing to the correct 11.x branch. Just to avoid multiple branches floating around.

So wonder if we should copy the existing test to standard folder

And keep what's specific for page_cache in this file.

What's missing from the issue summary: the actual speed-up of GitLab CI test runs?

This test class only has a single test method and isn't one of the very slowest tests, so it's more about overall load than job speed.

gitlab does record how long a test method takes to run, but it's not that easy to find:

https://git.drupalcode.org/project/drupal/-/pipelines/59193/test_report

Click through to functional tests, then click through the pages, until you get to PageCacheTagsIntegrationTest - then would have to do the same for a core test run on the default environment too.

I assume that this timing includes ::setUp() but haven't actually confirmed that.

marvil07 → changed the visibility of the branch 2254209-before to hidden.

@smustgrave, hey sorry for the noise here 😅

I have been trying to answer the question from Wim.

Measuring

What's missing from the issue summary: the actual speed-up of GitLab CI test runs?

On gitlab CI, it is not as trivial to find out.

Using the codebase at the point the code changes started is a good way to compare the running time.
That is what I have tried, opened a new MR without the changes (I needed to add one so pipelines are triggered, but it is just adding a comment), only for comparison.

Locally, as reported above 📌 Fix test performance of Drupal\system\Tests\Cache\PageCacheTagsIntegrationTest Needs work , I see a time change from 24s to 18s on the changed test running time.

We could compare the total running time, but I am guessing that may have lots of other variables that could influence the run.
E.g. test runners may change in time, the actually used ones can differ, the context they are in at the time of the run is important for performance measuring, etc.

Having all that in mind, I see a jump from 12 minutes 14 seconds to 7 minutes 24 seconds.
That is unlikely explained by the test change, see above in this comment that the jump on a not-that-fast local computer is of just 6s.

@catch, thanks a lot for pointing that out.
I was surprised that there is no download to explore in a text file 😅

Looking at the results of the two runs of testPageCacheTags:

- without the change, MR 5709, reports 24.35s, on page 7 at https://git.drupalcode.org/issue/drupal-2254209/-/pipelines/60135/test_r...
- with the change, MR 5625, reports 9.17s, on page 59 at https://git.drupalcode.org/project/drupal/-/pipelines/59193/test_report

Approach

@Wim Leers, re: #32:

If the idea is to test in a real scenario, and standard profile can represent that, we should document that the test is not expecting to be moved to a different profile.
Idem if the target profile is umami, but in that case it sounds way out of scope here.

I would suggest to actually use the topic branch, the test is not doing performance testing, it is ensuring right behavior.
That should not be affected by the context profile; but if that is the case, the test is likely wanted to be expanded to cover multiple scenarios, including with multiple profiles, and again that is likely out of scope of this issue.

- without the change, MR 5709, reports 24.35s, on page 7 at https://git.drupalcode.org/issue/drupal-2254209/-/pipelines/60135/test_r...
- with the change, MR 5625, reports 9.17s, on page 59 at https://git.drupalcode.org/project/drupal/-/pipelines/59193/test_report

This seems about right for the change to me. For one test it has very little impact at all, but if we do it for lots of tests (which we've been slowly doing) it will eventually make a difference.

Not sure what that means for the loss of coverage here though - it might be better to just move this test to the standard profile so it's taken out of the list of targets to move to the testing profile?

@catch, thanks for the feedback!

This seems about right for the change to me. For one test it has very little impact at all, but if we do it for lots of tests (which we've been slowly doing) it will eventually make a difference.

The way I think about it 💡: we run the test suite a lot every day, so any improvement over time running will be multiplied by the number of times it is executed, which is a lot, and therefore the change likely makes sense.
Here there is around 15 seconds gained on every testsuite run, let us say 10s, which multiplied by the runs for every MR push on any issue fork, plus the actual commits on core, is likely a reasonable gain itself.

Not sure what that means for the loss of coverage here though [...]

@Wim Leers, I guess this is a question for you.
Please find the Approach section in comment #34 above, with some ideas on next steps.

@Wim Leers feel free to move back if you disagree

But looking at the test it seems to just be about the functionality of page_cache, which wasn't lost because we are installing the appropriate modules.

If the concern is that the Standard profile doesn't have page_cache test coverage then a follow up could be opened to add a standard specific page_cache test I think. And probably could be expanded on to include the other modules and config that ship with standard.

Let's get a definitive answer from @Wim Leers first. I am setting back to NR.

I pinked @Wim Leers in Slack #contribute about this.

#38:

- without the change, MR 5709, reports 24.35s, […]
- with the change, MR 5625, reports 9.17s, […]

#42:

If the concern is that the Standard profile doesn't have page_cache test coverage then a follow up could be opened to add a standard specific page_cache test I think. And probably could be expanded on to include the other modules and config that ship with standard.

That was indeed my concern. But to be fair, it is a test owned by the page_cache module, not the Standard install profile.

Let me get out of the way here, but thank you for showing how much faster this made the test, that is still a valuable addition after my pushback in #32 😊

Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

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