Problem/Motivation

After #3465602: Order tests by number of public methods to optimize gitlab job times, test classes within a test job will be ordered by number of test methods descending.

Data providers mean that the number of test method runs diverges from the number of test methods, so we currently have to use @group #slow to identify most tests with data providers (at least ones with significant setup tasks) so that they run first.

Steps to reproduce

Proposed resolution

If we did some additional reflection work, we could detect the @dataProvider annotation, and add a 'bonus' to the method count, like +5 or +10 or something. Doing this would allow @group #slow to be removed from those tests which only have @group #slow due to a data provider, leaving just the ones which really are genuinely slow to run (Umami installs et al).

Additionally, if we order by type of test, this should benefit contrib test runs that run multiple types of test in the same job with concurrency (which is how the default template works) - i.e. functionl js tests first and unit tests last.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3470179

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
Title: Include a check for data providers when ordering by method count » Account for test type, number of methods, and data providers in test ordering

catch opened merge request !9322

catch’s picture

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

Since this adds a bit of complexity, tried to tidy up the overall logic to make it more parseable at the same time.

catch’s picture

catch
he/him
Primary language English
commented

This keeps the four kernel test jobs at around 2 minutes each while removing @group #slow from dozens of tests.

https://git.drupalcode.org/project/drupal/-/jobs/2553451
https://git.drupalcode.org/project/drupal/-/jobs/2553452
https://git.drupalcode.org/project/drupal/-/jobs/2553453
https://git.drupalcode.org/project/drupal/-/jobs/2553454

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Went to review but needs a rebase

catch’s picture

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

Rebased.

Note the weighting by test type doesn't affect core tests, but I'm hoping it will allow us to optimise contrib test run times/resource usage.

smustgrave’s picture

smustgrave commented 
Drupal\Tests\image\Functional\ImageStylesPathAndUrlTest::testImageStyleUrlAndPathPrivateLanguage
Behat\Mink\Exception\ExpectationException: Current response status code is
403, but 200 expected.
/builds/project/drupal/vendor/behat/mink/src/WebAssert.php:888
/builds/project/drupal/vendor/behat/mink/src/WebAssert.php:145
/builds/project/drupal/core/modules/image/tests/src/Functional/ImageStylesPathAndUrlTest.php:315
/builds/project/drupal/core/modules/image/tests/src/Functional/ImageStylesPathAndUrlTest.php:113
FAILURES!

Appears random right?

smustgrave’s picture

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

Believe test failure is random.

catch’s picture

catch
he/him
Primary language English
commented

Rebased here after a few more test optimisation issues landed and to try to get a clean pipeline. Most recent pipeline on the MR now is green and took 5m41s https://git.drupalcode.org/project/drupal/-/pipelines/274771

catch’s picture

catch
he/him
Primary language English
commented

No longer applied. Rebase wasn't clean but merging in 11.x was.

Last pipeline - 5m29s which is as good as HEAD gets at the moment. https://git.drupalcode.org/project/drupal/-/pipelines/287590

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Couple of nits on the MR.

quietone made their first commit to this issue’s fork.

quietone’s picture

quietone commented
Status: Needs work » Needs review

Updated the MR per longwave's review.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

The changes look good. match(TRUE) still feels like cheating to me but it is a lot more compact so I just need to get used to it. I think I can RTBC since it was just tweaks.

  • longwave committed 94b717e9 on 11.x 
    Issue #3470179 by catch, quietone: Account for test type, number of...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Committed 94b717e and pushed to 11.x. Thanks!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 10.4.x-dev
Status: Fixed » Patch (to be ported)

Again, maybe we want to backport this to 10.4.x? Doesn't apply cleanly to run-tests.sh unfortunately.

catch’s picture

catch
he/him
Primary language English
commented
Status: Patch (to be ported) » Fixed

I was able to get a clean backport by first backporting #3465602: Order tests by number of public methods to optimize gitlab job times. Only use statements conflicted after that, so went ahead.

  • catch committed f2e7da64 on 10.4.x 
    Issue #3470179 by catch, longwave, quietone: Account for test type,...

Status: Fixed » Closed (fixed)

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