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.