Deprecate TestDiscovery test file scanning, use PHPUnit API instead

We are hitting a PHPUnit bug here, The file attribute of node of XML test list can be wrong. So we still need to read the file of a test class from the classloader.

I think we can still avoid the classloader. On it.

Very interesting side effect of this - since PHPUnit discovery actually executes the dataproviders, and includes in the discovery list as many test cases as the number of datasets provided for a test method, we can now be precise in the number of test cases run for each test class, and use that information in run-tests.sh instead of the current approximate count.

This looks really good and cleans up quite a lot of mess.

I went ahead and added @group #slow to mainly kernel tests that are now ending up towards the end of the run. This is not a fault in the ordering logic at all, it's just a side effect that some tests that happened to run earlier in a test run now could run later either purely because other tests moved around, or subtle changes in the method counting logic.

The debugging logic at the top of the test runs is great and I wonder if we should just keep that all the time.

I was thinking in a follow up we could introduce a “weight“ to #slow (i.e. #slow300, #slow200, etc) and further tune the start sequence.

@mondrake I think at the moment this is OK because we have less #slow tests than we run tests concurrently per-job (e.g. we might have 5 slow tests but start 8 at the same time), however it took a while for that to be the case and it might not be the case again, so definitely worth an issue. Even now it might save a few seconds which is worth having.

We can leverage on PHPUnit API also to determine the test description, instead of doing separate parsing. On it.

Done #14.

Filed 📌 Introduce a “weight“ to #slow tests Postponed re #12 and #13.

merged with HEAD after commit of 🐛 Extend PhpUnitTestDiscoveryTest to test also PHPUnit API Active

Thanks for review @mstrelan, added some of your suggestions and replied inline to some other ones; NW to address the remaining comments.

Postponed on 🐛 Fix test annotations for tests introduced by #3485896 Active

Found some issues while testing this in combination with 🌱 [meta] Support PHPUnit 11 in Drupal 10 Postponed . Given that one is RTBC, let's postpone on that.

Blocker is in

Rebased. Since when this was RTBC, added logic for determining group metadata information under PHPUnit 11.

Adding this as a release priority because it will allow #[Group] to be used in contrib in combination with run-tests.sh (at least when 11.1 support is dropped).

Thanks for reviews!

I'd love to add @phpstan-type, but I am reluctant because of 📌 Forbid limited length primary and unique keys, allow only in indexes Needs work and ✨ Support PHPStan's annotations for local type aliases and array shapes Active .

No worries, it's a nice to have and shouldn't hold this up

removed duplicate @group #slow annotations

Made some updates to the CR to provide some input how to convert tests from annotation to attributes. Would be lovely to get a review there too.

CR looks great, nice one 🙌

There are failing tests in the latest build that are not familiar to me as intermittent failures, but they probably need a re-run attempt at least.

Only merged with head, now tests are green. There's fortuity in randomness of test failures.

I see only one unresolved MR comment from @larowlan about the file naming, but that comment was answered and seems to make sense to me. RTBC.

OK I read through this again. It's a really nice clean-up of test discovery.

Personally I am excited about the improvements to test ordering, both for core test suite performance and also because it unblocks potential improvements in contrib as described in 📌 Lower default concurrency Active .

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

catch → closed merge request !10809

I think this may have broken pipelines, e.g. https://git.drupalcode.org/issue/drupal-3046670/-/pipelines/505134 it's not possible to see the default test job. Reverting for now.

catch → reopened merge request !10809

Can't figure out what would be impacting pipelines here - the MR one after revert and rebase seems to work fine. Pipeline config is not touched apart from the addition of a parameter when calling run-tests.sh.

Yeah it doesn't make sense to me either - going to try again and trigger a couple of pipelines on different MRs in case it was a one-off.

Seems OK now, I think it was a false alarm/ gitlab itself having a problem.

Do we need publishing the CR?

Change record appears to already be published 👍

Filed 🐛 PhpUnitApiGetTestClassesTest and PhpUnitApiFindAllClassFilesTest need to execute PHPUnit discovery before TestDiscovery Active after finding an issue while working on the followup conversions from annotations to attributes.

Note that this has broken testing for 11.2.x+ pipelines for all contrib.

Related issue in "gitlab_templates": 📌 MissingGroupException: Missing group metadata with run-tests.sh in 11.2.0-dev Active

Modules are either getting MissingGroupException or ERROR: No valid tests were specified.

Key projects like Project Browser are affected 📌 Update for changes to System requirements Active , but again, this is all contrib testing NEXT_MINOR via run-tests.sh

Yes let's revert for now - hopefully we have enough to go on from the failed runs to figure out what's missing here.

In next iteration, I would consider doing:

