Add performance testing

This passes locally but fails on gitlab with:

1) Drupal\Tests\drupal_cms_starter\FunctionalJavascript\PerformanceTest::testPerformance
TypeError: Behat\Mink\Driver\Selenium2Driver::__construct(): Argument #1 ($browserName) must be of type string, array given

So something is up with the gitlab configuration I think.

Had to fight gitlab a bit to get functional javascript tests running, short version of what needed to change after some dead ends:

1. Needs to copy some bits from the d.o gitlab templates to get the chromedriver service done, and also to set the right env var for the driver args.

2. A red herring where the mysql database refuses connections, these seems to be an intermittent issue that has nothing to do with this MR, but convinced me I'd broken something in the gitlab config.

3. Arguably out of scope but would have helped me here - added junit logging so the tests tab works on the pipeline: https://git.drupalcode.org/issue/drupal_cms-3491405/-/pipelines/359106/t... - requires a chance to one line of YAML we're already changing and a extra two lines of YAML... so only a little bit out of scope.

This run successfully fails with the wrong query count on a page cache hit, so proves the tests are running at all (not the case when I was thrashing around with gitlab).

https://git.drupalcode.org/issue/drupal_cms-3491405/-/jobs/3587342

This passes https://git.drupalcode.org/issue/drupal_cms-3491405/-/jobs/3588474

This is the most recent pass with the junit change included, not much difference on the job log itself but just for reference: https://git.drupalcode.org/issue/drupal_cms-3491405/-/jobs/3589032

Looks great to me.

RTBC!

Started opening issues for performance problems found via the tests.

📌 Try to reduce database queries from MenuLinkTree::load() Active (not actually found by the test, found it when profiling drush cr instead of the page I was supposed to be looking at)

📌 Add render caching for the navigation render array Active - possible quick fix for navigation.

This could use a follow-up to add the ability to run the performance tests locally too.

One minor thing, and needs a follow-up, then this is mergeable IMHO.

Opened the follow-up for database query assertions 📌 Add database query assertions to performance tests Active .

Any additional performance tests we want can be covered by the performance testing meta for now I think.

Applied the suggestion.

Builds are failing on cannot stat patches.lock.json which I don't think is related to the MR.

Well, that's...weird. I'll look into that, try to get it fixed, and commit this.

Adding all the issues I discovered as a result of adding the tests - some were a direct result, some spotted while looking at other things.

Perf test is failing due to:

1) Drupal\Tests\drupal_cms_starter\FunctionalJavascript\PerformanceTest::testPerformance
Failed asserting that 4 is identical to 2.
/builds/project/drupal_cms/recipes/drupal_cms_starter/tests/src/FunctionalJavaScript/PerformanceTest.php:54
/builds/project/drupal_cms/recipes/drupal_cms_starter/tests/src/FunctionalJavaScript/PerformanceTest.php:36

@phenaproxima the extra two cache gets are apparently because requesting the front page of Drupal CMS now redirects to /home - has something changed? Seems non-optimal to have an extra http round trip from the front page of the site. I can update the test expectations to match, but would be good to know what happened too.

So the new test fails are all because Drupal is bootstrapped twice when you visit the homepage - once for the redirect and once to visit /home. I opened 📌 Front page always redirects to /home Active and updated the test coverage here to directly request the /home URL and avoid the redirect. If we'd done that in the first place it wouldn't have picked up the change, but now it has, there doesn't seem to be a lot of benefit in testing the additional redirect vs. a more 'normal' request. Can add more test scenarios later anyway.

I also had to increase the cache get and cache tag counts for the editor request, assume this is due to different config changes made since the tests were written but didn't investigate in detail yet - it will be easier to diagnose why things are changing when this is running against MRs rather than here.

Adding a screenshot of what an OpenTelemetry trace looks like in the Gander dashboard, which can be added to a local ddev install via a one-liner documented here and will automatically start to receive traces when running this test locally.

We can add this to the hosted Grafana dashboard at https://gander.tag1.io/ too but that will take some pipeline work at a minimum.

