Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
run-tests.sh has logic to split the list of tests to be executed to separate bins that can be run in parallel to leverage GitLabCI parallelization capabilities.
This logic is intertwined with the test discovery process and cannot be tested ATM.
Proposed resolution
- separate test allocation to bins into a TestWorkAllocator class, that takes as input the output of the test discovery
- extend the logic to sort slow vs normal tests also to test discovery methods that use '--class', '--file' and group selection
- add unit testing for the new class
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|
Issue fork drupal-3538002
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:
- 3538002-11.x
changes, plain diff MR !15683 - 3538002-run-tests.sh---separate changes, plain diff MR !12842
Comments
Comment #3
mondrakeComment #4
smustgrave commentedRecommended way to test this one?
Comment #5
mondrake#4 same as #3515347-17: Reduce run-tests.sh complexity in spawning subprocesses.
Comment #6
mondrakeComment #7
mondrakeComment #8
mondrakeComment #9
mondrakeComment #10
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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 #11
mondrakeComment #13
dcam commentedThis was a rough one. I've looked at it a few times over the last several months and never actually made it all the way through a review. But I vowed to get it done today.
I found a couple of things to comment on in the MR and am setting the status to Needs Work.
The array indentation in the new test's data provider is inconsistent. That made it kind of hard to review, especially the case that was ~1400 lines long. I had to fix some of it locally in order to parse what I was seeing. I don't know if there's a way to automatically indent those arrays properly, but if there is then it would help.
I did my best to review the code moves and changes. Overall I think they look good. The test results look good. I rebased the MR and then compared the test results to those from the latest commit to Core from this morning. There were minor differences between them, so it wasn't comparing apples to apples. But I assumed from the beginning that would happen. For instance, in the Kernel tests the list of discovered classes was slightly off when I diffed them. And that resulted in tests getting allocated to different bins. This isn't a problem. The number of tests that were discovered each type was exactly the same. And the tests were assigned to the bins in round-robin fashion as designed. So I'm convinced that the test discovery and allocation is working properly.
Comment #14
mondrake@dcam thanks for your effort. Yes working with this script is painful. Hopefully we're on a good path in moving this script tasks into dedicated unit testable subclasses. In this case, separating the allocation of test into separate parallel bins into this
TestWorkAllocatorclass could potentially open up for a later alternative implementation like #3040694: Use previous PHPUnit JUnit results to pack tests in bins when parallelizing.Comment #15
dcam commentedI can't provide a suggestion for fixing the array indentations. The file is too big. It makes the browser lag and there's too much too select. So I made a patch that should apply on top of the MR. It's attached. Please consider it and double-check my work. There were a lot of changes. I already caught one section that I missed on a review of it and had to recreate the patch.
Comment #16
mondrake#15 that’s just data, that was dumped with a var_export call. We shouldn’t be worrying about reviewing it IMHO. Sorry it caused such trouble.
Maybe we can do something like core/tests/Drupal/Tests/Component/Utility/RandomTest.php - dump the data in JSON format in a separate fixture file, and remove it from code.
Will do.
Comment #17
mondrakeDone #16 - now I think the MR should be more readable.
Comment #18
dcam commentedYes, thank you very much for coming up with this alternative option and enacting it. It's a lot easier to compare the outputs of the test cases and understand what's going on now.
My feedback has been addressed. This looks good to me.
Comment #19
longwaveAdded some feedback/suggestions. There are a lot of comments that don't appear to add much.
Comment #20
mondrake@longwave I addressed some of your points - there are still 3 more that would mean a wider refactoring of the algorithm that I would suggest to defer to a follow-up. Here the main intention was to move code away from run-tests.sh to a class of its own; doing refactoring of the new class will be simpler.
Comment #21
mondrakerebased
Comment #22
dcam commentedI reviewed the recent commits. The changes look OK to me. @longwave's feedback was addressed.
For what it's worth, in code-move issues my preference is to limit changes to that code as much as possible. In my opinion it makes reviews easier. But I understand not everyone may share that sentiment.
Comment #23
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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 #24
mondrakeRebased after #3536964: run-tests.sh - segregate command line parsing and use Symfony Console classes.
Comment #26
longwaveCommitted and pushed 7410fa37545 to main. Thanks!
Doesn't apply cleanly to 11.x, but I'm assuming we're not going to backport this sort of thing now. Please reopen if that's not the case!
Comment #29
mondrakeIt may be good to backport to 11.4 since I think contrib is willing to get #3549601: Allow --directory and @group to work together in run-tests.sh there too - maybe @jonathan1055 has an opinion?
Comment #30
mondrakeComment #32
mondrakeComment #33
smustgrave commentedSeems like a fine backport.
Comment #34
quietone commentedThe special title docs states that "[backport]" is for "The issue is for porting a patch from another issue to a previous version of the project.". Since this is the same issue, I am removing that from the title.
Comment #35
catchDiscussed briefly with @longwave - going ahead with the backport here to make the backport of #3549601: Allow --directory and @group to work together in run-tests.sh easier.
Comment #40
catch