Skipped tests should make PHPunit job fail

Here is the initial pipeline. I faked an error in the job by setting MINK_DRIVER_ARGS_WEBDRIVER: '--'
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

'next minor' at core 11.2 fails with Driver info: driver.version: unknown. All the other jobs just skip the javascriopt test and end green.

Adding --fail-on-skipped into $_PHPUNIT_EXTRA causes all the jobs to fail as required.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...
The above is for _PHPUNIT_CONCURRENT=0 which runs the native phpunit binary

When testing with _PHPUNIT_CONCURRENT=1 using run-tests.sh we get ERROR: Unknown argument '--fail-on-skipped'
https://git.drupalcode.org/issue/gitlab_templates_downstream-3515487/-/j...
So that needs to be a condition.

Both d9-basic and d10-plus use the default concurrent=0, so I think it would be better test coverage if one of them used run-tests.sh

Agree to have both usages of _PHPUNIT_CONCURRENT in our branches for testing.

We should definitely document that behaviour of not failing on skipped tests and the option to enable it, either by setting _PHPUNIT_EXTRA or changing the PHPUnit configuration file.

I have added a paragraph abut skipped tests in the doc issue
https://git.drupalcode.org/issue/gitlab_templates-3473000/-/blob/3473000...

The change to only add the option when _PHPUNIT_CONCURRENT=0 is tested here and the skipped tests fail as required -
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

And when running with _PHPUNIT_CONCURRENT=1 the option is not added, and the skipped tests have no effect (as per the existing behavior)
https://git.drupalcode.org/issue/gitlab_templates_downstream-3515487/-/p...

Do you have a preference for which out of d9-basic and d10-plus gets changed to use _PHPUNIT_CONCURRENT=1? There are more variants being tested in the d10 branch, and it includes Drupal 11, but concurrent=0 is the default setting. I'm not sure it makes a big difference which we pick?

I've just realised that MINK_DRIVER_ARGS_WEBDRIVER has no effect when running concurrent with run-tests.sh. The second pipeline in #6 passed all tests, inlcuding the javascript ones. So we should definitely leave _PHPUNIT_CONCURRENT=0 for the d10-plus branch, otherwise we'd have no coverage for the w3c driver issue 📌 Update PHPUnit jobs to new driver when D11 "current" is W3C-compliant Needs work

I've removed the temporary bits. Here is the regular pipeline on the MR
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

Here is the pipeline from UI, where I set the two variables to cause javasecript tests to be skipped
In the log show-environment-variables we see

MINK_DRIVER_ARGS_WEBDRIVER=-added-in-UI-
MINK_DRIVER_ARGS_WEBDRIVER_LEGACY=-legecy-added-in-UI-

https://git.drupalcode.org/issue/gitlab_templates_downstream-3515487/-/p...
The jobs failed as required.

This is ready for review.

Thanks for the tests. Yeah, agree with leaving _PHPUNIT_CONCURRENT=0 for the d10-plus branch, so this is RTBC.

Actually, we can set _PHPUNIT_CONCURRENT=1 for the d10-plus, just for the "max PHP version" variant. That runs the same Core version as "current" which gives us test coverage for MINK_DRIVER_ARGS_WEBDRIVER over there. It means we cover both methods of running phpunit tests within d10-plus which is highly desirable. I've pushed that change and here is the job running concurrent=1

Even better. Having both 0 and 1 for the same project is a great way to see that both options should work, even if we're talking of a small set of tests, but the set up, etc is what matters here.

When cherry-picking to d9-basic, do we need to have a variant running _PHPUNIT_CONCURRENT=1? That branch only runs two variants - "current" which is Drupal 10, and "previous major" which is 9.5. I think both of those should remain with _PHPUNIT_CONCURRENT=0 so that the MINK_DRIVER_ARGS_WEBDRIVER variable is tested in both of the phpunit jobs. We could opt in to 'max PHP' which would be defined to test Drupal 10 at the maximum php which I think is CORE_PREVIOUS_PHP_MAX: '8.4'. This could be a useful extra variant in the d9 branch. What do you think?

100% with that suggestion. That way we’ll check both options within a “current” variant.

There seems to be a lot of unpredictability in pipelines at the moment. Several composer jobs fail and are autmatically re-run and pass. But some never do.

Fetch step
camelcase@npm:6.3.0: The remote server failed to provide the requested resource
Response Code: 403 (Forbidden)
Request Method: GET
Request URL: https://registry.yarnpkg.com/camelcase/-/camelcase-6.3.0.tgz
section_end:1743524709:fetch_step
camelcase@npm:6.3.0: The remote server failed to provide the requested resource
touch $_WEB_ROOT/core/.env
touch: cannot touch 'web/core/.env': No such file or directory

Look at the number that failed here on the branch after I merged MR15
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/48...
But I do think it could have been caused by the commit, so I will continue, and assume all will be OK later.

MR16 is ready for review
https://git.drupalcode.org/project/gitlab_templates_downstream/-/merge_r...

To test the change I triggered a pipeline from UI with incorrect mink webdriver values, causing the javascript tests to be skipped. The jobs fail. Note - Max PHP ends with a warning, because the tests are skipped. It is running concurrent=0 for every variant because the UI form variable takes precedence over the job-level variables
https://git.drupalcode.org/issue/gitlab_templates_downstream-3515487/-/p...

Another from UI, this time with concurrent=1 and also bad mink driver args - all jobs end green because run-tests.sh does not cater for the 'fail-on-skipped' concept
https://git.drupalcode.org/issue/gitlab_templates_downstream-3515487/-/p...

Maybe if run-tests.sh has a long future, which I presume it does, we could ask for that enhancement? It should be quite easy, just pass the argument through to phpunit. Or is there a way we can do that from this end? It's a feature which is quite important for control of pipeline success/failure.

There seems to be a lot of unpredictability in pipelines at the moment.

Yes, dockerhub needs to mark our account in a certain way so we don't hit limits. This was sudden, unexpected and Tim (@hestenet) is dealing with that atm.

Maybe if run-tests.sh has a long future, which I presume it does, we could ask for that enhancement?

At the very least, it's worth opening an issue in core to discuss it.

The changes on this MR look good, so I'm marking it RTBC.

Thanks, merged.

I see now it is being discussed in this slack thread about docker.

Given than our Drupal 7 template only uses run-tests.sh there is nothing to port to main-d7.yml. And the d10-profile and d10-theme branches do not run any phpunit tests. So this is all done now.