I've added @todos for the known issues. Updated assertions for the increased cache get counts, CSS and JavaScript weight for the editor user on the home page. I think the MR should be green now but test takes a long time to run locally and need to head afk soon, so pushing what I have while waiting for the hopefully last local run to come back.

https://git.drupalcode.org/issue/drupal_cms-3491405/-/jobs/3678952

MR is green again. I haven't investigated all the increased assertion limits apart from the new front page redirect, but as above it would be better if MRs find out they're changing things to not have to investigate in retrospect.

Question about this one: is there a risk of this slowing down contribution if various changes alter the number of queries and whatnot, in ways that can't be anticipated? Should this perhaps be moved to a separate CI job that is allowed to fail? Or maybe the assertions just become looser?

re #19: It isn't an issue w/ Drupal core. It is much more stable though with the numbers and types of modules. So we won't know the answer to that with Drupal CMS until we get it in and start seeing results.

OK, I guess that's good enough for me. We can back it off if it becomes disruptive.

I am assigning this to @tim.plunkett for final review and commit, once we come out of commit freeze (which should end next week).

Question about this one: is there a risk of this slowing down contribution if various changes alter the number of queries and whatnot, in ways that can't be anticipated?

When the test fails, you need to update the assertions, then it's possible to see which commit improved or regressed things. There is some headroom in the css and js assertions (i.e. they round up) to allow for small CSS and JS changes not to change too much. But if something suddenly adds 50k of CSS or 25 database queries to a page, it's good to know about it. It doesn't mean things can't get committed that do this, they just can't silently make things worse and require someone to track it down months later when they get bad lighthouse scores or their site slows to a crawl.

It's a bit more of a pain with the query logging/assertions because there is more chance of commit conflicts etc. that was the reason to defer those to a follow-up - but I think that would be a good idea to add once coffee module issues are sorted and things are generally a bit more stable - when there's less queries overall, less chances of conflicts.

I'm okay with merging this but it looks like it needs to be synced back up with 1.x.

Rebased.

MR is green.

I'd like to merge this first thing tomorrow - can it be rebased one more time just to ensure that the most recent bunch of commits haven't broken its assertions?

Lots of merge conflicts in the rebase again, so just went for the merge commit this time to save time.

🐛 Privacy policy link goes to a 403 Active will probably start to fail when this is merged, but only in a good way in that things are improving.

Would like to commit but tests seem to be failing.

