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
After commit of #3477529: [CI] Remove the 'with-database' requirement for unit tests, we do not need to run Unit tests for each db configuration anymore.
Steps to reproduce
Proposed resolution
Move PHPUnit unit tests and PHPStan Drupal rules tests from the child pipeline (where other tests are executed) to the lint section of the main pipeline.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #58 | 3484966-nr-bot.txt | 1.73 KB | needs-review-queue-bot |
Issue fork drupal-3484966
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:
- 3484966-ci-introduce-a
changes, plain diff MR !10020 - 3484966-no-addl-pipeline changes, plain diff MR !11188
Comments
Comment #3
mondrakeComment #4
mondrakeComment #5
mstrelan commentedI like this in principle, but worry it will be harder to find the test results summary. Currently you need to go to the child job so see that, now you may need to go to two child jobs. I wonder if there's any way to bubble up the test results to the parent job.
Comment #6
mondrakeAdded parallel:matrix to be able running unit test on multiple PHP versions.
This is highligthing PHP 8.4 deprecations, for instance.
Comment #7
mondrakeLatest commit to the MR adds a job to run tests for Drupal components directly in PHPUnit, including code coverage report.
Probably needs to be spun off to a follow up, just wanted to showcase it.
Comment #8
mondrakeRemoved #7.
Comment #9
longwave@mstrelan yeah I kinda agree that finding test results is already quite tricky when you don't know where to look, and this makes it more complicated. There is no way to surface child pipeline results in a parent yet, see https://gitlab.com/gitlab-org/gitlab/-/issues/363019
Comment #10
smustgrave commentedQuestion how come phpunit had to be updated in composer?
Comment #11
mondrakeThat one is the composer.json of the PHPStan rules testing, not core's. That is separate from the rest to allow independent testing from core. Being independent, it's also not bound to core's dependencies and it happens PHPUnit 11 can be used for this without any of the hurdles that core has: #3465125: [PP-1] Make PHPStan rule testing use PHPUnit 11. Here's is not strictly necessary, but I made that change to 'force' the PHPStan rule testing job to run as that job is based on changes in the PHPStan rules directory.
Comment #12
smustgrave commentedThink I see what @mstrelan and @longwave are talking about now. Anyway to get the results https://git.drupalcode.org/issue/drupal-3484966/-/pipelines/335642 to the rest of the tests?
Comment #13
mondrakeNot until upstream gets #9 done, I am afraid.
Comment #14
smustgrave commentedShould this be postponed or is there a larger gain to be add with moving it out?
Comment #15
mondrakeThe gain I see is to be able to test future versions of PHP through matrix (and potentially PHPUnit, too) on unit tests only, earlier than the rest of the test suite, and highlight deprecations in advance without failing overall testing job.
For example, PHP 8.4 is going to be delivered the day after tomorrow, and there is not yet a regular job testing with it AFAICS.
Comment #16
smustgrave commentedThat does seem worth it, not 100% I can make the call but maybe @longwave?
Also the current MR needs a manual rebase.
Comment #17
andypostTo have a proper matrix we need to unify image names too
Comment #18
smustgrave commentedGoing to go on a limb here
Comment #19
mondrakePer #17, adjusted after commit of #3488570: Release ubuntu images for PHP 7.4, 8.0, 8.1 and 8.2. Leaving at RTBC.
Comment #20
mondrakeAfter #3488401: upgrade prophecy to 1.20, unit tests pass on PHP 8.4 so it's no longer necessary to let them fail in the job
Comment #21
mondrakeComment #22
mondrakeJust found that instead of running all tests on both 8.3 and 8.4, actually the tests get spread across the two.
Comment #23
andypostadded suggestion and asked why phpunit-11 is not follow-up?
Comment #24
mondrake--ci-parallel-node-* args in run-tests.sh conflict with gitlab matrix. Since we do not need to break out unit tests in multiple jobs, removed the args.
Comment #25
mondrake@andyoost re #23 see #11.
Comment #26
mondrakePHPUnit version for PHPStan testing is rather irrelevant, but I removed allowing PHPUnit 11 from the MR to make things more straightforward. Since a change in the PHPStan directory is needed to show how the test job would trigger, I changed a comment in a test class instead.
Comment #27
andypostThere's a list of deprecations for PHP 8.5 already https://wiki.php.net/rfc/deprecations_php_8_5
So it makes sense to catch them in core or via https://github.com/php/php-src/pull/10050
Comment #28
mondrake#27 when there will be a PHP 8,5 docker image available, we can easily add it to the job matrix here, allowing failures. That will help showing deprecations for 8.5 while keeping the entire pipeline green.
Comment #29
andypostThe last PHP 8.4 fix been commited, so the only question is should we add a daily or on-commit run via #3488473: Add core testing using PHP 8.4
Comment #30
andypostI find it ready!
PHPUnit 11 has own meta issue #3418267: Support PHPUnit 11 in Drupal 11
Comment #31
alexpottIt would be great if we can define the matrix in the root .gitlab-ci.yml file - means all the php version stuff is in one place.
Comment #32
mondrakeComment #33
smustgrave commentedBelieve the 1 open thread has been answered.
Comment #35
catchWanted to see how this will affect pipeline times, and wondered what happens in terms of job priority between the two child pipelines and whether that depended on definition order. As far as I can tell there is none, so we should just ago ahead here.
However when doing that I agree with comments above that having to click through child pipelines is tricky.
Could we manually define multiple phpunit jobs as part of the lint stage instead? We can set things up so they don't block the test pipelines from running, but just immediately run alongside the other lint jobs.
Comment #36
mondrake#35 wouldn't
parallel matrixsetup multiply then also the other lint jobs per each PHP version? Or the idea is to drop that setup and hardcode the jobs' PHP versions in separate but equal jobs?Comment #37
catchYes that's what I was thinking. It wouldn't look as tidy in the YAML, but it would look nicer in the gitlab UI. I had separately thought about moving unit tests into the lint stage to get ultra-fast feedback about test failures, but hadn't done it yet because there didn't seem enough reason to, but building in tests on different PHP versions + the findability issues with child pipelines seems like enough reason.
Comment #38
mondrakeI'll give it a try.
Comment #40
mondrakeMR!11188 seems to work. I won't say that I like it though, now the Lint pipeline is a wall of jobs and they all need to be complete to let the Test pipeline go. Also the Lint pipeline is taking a bit longer as the PHPUnit test jobs do take some more time than the rest of the linting ones.
Anyway, it's a compromise I guess. It looks like upstream is moving re. #9, maybe that will solve the problem.
Personally I like MR!10020 better because it helps breaking the pipeline in more atomic bits and this would help for instance to add testing i.e. PHPUnit 11 and/or paratest allowing failures, or splitting Unit tests between component and core tests like in #3485069: [CI] Spin off Drupal Components tests in a job of their own, add test coverage metrics. But yeah, needs patience.
Comment #41
catchPushed a couple of commits:
- moved the phpunit jobs into their own stage
- made the Test pipeline depend only on specific jobs in the Lint pipeline. Kept it so that unit tests only start once the entire Lint stage is finished, but we could make them depend on the phpstan + phpcs jobs only if we wanted
Looks like this:
https://git.drupalcode.org/project/drupal/-/pipelines/422396
If we didn't want the extra stage, we could revert that bit but keep the explicit dependencies for the test pipeline maybe?
Comment #42
catchhmm that's not actually working yet.
Comment #43
mondrakeWouldn’t test failures of unit tests be reported separately from the other tests anyway, though? because of separate pipelines?
Comment #44
catchThey will if they're in different pipelines, but if we block the child pipelines on phpunit, then the test failures will be visible in the parent, and it won't be possible to have failures in both.
Trying out a different approach to speed things up - just higher concurrency for the phpunit jobs.
Comment #45
catchOK PHPUnit jobs now finish in the same overall time as the other linting jobs - just needed higher concurrency. The 8 cpu runners will be re-used for the test stage, might actually work out less CI time overall once we take into account on-commit and daily tests.
Also moved things around a bit to hopefully make the lint stage more readable.
Comment #46
mondrakeRight. Just a remark then - if unit tests fail, after committing this the other tests will no longer be executed in MRs.
Looks good to me, but obviously cannot set to RTBC.
Comment #48
catchFor a follow-up, but I wonder if there's a way to merge the default tests pipeline into the parent - and only use the child pipelines for the non-default environment combinations. if we could do that, we could use needs: specific lint jobs for specific test jobs (e.g. kernel tests don't need CSS linting) and it would not require unit tests to pass to find out if functional javascript tests pass.
We'd have to factor out the parent pipeline references so they only get included by the child pipelines, and have equivalents for the non-child jobs, but that seems like it ought to be doable.
That would mean even less clicking through in that case.
Comment #49
catchComment #50
mondrakeIS and title need to be updated to reflect latest proposals.
edit - xposted with #49
Comment #51
mondrakeUpdated IS.
Comment #52
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 #53
mondrakeReroll
Comment #54
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 #55
mondrakeUpdated fork
Comment #56
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 #57
mondrakeI'd like not to spend time fighting with the bot. #56 is wrong, the MR fork is up-to-date with head.
Comment #58
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 #59
mondrakeComment #60
bbralaThis seems good, went through logs and didnt really notice anything wrong. We (castch, mstrelan) have discussed how one would run the child pipeline with failing unit tests, for now manually adding allow failure to the unit tests will be fine. I've seen few comment changes, but those all are minimal and an improvement. Setting to RTBC.
Comment #62
catchI think this is in a good spot. Let's see how it goes.
Committed/pushed to 11.x, thanks!
Comment #65
catchFollow-up: #3515704: [CI] Move unit tests to a 'unit tests' stage