Add PerformanceTestBase for allowing browser performance assertions within FunctionalJavaScriptTests

added updated patch and try to fix CCF of MR #3

I got the test finally running on local. While doing that, I stumbled upon core/tests/README.md that needs to be updated to mention this new test type. But this really great stuffs.

1 small comment about a commented out line.

Added follow-up tag to address #7 unless it's agreed it can be added here.

We should add the docs here, and also I think we need to update phpunit.xml.dist to add the new test type alongside the others in there.

Apart from that there's the general issue of actually putting the performance data somewhere accessible.

Both me and @heddn were seeing 2.3-2.7 seconds for largest contentful paint. I cross-checked that against lighthouse on a 10.1.x install, and confirmed it's accurate, opened 📌 Improve Largest Contentful Paint in Umami front page Fixed to improve it. Once we fix that, we can see if the test can pick up the improvement.

A couple of recent updates:

- now sending time to first byte -> first paint and first paint -> largest contentful paint as spans to opentelemetry.

- added assertions for number of styles/scripts/images requested on the page. Not sure if the image assertion is really possible because chromedriver browser window could be non-deterministic potentially.

Meantime PECL auto-instrumentation extension is packaged https://github.com/open-telemetry/opentelemetry-php-instrumentation

Looks it could be useful for measuring other tests

I've added an additional test method + supporting code. We now collect the number of scripts, stylesheets and images via chrome network requests. Testing the Umami front page with a user logged in that has access to tours, I'm able to get a proper failure if I revert 🐛 Aggregation creates two extra aggregates when it encounters {media: screen} in a library declaration Fixed locally - i.e. the test expects four stylesheets but with that issue reverted there are six.

Added image support in there, but (ironically) lazy image loading makes it hard to assert on a specific number of images, because it depends what the browser engine decides needs to be loaded or not, and that appears to be slightly variables - sometimes I get 19, sometimes 20. Not sure what if anything to do about that yet - we can always put that data into OpenTelemetry even if it's not viable for assertions.

Here's a test only patch includes the diff from the MR and a revert of 🐛 Aggregation creates two extra aggregates when it encounters {media: screen} in a library declaration Fixed .

Now that it's possible to do useful assertions in phpunit (although I still need a failing patch that fails in the right way), I've split the OpenTelemetry part of this out to ✨ Add OpenTelemetry Application Performance Monitoring to core performance tests Fixed and 📌 Add open-telemetry/sdk and open-telemetry/exporter-otlp as dev dependencies Active .

Here's another attempt at a fail patch - code from the MR plus a revert of 🐛 Aggregation creates two extra aggregates when it encounters {media: screen} in a library declaration Fixed .

I think we're approaching a committable patch here so marking needs review.

Discussed with @heddn whether we could use a trait to add server-side metrics to functional/kernel and maybe unit tests.

I think this is doable. The chromedriver stuff in here wouldn't be relevant for those tests, but we can also make this a trait instead of adding an entirely new test suite. The only complication is having to override ::setUp() to re-enable css/js aggregation, so I moved that out to the Umami test. Also moved the Umami test to Umami itself.

OK issue with the recent test fails, which I wasn't seeing locally as dramatically, was that chromedriver stores the performance log until it's retrieved, then empties it out - so if you have http requests in-between drupalGet, you'd get the log for all of them, not just the drupalGet - this is easily solved by requesting the performance log prior to making the request, which resets it so you only get what you're expecting next time it's retrieved.

Attaching a fail patch that should fail properly this time - code from the MR + a revert of 🐛 Aggregation creates two extra aggregates when it encounters {media: screen} in a library declaration Fixed , you'll be able to see the extra two http requests causing the test to fail. We can apply the same pattern to database queries and assert on those, since that will be consistent across environments.

The last submitted patch, 22: 3346765-fail.patch, failed testing. View results →

1) Drupal\Tests\demo_umami\FunctionalJavascript\PerformanceTest::testFrontPageTour
Failed asserting that 4 is identical to 2.

That's what we wanted with the fail patch.

Back to needs review.

I'm good w/ this. Marking RTBC. But it still looks like we need several more reviews; which hopefully can happen soonish.

Untagging for release manager review since there's been a couple of +1s from @xjm and @quietone to the general concept elsewhere, and any dependency additions have been split out of here now. However the tests needs framework and front-end framework reviews still I think - it's a light patch but it's got implications once we keep going.

The last submitted patch, 22: 3346765-fail.patch, failed testing. View results →

Hiding the fail patch so it doesn't get retested and restoring RTBC.

After doing some extra testing I don't think we can add assertions on exact image count because I think chromedriver is slightly indeterminate there, however we might be able to figure out the maximum by running a test lots of times and seeing what level it fails at then using assertLessThan(). With opentelemetry we can also send the number of image requests as a trace attribute.