I think one risk of this is that if a dependency updates (remember, we don't pin and we use the relatively lax ^ operator), this test may just break out of nowhere. That's why I think it might make sense to have a follow-up that moves the performance testing into its own job.

All green on https://git.drupalcode.org/issue/drupal_cms-3491405/-/pipelines/370852 - did you rerun the failing jobs or something?

If we move the performance tests to their own job it will then be easier from there to add a job to populate the Grafana dashboard, so +1 to that. Opened 📌 Run performance tests in their own job Active .

phenaproxima → credited breidert → .

Finally! Merged into 1.x. Thanks for the hard work and persistence on this.

This is causing other issues to fail the pipeline due to performance:

📌 Move helpers in node_access_test.module and delete it Needs work
https://git.drupalcode.org/project/drupal/-/merge_requests/10633

#3493406-26: Add render caching for the navigation render array →

That was 🐛 Navigation Top Bar hides entity local tasks even if the user has no access to the bar Active , should be fixed now hopefully.

re #35: A small side note, stating that tests found regressions is kinda of saying that the tests are doing their job. No one likes random failures but if a test finds issues, it isn't necessarily the tests fault. I sense some resistance to the performance testing addition. We should be embracing these things.

@catch thanks!

@heddn in principle yes, however in this case if you see the fix the value for the performance test was not correct so every test on HEAD was failing. If you check the referenced issue @nikolay only move code from a procedural function to a helper in a test module, there was no functional change, so no way for that to affect this performance test.

#37: Resistance? Who's resisting?

The performance test is beneficial, but I insisted on adding it in a separate allowed-to-fail CI job for a few reasons:

• I myself don't fully understand how the performance tests work or how to properly fix them when they fail. It's not that I don't want to, but I simply do not have time for it, in light of all the work that needs to get done for Drupal CMS's stable release in January.
• We have a large number of dependencies that are not pinned, and that we don't control. Any one of them could make a change at any time which would break the expectations of the performance tests. This is a completely different situation from core, which has far greater control.
• Many of our contributors don't seem to be super comfortable with automated testing at all, let alone performance testing for which I can't yet offer any guidance. Seen this way, adding a potential new barrier to contribution would be counterproductive, slow velocity, and make more work for me.
• This is just my opinion, but I think performance is not going to be a priority for our target audience of marketers. It's just not. There are plenty of site owners out there who will gladly bog down their websites with gigabytes of JavaScript in order to make users have a particular experience, with no concern for performance. (It's exasperating, but it's the reality.) The fact that we are being more responsible than that, and trying to make Drupal CMS snappy, reflects well on us...but it need not be a priority.

So yes, I embrace the performance testing. I think it is a good thing. But it cannot become a barrier to our rapid iterating.

As of about 5 hours ago (the daily overnight test run), the performance test was passing in HEAD. https://git.drupalcode.org/project/drupal_cms/-/pipelines/375659

To clarify the test failure being discussed is unrelated to this issue or Drupal CMS, it's a case of mistaken identity.

There was a cross-commit in core HEAD where a new performance test was added for the navigation bar, and then another commit changed the behaviour of the navigation bar causing an additional cache get, without a fresh test run first.

This got missed for several hours until late Friday night, then I updated the test this morning. It could happen with any test coverage and is a symptom of us not using merge trains on gitlab for core yet (and the sheer number of random test failures in core making it harder to spot a non-random one getting introduced, and the length of the RTBC queue etc.). The test fix was just changing an assert expectation from 60 to 61, nothing complicated, but took a while to figure out which commit had changed things.

We have a large number of dependencies that are not pinned, and that we don't control. Any one of them could make a change at any time which would break the expectations of the performance tests.

Fwiw I think this is reasonable but it applies to literally all of the test coverage for Drupal CMS not just the performance tests. It might be worth opening an issue to discuss min/max testing or a job that tests with some pinned dependencies and gets gradually updated or some combination of this so it's easier to understand where new failures come from when they happen. At the moment Drupal CMS is moving faster than the contrib modules so it wouldn't be that noticeable but if a module starts moving fast it won't be.

This is just my opinion, but I think performance is not going to be a priority for our target audience of marketers. It's just not. There are plenty of site owners out there who will gladly bog down their websites with gigabytes of JavaScript in order to make users have a particular experience, showing zero concern for performance

This is true for some people but I think it's a narrow definition of 'marketers'.

Some people want visitors to actually enjoy using their sites, and their sites to rank highly in search engines (which depends on cwv) etc. and we should provide that, then if they ruin their sites with bloat it's their fault and not ours.

Have to say that klaro going from over 200kb of js, plus CSS, out of the box down to 0kb was great to see (and now 64kb when it's enabled instead of 200kb too) and I hope we're able to do more of that.

I'm open to making the performance testing a must-pass job, but we'd need to figure out the margins of error (like "how much JavaScript is too much"). I'm not yet clear on the benefits of doing things like exact query count assertions, but I would imagine that they, too, would need a pretty healthy level of forgiveness built in.

Worth discussing further, for sure. Another issue?

Sorry about the noise, I was just trying to link where it looked like the failures started and where I was seeing them.

I should have realized it wasn't this issue since this issue introduced those tests and would have been failing so it wouldn't have gotten in.

Also that is impressive on the klaro changes!

The issue that added the new performance tests in core was 📌 Add render caching for the navigation render array Active , not this issue which is against Drupal CMS. These tests can't fail in core test runs. They are related though because I noticed the navigation toolbar optimisation while working on these.

Opened 📌 Add a (daily?) job against development versions of dependencies Active

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