1) Bump PHPUnit to 11 in the lock file, instead of keeping it locked to 10. 🌱 [meta] Support PHPUnit 11 in Drupal 10 Postponed has been in for 3 weeks now and AFAICS no issue have been reported about that. PHPUnit 11 also support PHP 8.3, which is minimum for D11. We can still keep 10 in the composer.json in case anyone has the need to downgrade for incompatibility (which, again, was not found yet).

2) Include 🐛 PhpUnitApiGetTestClassesTest and PhpUnitApiFindAllClassFilesTest need to execute PHPUnit discovery before TestDiscovery Active which was meant to be a followup of this, but since now it’s open again, better do straight ahead.

Then, if 📌 MissingGroupException: Missing group metadata with run-tests.sh in 11.2.0-dev Active willl still not be resolved, it should be (theoretically) only a matter of listing the relevant directories in phpunit.xml.dist - and that would require another core fix if contrib is meant to run with core configuration.

I’d also suggest to commit initially only to 11.x to allow proper checking in gitlab templates, and only eventually backport to 11.2.x upon complete validation.

@mondrake that all sounds good. I think defaulting to phpunit 11 is fine

One more strong reason is that PHPUnit 10 no longer has bugfix support (since Feb'25) https://phpunit.de/supported-versions.html

So moving to 11 is required step to start transition to 12

I do not understand why the CSpell job fail given that the file reported is included in the ignored paths in .cspell.json.

Then waiting for feedback to question in #3527063-8: Update for changes required by D11.2 → to see if we do need to edit the <directory> sections of phpunit.xml.dist to accomodate contrib testing.

Solved the CSpell problem.

Done #58.

Apparently the syntax to pick the test files in the /module subdirectory was failing somehow, changed after some thorough manual testing documented in https://drupal.slack.com/archives/CGKLP028K/p1748635336355999

This should hopefully fix the issue in 📌 Update for changes to System requirements Active with no changes to need happen there (once this is back in).

Currently you can enable test modules with a simple setting , it's extremely useful for debugging is that kept?

@ghost of drupal past sorry I do not understand your question. Can you please elaborate?

There's no change to module discovery here, only the test classes themselves.

OK, confirmed that the changes for the build in[#3527555] for Project browser are passing (phpstan issues there are unrelated), and the changes there are the same as the changes in the MR 10809 here, other than the use of globstar patterns core/.phpunit-next.xml here. But that file isn't used in the PB build per https://git.drupalcode.org/project/drupal/-/merge_requests/10809#note_51..., so everything LGTM.

Discussed this with @xjm - since this was committed for the beta, then reverted during the beta, and the actual change since is tiny, and it unblocks several other things, going ahead and committing it again relatively last minute during rc. Would be good peace of mind to see a successful contrib test run before we get anywhere near tagging 11.2.0 next week, but generally very glad to see this in!

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

Image Effects seems to work fine Test with 11.2.0-rc2 📌 Test with 11.2.0-rc2 Active

Project Browser seems to work too Test with 11.2.0-rc2 📌 Test with 11.2.0-rc2 Active

Updated release note snippet.

mondrake → closed merge request !10809

I know this was going to be committed to 11.x but was surprised to see it also committed to 11.2.x

As soon as Gitlab Templates makes the shift from 11.1 to 11.2 as 'current core version' this will break some existing contrib pipelines, see https://git.drupalcode.org/project/scheduler/-/jobs/5557961 This phpunit job uses the documented @group to divide the tests into parallel streams. I know we can also add the #[group ] metadata, but until contrib has a chance to do that then some pipelines will break. Maybe that's acceptable, but I just wanted to let you know, and to understand the process for Contrib changes.

The new code can manage both @group annotations and #[Group] attributes, otherwise 99% of core would be failing. So contrib is not requested to add the attribute. The linked tests in #72 and #73 prove it anyway.

So it must be something else, looking.

The error is

Drupal\Core\Test\Exception\MissingGroupException: Missing group metadata in test class Drupal\Tests\Composer\Generator\BuilderTest in /builds/project/scheduler/web/core/lib/Drupal/Core/Test/PhpUnitTestDiscovery.php:304

which is very very strange as that test (which is a core test btw) clearly DOES have a @group Metapackage annotation and it runs no problem in core.

Gut feeling is that the contrib is playing with the pwd of the job and PHPUnit/autoloading cannot find the class.

The actual command that runs the test in these jobs is
sudo -u www-data -H -E php web/core/scripts/run-tests.sh --php '/usr/local/bin/php' --color --keep-results --concurrency 32 --repeat '1' --sqlite 'sites/default/files/.sqlite' --dburl mysql://drupaltestbot:drupaltestbotpw@database/mysql --url http://localhost/web --xml /builds/project/scheduler/web/sites/default/files/simpletest --verbose --non-html scheduler_js
It is the final argument, 'scheduler_js' in the above example, which in 11.1 runs the tests annotated with @group scheduler_js but which fails at 11.2

The main reason to commit this to 11.2 is to allow contrib to start using attributes to support future updates to phpunit 11 and higher.

If we can make #79 work with a quick follow-up that would be great, but otherwise I think we'd need to revert this from 11.2 before we tag 11.2.0 (but probably leave it in 11.x) this time.

I am looking into this. Knowing the inners, it's very strange as that exception is thrown after the test list has been already built by PHPUnit, which is common with the other two tests.

Image Effects runs tests with pwd = /builds/project/image_effects
Project Browser runs tests with pwd = /builds/issue/project_browser-3530110

Opened 📌 Test with 11.2.0-rc2 Active in Scheduler's module issue queue to try and understand what's going on.

But with test changes, isn't the breakage broken regardless of whether it's in the release branch or not? Like 11.x will break things just as hard if it's broken, no? If what it's breaking are contrib pipelines?

📌 Test with 11.2.0-rc2 Active has a debug patch for core that now makes tests pass. Most important would now be to understand why PHPUnit sometimes does not pass a filled in Test object; then we can make a core patch.

Do I need to open a separate issue or an additional MR in this issue, eventually?

Filed 📌 Introduce PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION Active in GitLab Templates issue queue.

We might have surfaced a bug upstream (PHPUnit's test suites builder seems to be adding empty TestSuite sub-objects to the main TestSuite object under some unclear circumstances), but this minimal MR!12382 fix seems to let the overall testing discovery complete in that case. 📌 Test with 11.2.0-rc2 Active is now passing with the equivalent of the MR as a patch.

Will try to see if I can report upstream, but that will require quite some investigation before being able to - building a self-contained minimal patch reproducing the issue is not going to be a simple one.

Copy/pasting here more details I added in #3530183-8: Test with 11.2.0-rc2 → .

I could replicate the issue in Image Effects https://git.drupalcode.org/project/image_effects/-/merge_requests/72 by choosing to select tests by group instead of by directory.

To me the issue is a bug upstream: PHPUnit 'builds' the list of tests to execute as a hierarchical structure of TestSuite and TestCase objects, and seems to be adding empty TestSuite sub-objects to the main TestSuite object under some unclear circumstances.

Core patch developed here seems to work, and it is now a follow-up MR in core's #3497431-88: Deprecate TestDiscovery test file scanning, use PHPUnit API instead → .

It just skips, though, empty TestSuite objects which may lead to skipped test runs if the @group/#[Group] of the test class skipped corresponds to the group filter when running runt-tests.sh. I will keep investigating in Image Effects issue 📌 Test with 11.2.0-rc2 Active .

I think core's 📌 Allow indicating alternative phpunit.xml than core's when testing via run-tests.sh Active would help as it could open up the possibility to declare the directories to be searched for test discovery by the module, if run-tests.sh execution is filtered by test group. This will prevent discovering all of the core test files as it is happening now.

So as detailed in #3530183-13: Test Scheduler at 11.2.x-dev with patched PhpUnitTestDiscovery.php → , issue reported in #76 are due to gaps present elsewhere, that the PHPUnit discovery process legitimately cannot deal with.

In the MR, now, an implementation of PHPUnit's Tracer is collecting warnings thrown by PHPUnit during discovery, and reporting them in the run-tests.sh output so they can be actioned. The discovery process is now completed insetad of failing as it was identified in #76, but the actual list of tests to be executed will be missing those tests that errored during discovery.

The MR looks tidy to me, and the new error output is good. I think we should go ahead and commit this as a follow-up to both 11.x and 11.2.x.

I was on the verge of self-RTBC already, #91 confirms, so let's get going.

unless we just wanted to leave it around a little bit for aggressively enforced debugging or something. :)

Yes this is more or less the reason. It turned out the last round of failures was surfacing specific issues discovering tests in specific modules due to things like non-static data providers and missing directories etc.

I think we probably need a new CR here that modules that previously had silent test discovery errors will now get a loud error with useful information, however I don't feel like I understand the intricacies of the specific errors found to write a useful summary.

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

Moving to needs work for the new/updated change record.

catch → closed merge request !12382

I had just written the below, simultaneously, but copy/pasting here anyway.

the issue reported in #76 is due to gaps present elsewhere, that the PHPUnit discovery process legitimately cannot deal with.

It seems that PHPUnit 11 is obviously a lot more picky about test discovery than PHPUnit 10, which easily found all the @group mygroup and did not complain at all.

But yes I agree, get this in 11.2.x then those tests at Next Minor will start running again. If the output clearly lists the skipped tests and the reasons then at least we have something to work on. But if they are skipped due to 3rd-party module problems then that will me we lose test coverage until they can be fixed. In the earlier 11.2.x work, those were merely reported as deprecations in 3rd-party modules wich could be ignored and the tests ran. Anyway, we have to move forward, so yes RTBC+1. Good work mondrake, thank you for taking this on.

I'll work on a CR. Re #97:

But if they are skipped due to 3rd-party module problems then that will me we lose test coverage until they can be fixed.

to me this is not going to mean lost test coverage. We need to distinguish test discovery from test execution. Test discovery looks for ALL the test classes available (based on some pre-constraints sometimes, e.g. if you specifically indicate a directory to search in, it will be limited to that), then filters only those that need to be executed.

In the case of the wrong dataprovider in address and token modules of the related issue, these are reported as failing when building the test list during discovery, but are not executed because the filter is @group scheduler_xxx. So, the scheduler tests are not really affected. They would fail loud on tests executed on those modules with PHPUnit 11 (time to apply the static modifier...), or if, from the scheduler module, you'd run tests for @group token or @group address. But that's because the dataprovider must be static, not for the discovery.

Ah thank you for explaining that. I see now that your comment in #90 "but the actual list of tests to be executed will be missing those tests that errored during discovery." is actually the "list of tests discovered will be missing ..." but the full list for the intended module should still be run (providing the errors are only in 3rd-party modules). That sounds perfect.

Added CR https://www.drupal.org/node/3530388 → , appreciate review and publishing.

This issue added some new deprecations to the baseline. I've opened 📌 Remove deprecated use of Assert::isType Active to fix them.

@jonathan1055 kindly reviewed the new CR, I published it. Tentatively marking this fixed as it looks to me there's nothing else to do.

Thanks all for the hotfix and related followup work!

Retroactively bumping priority.

Now that this has been re-committed to 11.2.x I presume the idea is to fix all the Core tests so that they do not produce PHPUnit test runner deprecations? Contrib tests get deprecation warnings for Core classes and methods that don't have the new attributes yet. For example a javascript test that extends use Drupal\FunctionalJavascriptTests\WebDriverTestBase gets warings that the failOnJavaScriptErrors() method contains doc-comment metadata.
See https://git.drupalcode.org/project/scheduler/-/jobs/5596390#L670

Here is the failOnJavaScriptErrors() method in webDriverTestBase

I've not found any open core issues to start takling this task yet, or maybe I missed it. I can open an issue if this is the plan.

📌 Remove support for PHPUnit 10 Postponed or 📌 Allow indicating alternative phpunit.xml than core's when testing via run-tests.sh Active are probably the first steps towards that - e.g. we would stop running core tests against phpunit 10 at all. Until we do that I don't think we can remove deprecated usages in the test themselves because they'd be needed for phpunit 10. I don't know if there's a specific issue for that bit though or not.

Thanks for the info. But you don't have to remove the deprecated @ metadata just yet, you only need to add the equivalent #[ ] version to prevent the deprecation messages. Both can exist, and the tests run at both PHPUnit11 and PHPUnit10 without a problem. At least, they do with Contrib @group and @dataprovider, because I've done that here.

There are a few issues already towards converting metadata from annotations to attributes, see related 📌 [meta] Define a Rector rule to convert test annotations to attributes Active , 📌 Convert test annotation to attributes in Drupal/Test/Component Active , 📌 Convert test annotations to attributes in Drupal/BuildTests Active . But they are currently children of the original 🌱 [meta] Support PHPUnit 11 in Drupal 10 Postponed . It would be probably be the case now to open a new meta for the conversions (there are 000s of files to convert), and reparent those to it.

When it comes to the deprecations, with PHPUnit 11 we need to distinguish between SUT deprecations and PHPUnit runner deprecations - the former are the 'traditional' ones, the latter are internal to PHPUnit. Core, when running tests through run-tests.sh, currently DISABLES PHPUnit runner deprecations by setting the env variable PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION to FALSE. This leads to SUT deprecations being reported but no PHPUnit ones. 📌 Introduce PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION Active aims at introducing the same capability in GitLab Templates; contrib using run-tests.sh can anyway already use it if needed.

Re #105

Until we do that I don't think we can remove deprecated usages in the test themselves because they'd be needed for phpunit 10.

per se PHPUnit 10 can perfectly process attributes only, so if tests are run via PHPUnit CLI, no problem to have attributes only in contrib. See https://www.drupal.org/project/sophron → for example. The problem is if tests are run via the run-tests.sh script BEFORE Drupal 11.2: in that case the 'old' TestDiscovery process is in place, and that one is not capable of dealing with test classes that lack the @group annotation.

Ah OK that makes things a lot easier then, thanks for the clarification.

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