- 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 11:44am 15 February 2023 - 🇨🇳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 thatsetUpBeforeClass()
is static and so can't callparent::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 9:32pm 17 February 2023 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.