[CI] Introduce a separate stage for unit tests

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

Added parallel:matrix to be able running unit test on multiple PHP versions.

This is highligthing PHP 8.4 deprecations, for instance.

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

Removed #7.

@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

Question how come phpunit had to be updated in composer?

That 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: 📌 [PP-1] Make PHPStan rule testing use PHPUnit 11 Postponed . 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.

Think 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?

Not until upstream gets #9 done, I am afraid.

Should this be postponed or is there a larger gain to be add with moving it out?

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

That does seem worth it, not 100% I can make the call but maybe @longwave?

Also the current MR needs a manual rebase.

To have a proper matrix we need to unify image names too

Going to go on a limb here

Per #17, adjusted after commit of ✨ Release ubuntu images for PHP 8.1 and 8.2 Active . Leaving at RTBC.

After 📌 upgrade prophecy to 1.20 Active , unit tests pass on PHP 8.4 so it's no longer necessary to let them fail in the job

Just found that instead of running all tests on both 8.3 and 8.4, actually the tests get spread across the two.

added suggestion and asked why phpunit-11 is not follow-up?

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

@andyoost re #23 see #11.

PHPUnit version for PHPStan testing is rather irrelevant, but I removed 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.

There'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

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

The last PHP 8.4 fix been commited, so the only question is should we add a daily or on-commit run via 📌 [PP-2] Add core testing withPHP 8.4 Postponed

I find it ready!

PHPUnit 11 has own meta issue 🌱 [meta] Support PHPUnit 11 in Drupal 10 Postponed

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

Believe the 1 open thread has been answered.

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

#35 wouldn't parallel matrix setup 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?

Or the idea is to drop that setup and hardcode the jobs' PHP versions in separate but equal jobs?

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

I'll give it a try.

MR!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]. But yeah, needs patience.

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.

Pushed 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?

hmm that's not actually working yet.

Wouldn’t test failures of unit tests be reported separately from the other tests anyway, though? because of separate pipelines?

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

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

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.

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

mondrake → changed the visibility of the branch 3484966-ci-introduce-a to hidden.

Right. Just a remark then - if unit tests fail, after committing this the other tests will no longer be executed in MRs.

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

IS and title need to be updated to reflect latest proposals.

Updated IS.

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

Reroll

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.

Updated fork

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.

I'd like not to spend time fighting with the bot. #56 is wrong, the MR fork is up-to-date with head.

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.

This 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. Setting to RTBC.

I think this is in a good spot. Let's see how it goes.

Committed/pushed to 11.x, thanks!

catch → closed merge request !11188

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

Follow-up: 📌 Move unit tests to a 'unit tests' stage Active