Problem/Motivation
CoreRecipesTest appears to be the single longest running functional test, this is because it uses a dataprovider to install 25 different recipes, meaning core has to be installed 25 times.
See https://git.drupalcode.org/project/drupal/-/jobs/2157400 test run duration 4m25s. All other functional test jobs tend to finish in 4m or less.
vs https://git.drupalcode.org/project/drupal/-/jobs/2157401 4m
or https://git.drupalcode.org/project/drupal/-/jobs/2157404 3m29s
Steps to reproduce
Proposed resolution
I tried to ditch the data provider and just apply every recipe twice in a foreach loop instead, but got:
1) Drupal\FunctionalTests\Core\Recipe\CoreRecipesTest::testApplyRecipe
In ConfigConfigurator.php line 47:
The configuration 'block.block.claro_messages' exists already and does not
match the recipe's configuration
Can we do something more like GenericTest where we add a test for every recipe, but where that test is a minimal subclass of a recipe test base class?
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 3462383-nr-bot.txt | 12.29 KB | needs-review-queue-bot |
| #2 | failed-attempt.patch | 1.89 KB | catch |
Issue fork drupal-3462383
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:
Comments
Comment #2
catchUploading where I got to as a patch just for documentation of what didn't work, but pretty sure that's not the way to do this, however no idea what recipes shipping tests would look like either.
Comment #5
phenaproximaWe were hoping to eventually do something like this, but it wasn't a priority. But since core's test performance is being impacted, I guess this just became more important.
I would propose the following conventions and rules for recipe tests:
@groupannotation should be suffixed by_recipe. (I don't think this needs to be enforced, but it makes sense as a convention.)Drupal\Tests\Recipe\Core\recipe_name\(Functional|Kernel|FunctionalJavascript)namespace.Drupal\Tests\Recipe\recipe_name\(Functional|Kernel|FunctionalJavascript)namespace (this is for another issue to decide; I'm not adding support for non-core recipes in this issue right now).Comment #6
catchLooked at https://git.drupalcode.org/issue/drupal-3462383/-/pipelines/228183 (the last green test run)
First functional tests job took 3m41s - so that's 50 seconds less. Obviously the time is just distributed amongst the runners more evenly, but splitting tests like this should mean we use the runners more efficiently (i.e. minimises the time at the end of a job where just one test is still running) as well as letting the entire pipeline return quicker (the restriction on total pipeline runs is determined by the single slowest test, at least unless we significantly speed up installs).
https://git.drupalcode.org/issue/drupal-3462383/-/jobs/2162836
Left a couple of nits on the MR but it looks great to me.
I am not close enough to the recipes system to have a strong opinion on where and how to ship tests with recipes so will leave that bit for someone more involved - it seems like a good idea to support it to me but that is as far as I get.
Comment #7
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #8
phenaproximaComment #9
alexpottIt's with sad regret about test autoloading that I rtbc this. Nice work @phenaproxima within the confines of our system.
Comment #11
catchComment #14
catchCommitted/pushed to 11.x and 11.0.x, thanks!
This doesn't cherry-pick to 10.4.x, but I'm not sure how much we're going to be changing recipes or their tests in 10.4.x anyway so maybe that's fine? Marking fixed but not opposed to a backport if people want to.
Comment #17
catchThis is registering functional tests as unit tests, opened #3466169: Generic recipe functional tests ar discovered as a unit test.
Comment #18
catchTried a quick MR on #3466169: Generic recipe functional tests ar discovered as a unit test but it will need a bit more work. Re-opening this because I've reverted it from 11.0.x for now - we're going to need to change the namespace the classes are registered in so it's a test API change. Will need an 11.0.x release note although probably won't affect any contrib recipes because it's only been in 11.0.x for a week or so.
Comment #21
catchFollow-up MR up for review.
Comment #22
smustgrave commentedSeems to have gone through rounds of review
The change to core/lib/Drupal/Core/Test/TestDiscovery.php seems straight forward enough.
Going to mark it.
Comment #24
longwaveCommitted ccca0ab and pushed to 11.x. Thanks!
Marking PTBP as this needs a new MR for 11.0.x.
I think #3466169: Generic recipe functional tests ar discovered as a unit test can be closed as duplicate.
Comment #26
catchBackport is up - just had to cherry-pick two commits from the 11.x MR without the additional revert in 11.0.x because that was already in HEAD.
Comment #28
catchWent ahead and committed/pushed the backport to 11.0.x since there were no changes from 11.x except the sequence of commits.