Speed up tests when possible

Created on 15 February 2023, almost 2 years ago
Updated 20 August 2023, over 1 year ago

Problem/Motivation

LinkFieldTest is a functional test with seven test methods, this means seven installs of core.
We could potentially have a single test method calling out to helpers, and just do the one install.

See 📌 Potentially speed up LinkFieldTest Fixed

Steps to reproduce

Proposed resolution

@joachim "That makes me wonder whether we should allow this more generally -- functional test classes which do the setup once for the whole class, not for each method."

  • Do something in base classes.
  • Or make this issue a meta issue to discover more tests to be refactored.

Remaining tasks

Needs discussion

User interface changes

API changes

Data model changes

Release notes snippet

📌 Task
Status

Needs work

Version

11.0 🔥

Component
PHPUnit 

Last updated about 1 hour ago

Created by

🇨🇳China jungle Chongqing, China

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Issue created by @jungle
  • 🇬🇧United Kingdom catch

    functional test classes which do the setup once for the whole class, not for each method

    Supporting this at the API level sounds good, but I'm not sure what it looks like with phpunit, it might be easy, it might be really hard.

    Probably a way to find possible candidates for the approach would be to identify functional tests with the highest number of test methods, those will benefit the most.

    There's a trade-off here between overall test run speed vs. how long an individual test takes to run locally. I didn't test on the other issue whether the doTest methods can be force-run with phpunit by themselves or whether that's blocked. However, it would be easy to hack a test locally to rename one of the doTest* methods back and run that, so an easy workaround compared to 1hr test runs on DrupalCI.

  • Status changed to Needs review almost 2 years ago
  • 🇨🇳China jungle Chongqing, China

    As a POC, added a new Trait (\Drupal\Tests\Traits\AllInOneTrait) with a test (\Drupal\Tests\Traits\AllInOneTrait::testAllInOne) to run methods in \Drupal\Tests\BrowserTestBase::$methodNamesToRunInOneGo in one go.

    So if a class wants to run methods in one go, just use AllInOneTrait and add methods into $methodNamesToRunInOneGo.

  • 🇬🇧United Kingdom longwave UK

    PHPUnit has setUpBeforeClass() which is only run once per class, maybe we could leverage that somehow? The issue is that setUpBeforeClass() is static and so can't call parent::setUp() which would otherwise be a quick workaround.

  • 🇨🇳China jungle Chongqing, China

    Note: setUpBeforeClass() is called before each test when used with @runTestsInSeparateProcesses See https://github.com/sebastianbergmann/phpunit/issues/3925#issuecomment-54...

  • 🇬🇧United Kingdom joachim

    From a DX POV, having separate tests is really good because you can more easily focus on the failing part. For example, if you have a debugger enabled, a monolithic test might mean you spend ages stepping through the earlier parts of the test before getting to the problematic bit.

    But debugging working tests is something that happens FAR less often than tests being run on d.org, so optimisation is important.

    This might be over-engineering but I was thinking of something like this:

    // in base class
    function testMultipleBrowserTests() {
      // find class methods that start with browserTest*
      // run each one
    }
    
    // in Test class
    
    function browserTestOne() {}
    
    function browserTestTwo() {}
    

    If a developer needs to run just one test, they can temporarily rename browserTestOne() to testOne() and then PHPUnit will pick it up as a single test.

  • 🇨🇳China jungle Chongqing, China

    Re #6, testMultipleBrowserTests might be empty. Running empty tests got a warning (suppressed in Drupal? tested, got no warnings).

    Adding testMultipleBrowserTests() to the base class(es) gets it running on every sub-class, which is not expected.

    So I prefer adding a trait as the POC in #3 with an extra property added to the base class.

    • The test only runs on subclasses using that trait.
    • Devs can rename the method and comment the corresponding method in the newly added property to run a single test.
  • 🇬🇧United Kingdom longwave UK

    Another option here is to profile the functional test setup code and see if we can identify bottlenecks and/or add caching or reuse somewhere that would help all tests, instead of having to rearchitect each individual test.

  • 🇬🇧United Kingdom joachim

    > So I prefer adding a trait as the POC in #3 with an extra property added to the base class.

    Yup, you're right. Sorry, I dove in without reading the patch first :(

    > Another option here is to profile the functional test setup code and see if we can identify bottlenecks and/or add caching or reuse somewhere that would help all tests, instead of having to rearchitect each individual test.

    There's this issue: 📌 [Meta] Perform set-up tasks in Browser tests using API calls rather than browser requests Active .

  • 🇬🇧United Kingdom catch

    There's 📌 Improve performance of functional tests by caching Drupal installations Needs work and 🌱 [meta] Improve WebTestCase / BrowserTestBase performance by 50% Needs work .

    If we try to share more between tests, there's broadly three levels:

    1. The install profile.
    2. Any additional modules installed.
    3. ::setUp()

    1 and 2 really need to be done together, since benefit is from loading a database dump and bypassing the installer, even if there's more duplication once you take modules into account.

    For #3 I think this issue might be the best we can do, and it would be compatible with caching database dumps at a lower level of granularity.

  • Status changed to Needs work almost 2 years ago
  • The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

    Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

  • 🇬🇧United Kingdom joachim

    There's also this issue: 📌 Add a flag to set up test environment only once per test class; introduce setUpBeforeClass() and tearDownAfterClass() [PHPUnit] Needs work , which is a less Drupalism-ish way of doing multiple tests without repeating setup tasks.

Production build 0.71.5 2024