Problem/Motivation

Core performance test are (somewhat ironically) some of the slowest functional javascript tests in core.

This is because they rely on creating somewhat realistic scenarios (standard and umami profile), and on the chrome performance log which has a lot of quirks. To avoid this, most of the tests only add performance assertions and opentelemetry logging for a single page/operation.

However, since PerformanceTestBase was added, we've fixed a lot of weird performance log timing issues and other race conditions, so it might be OK to try to consolidate things down to fewer methods or one per test class, with more requests/assertions in each.

Doing this for StandardPerformanceTest first since it's the smaller compared to Umami

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3463288

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review

Local ddev timings. Down from nearly 9 minutes to 1m43. On gitlab it currently finishes after about 4m10s so would expect it to finish in somewhere around 1 minute give or take.

Before:

../vendor/bin/phpunit profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php 
PHPUnit 10.5.26 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.8
Configuration: /var/www/html/core/phpunit.xml

...                                                                 3 / 3 (100%)

Time: 08:55.340, Memory: 6.00 MB

After:

../vendor/bin/phpunit profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php 
PHPUnit 10.5.26 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.8
Configuration: /var/www/html/core/phpunit.xml

.                                                                   1 / 1 (100%)

Time: 01:43.987, Memory: 6.00 MB

catch opened merge request !8885

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: +Test suite performance
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Oh nice, I like tests like this that call the functions. Wonder if we could make that more standard?

Change here looks good though, no loss of coverage.

  • longwave committed 816ac01b on 10.3.x 
    Issue #3463288 by catch: Consolidate test methods in...

  • longwave committed 12448d24 on 10.4.x 
    Issue #3463288 by catch: Consolidate test methods in...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Makes sense to me, as long as it doesn't affect the timings why reinstall over and over. Backported down to 10.3.x as a test-only change.

Committed and pushed 24b7c3ffa3 to 11.x and 64e80c8d84 to 11.0.x and 12448d24bc to 10.4.x and 816ac01be9 to 10.3.x. Thanks!

  • longwave committed 64e80c8d on 11.0.x 
    Issue #3463288 by catch: Consolidate test methods in...

  • longwave committed 24b7c3ff on 11.x 
    Issue #3463288 by catch: Consolidate test methods in...
smustgrave’s picture

smustgrave commented

Can I ask a good rule of thumb on when this would be preferred

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I don't think there is one, it's kinda case by case - it's suitable when the individual tests don't do much work but also we must ensure that combining them doesn't cause side effects where data from an earlier test leaks into a later one.

catch’s picture

catch
he/him
Primary language English
commented

An example of it not working is in #3463351: Consolidate Umami performance tests which I am currrently struggling with. Passes locally but fails with what looks like a race condition in the chrome driver on gitlab.

However if it's a functional or functional javascript test, and we don't need to make big changes for the test to work, then it makes everything run a lot faster.

Status: Fixed » Closed (fixed)

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

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#2: 🤯 Wow, now that is an epic speed-up! 😄