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

Issue fork drupal-3462383

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:

Comments

catch created an issue. See original summary.

catch’s picture

StatusFileSize
new1.89 KB

Uploading 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.

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

phenaproxima’s picture

Status: Active » Needs review

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?

We 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:

  • They can be kernel, functional, or functional JavaScript tests. (I can't think of any way that recipes would benefit from unit tests, but I don't think we need to somehow enforce that recipes can't have them too.)
  • Their @group annotation should be suffixed by _recipe. (I don't think this needs to be enforced, but it makes sense as a convention.)
  • Core recipes are in the Drupal\Tests\Recipe\Core\recipe_name\(Functional|Kernel|FunctionalJavascript) namespace.
  • Non-core recipes are in the 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).
catch’s picture

Looked 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.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new12.29 KB

The 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.

phenaproxima’s picture

Status: Needs work » Needs review
alexpott’s picture

Status: Needs review » Reviewed & tested by the community

It's with sad regret about test autoloading that I rtbc this. Nice work @phenaproxima within the confines of our system.

catch credited longwave.

catch’s picture

  • catch committed 701d8f16 on 11.x
    Issue #3462383 by phenaproxima, catch, alexpott, longwave:...

  • catch committed e47f22df on 11.0.x
    Issue #3462383 by phenaproxima, catch, alexpott, longwave:...
catch’s picture

Version: 11.x-dev » 11.0.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/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.

Status: Fixed » Closed (fixed)

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

catch’s picture

This is registering functional tests as unit tests, opened #3466169: Generic recipe functional tests ar discovered as a unit test.

catch’s picture

Status: Closed (fixed) » Needs work

Tried 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.

  • catch committed 408df6b7 on 11.0.x
    Revert "Issue #3462383 by phenaproxima, catch, alexpott, longwave:...

catch’s picture

Version: 11.0.x-dev » 11.x-dev
Category: Task » Bug report
Priority: Normal » Major
Status: Needs work » Needs review

Follow-up MR up for review.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Seems 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.

  • longwave committed ccca0abb on 11.x
    Issue #3462383 follow-up by catch: CoreRecipesTest is slow
    
longwave’s picture

Version: 11.x-dev » 11.0.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Related issues: +#3466169: Generic recipe functional tests ar discovered as a unit test

Committed 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.

catch’s picture

Status: Patch (to be ported) » Reviewed & tested by the community

Backport 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.

  • catch committed d3c711f6 on 11.0.x
    Issue #3462383 by phenaproxima, catch, longwave, alexpott:...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Went ahead and committed/pushed the backport to 11.0.x since there were no changes from 11.x except the sequence of commits.

Status: Fixed » Closed (fixed)

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