That should definitely be in a follow-up (which I'll open next week), question is whether to leave the supporting code in here or also move that to the follow-up.

I don't feel like we need to test it here in this patch. We could move that bit out into the follow-up. Or we could do a assertMoreThan(1) or something that will be fairly resilient to test changes.

Some typehinting nits, leaving RTBC

I surfaced some concerns. 😅

I looked at converting NoJavaScriptAnonymousTest and there is one problem:

We could disable those modules maybe but opened 📌 [PP-1] Convert NoJavaScriptAnonymousTest to use PerformanceTestTrait Closed: duplicate since it won't be a direct conversion of just the test.

Also opened 📌 Add xhr and BigPipe assertions to PerformanceTestTrait Active for the xhr and BigPipe assertions.

If @Wim agrees with the two follow-ups I think we're back to needs review here.

#32: hm, that makes me wonder whether we should this not as a trait, but as a new base class, because that'd allow us to not do \Drupal\FunctionalJavascriptTests\WebDriverTestBase::installModulesFromClassProperty(). But based on #20 that was highly intentional: to allow non-functional tests to use this too. But … I see no such uses yet?

That'd also allow every performance test to not have to worry about overriding ::setUp() to re-enable CSS/JS aggregation?

#33: Definitely agreed. 👍

Marking for the question at the start of this comment plus the missing documentation for the used webdriver arguments.

hm, that makes me wonder whether we should this not as a trait, but as a new base class, because that'd allow us to not do \Drupal\FunctionalJavascriptTests\WebDriverTestBase::installModulesFromClassProperty(). But based on #20 that was highly intentional: to allow non-functional tests to use this too. But … I see no such uses yet?

I think this and the demo_umami setUp() override are the clincher - we need a base class, although 'just' a base class this time not a new test type as I started with.

We can still put any shared logic with functional tests like database querylogging into a trait. Have made that change in the MR and hopefully addressed the other points too.

With the base class approach that allows 📌 [PP-1] Convert NoJavaScriptAnonymousTest to use PerformanceTestTrait Closed: duplicate to pass without overrides now.

All feedback addressed! LGTM.

That was a random failure.

This looks superb, but I have a few trivial nits and one semantical question 😊 This is on the verge of RTBC for me.

Do we really need to update core/tests/README.md like #7 and #8 were saying? 🤔

Do we really need to update core/tests/README.md like #7 and #8 were saying?

Ah no that's when I was considering a full new test type, over and above a base class, we don't need it for just a base class.

Doesn't that mean we should move this out of core/modules/system and into core/profiles/standard for consistency?

I thought about that but it felt out of scope... don't really mind either way.

I do think there's a good reason to move NoJavaScriptAnonymousTest (see comment on MR), but I defer to @catch on that, and it obviously shouldn't block commit.

I think this is an exciting step forward to avoid future front-end performance regressions 🤩 And it enables exciting next steps:

• 📌 Allow assertions on the number of database queries run during tests RTBC
• ✨ Add OpenTelemetry Application Performance Monitoring to core performance tests Fixed
• 📌 Add xhr and BigPipe assertions to PerformanceTestTrait Active

Let's do this? 😄

Opened 📌 Move NoJavaScriptAnonymousTest to the standard profile Fixed for the test move.

👍

Split image handling out to 📌 Add image request counts to PerformanceTestBase Needs work , it works but it's not entirely clear how to assert on it. Makes the diff here a dozen lines smaller. Leaving RTBC because this is pure removals to a follow-up.

👍

This patch issue is now doing a lot less than when I opened it, so moving the needs x review tags over to ✨ Add OpenTelemetry Application Performance Monitoring to core performance tests Fixed which is the one with more implications.

Would still love to see this one land! 😊

Issue credits

Pushed to 11.x, have asked RMs if this should go into 10.1.x too

larowlan → closed merge request !3617

Added a bare bones CR, feel free to expand https://www.drupal.org/node/3366904 →

Thank you! I bet it needs follow-up to write some docs, moreover it could be useful for custom development

Issues from summary could be unpostponed

Glad to see this land!

Looking at CI logs I see that no new tests are executed
PHP Notice: Undefined index: Drupal\Tests\system\FunctionalJavaScript\NoJavaScriptAnonymousTest in /opt/drupalci/testrunner/src/DrupalCI/Build/Artifact/Junit/JunitXmlBuilder.php on line 82

Moreover this code fails on PHP 8.3 📌 Fix tests broken on PHP 8.3 Closed: cannot reproduce

Filed follow-up 🐛 Fix namespace in NoJavaScriptAnonymousTest Fixed

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