Deprecate TestDiscovery test file scanning, use PHPUnit API instead

Created on 5 January 2025, 5 months ago

Problem/Motivation

Drupal is using its own code to discover tests to be executed, which along time lead to discrepancies in the tests executed betwen PHPUnit CLI and run-tests.sh.

PHPUnit 10 introduced an API to discover the test suites, that can be used instead.

Proposed resolution

  • Deprecate TestDiscovery::getTestClasses() and related methods.
  • Introduce a like-for-like replacement that uses PHPUnit's API for the discovery, and enriches the information returned by the API to provide same info as TestDiscovery::getTestInfor().
  • Make the new code ready for PHpUnit's 10+ attributes instead of annotations.
  • In the process, fix some bugs of TestDiscovery::getTestClasses() that were identified:
    • for each test class, it only reports groups present in the class-level annotations: this means that method level annotations do not qualify test classes for a group, potentially missing the execution of some tests. PHPUnit is more accurate in bubbling up method-level annotations/attributes to the class
    • it does not check duplication of @group annotations that specify the same group. PHPUnit make them unique.
    • when filtering by test suite, it reports some test groups with an empty array, since in any case it iterates all test directories and files. PHPUnit only scans the directories indicated in phpunit.xml for each test suite, providing more accurate results.

Remaining tasks

User interface changes

nope

API changes

nope

Data model changes

nope

Release notes snippet

Drupal tests now can use PHPUnit's 10+ attributes instead of the legacy annotations. New tests MUST use attributes.

📌 Task
Status

Active

Version

11.0 🔥

Component

phpunit

Created by

🇮🇹Italy mondrake 🇮🇹

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @mondrake
  • Merge request !10809Closes #3497431 → (Open) created by mondrake
  • Pipeline finished with Failed
    5 months ago
    Total: 136s
    #386235
  • Pipeline finished with Failed
    5 months ago
    Total: 175s
    #386257
  • Pipeline finished with Failed
    5 months ago
    #386276
  • Pipeline finished with Failed
    5 months ago
    #386277
  • Pipeline finished with Failed
    5 months ago
    Total: 88s
    #386279
  • Pipeline finished with Failed
    5 months ago
    Total: 697s
    #386315
  • Pipeline finished with Failed
    5 months ago
    Total: 420s
    #386438
  • Pipeline finished with Failed
    5 months ago
    Total: 131s
    #386605
  • Pipeline finished with Failed
    5 months ago
    Total: 429s
    #386613
  • Pipeline finished with Failed
    5 months ago
    Total: 345s
    #386630
  • Pipeline finished with Failed
    5 months ago
    Total: 702s
    #386639
  • Pipeline finished with Failed
    5 months ago
    Total: 129s
    #386645
  • Pipeline finished with Failed
    5 months ago
    Total: 89s
    #386651
  • Pipeline finished with Failed
    5 months ago
    Total: 990s
    #386652
  • Pipeline finished with Failed
    5 months ago
    Total: 1222s
    #387009
  • Pipeline finished with Success
    5 months ago
    Total: 709s
    #387172
  • Pipeline finished with Failed
    5 months ago
    Total: 503s
    #387395
  • Pipeline finished with Failed
    5 months ago
    Total: 129s
    #387696
  • Pipeline finished with Success
    5 months ago
    Total: 495s
    #387707
  • Pipeline finished with Success
    5 months ago
    Total: 505s
    #387737
  • Pipeline finished with Canceled
    5 months ago
    Total: 117s
    #387746
  • Pipeline finished with Failed
    5 months ago
    Total: 111s
    #387750
  • Pipeline finished with Failed
    5 months ago
    Total: 381s
    #388227
  • Pipeline finished with Failed
    5 months ago
    Total: 112s
    #388331
  • Pipeline finished with Success
    5 months ago
    Total: 382s
    #388341
  • 🇮🇹Italy mondrake 🇮🇹
  • 🇮🇹Italy mondrake 🇮🇹
  • Pipeline finished with Failed
    5 months ago
    Total: 141s
    #390881
  • Pipeline finished with Failed
    5 months ago
    Total: 317s
    #390921
  • Pipeline finished with Failed
    5 months ago
    Total: 331s
    #390964
  • 🇮🇹Italy mondrake 🇮🇹

    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.

  • 🇮🇹Italy mondrake 🇮🇹
  • Pipeline finished with Success
    5 months ago
    Total: 1231s
    #392885
  • 🇮🇹Italy mondrake 🇮🇹

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

  • Pipeline finished with Canceled
    5 months ago
    Total: 411s
    #397047
  • Pipeline finished with Failed
    5 months ago
    Total: 5512s
    #397051
  • Pipeline finished with Failed
    5 months ago
    Total: 626s
    #397148
  • 🇮🇹Italy mondrake 🇮🇹

    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.

  • Pipeline finished with Failed
    5 months ago
    Total: 654s
    #397523
  • 🇮🇹Italy mondrake 🇮🇹
  • Pipeline finished with Failed
    5 months ago
    Total: 1661s
    #397531
  • Pipeline finished with Failed
    5 months ago
    Total: 492s
    #397680
  • Pipeline finished with Failed
    5 months ago
    Total: 3578s
    #397725
  • Pipeline finished with Failed
    5 months ago
    Total: 560s
    #397804
  • Pipeline finished with Failed
    5 months ago
    Total: 125s
    #397813
  • Pipeline finished with Canceled
    5 months ago
    Total: 249s
    #398024
  • Pipeline finished with Success
    5 months ago
    Total: 700s
    #398029
  • Pipeline finished with Failed
    4 months ago
    Total: 208s
    #400143
  • Pipeline finished with Failed
    4 months ago
    Total: 179s
    #400161
  • Pipeline finished with Failed
    4 months ago
    Total: 497s
    #400164
  • Pipeline finished with Failed
    4 months ago
    Total: 1015s
    #400252
  • First commit to issue fork.
  • Pipeline finished with Success
    4 months ago
    Total: 840s
    #411054
  • 🇬🇧United Kingdom catch

    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.

  • 🇮🇹Italy mondrake 🇮🇹

    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.

  • 🇬🇧United Kingdom catch

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

  • 🇮🇹Italy mondrake 🇮🇹

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

  • Pipeline finished with Failed
    4 months ago
    Total: 428s
    #413389
  • Pipeline finished with Failed
    4 months ago
    Total: 179s
    #413442
  • Pipeline finished with Success
    4 months ago
    Total: 357s
    #413443
  • 🇮🇹Italy mondrake 🇮🇹

    Done #14.

  • 🇮🇹Italy mondrake 🇮🇹

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

  • Pipeline finished with Canceled
    4 months ago
    Total: 106s
    #422211
  • Pipeline finished with Failed
    4 months ago
    Total: 553s
    #422212
  • 🇮🇹Italy mondrake 🇮🇹

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

  • Pipeline finished with Success
    4 months ago
    Total: 4893s
    #424571
  • Pipeline finished with Failed
    3 months ago
    Total: 731s
    #434842
  • Pipeline finished with Failed
    3 months ago
    Total: 329s
    #435548
  • Pipeline finished with Success
    3 months ago
    Total: 439s
    #436786
  • Pipeline finished with Success
    3 months ago
    Total: 421s
    #439168
  • Pipeline finished with Success
    3 months ago
    Total: 1557s
    #439579
  • Pipeline finished with Failed
    3 months ago
    Total: 599s
    #441372
  • Pipeline finished with Failed
    3 months ago
    Total: 518s
    #447250
  • Pipeline finished with Success
    2 months ago
    Total: 624s
    #456038
  • Pipeline finished with Success
    2 months ago
    Total: 780s
    #457802
  • Pipeline finished with Failed
    about 2 months ago
    Total: 660s
    #466780
  • Status changed to Needs review about 2 months ago
  • Pipeline finished with Success
    about 2 months ago
    Total: 2642s
    #473283
  • 🇮🇹Italy mondrake 🇮🇹

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

  • Pipeline finished with Success
    about 2 months ago
    Total: 533s
    #473617
  • Pipeline finished with Success
    about 2 months ago
    Total: 731s
    #473627
  • Pipeline finished with Success
    about 2 months ago
    Total: 1934s
    #473734
  • Pipeline finished with Failed
    about 2 months ago
    Total: 219s
    #473786
  • Pipeline finished with Success
    about 2 months ago
    Total: 1135s
    #473931
  • Pipeline finished with Failed
    about 2 months ago
    Total: 120s
    #474982
  • Pipeline finished with Failed
    about 2 months ago
    Total: 120s
    #474985
  • Pipeline finished with Failed
    about 2 months ago
    Total: 532s
    #475047
  • Pipeline finished with Canceled
    about 1 month ago
    #482571
  • Pipeline finished with Failed
    about 1 month ago
    Total: 482s
    #482574
  • Pipeline finished with Failed
    about 1 month ago
    Total: 598s
    #483002
  • Pipeline finished with Failed
    about 1 month ago
    Total: 1970s
    #484977
  • Pipeline finished with Failed
    about 1 month ago
    Total: 252s
    #485040
  • Pipeline finished with Failed
    about 1 month ago
    Total: 557s
    #485056
  • Pipeline finished with Success
    about 1 month ago
    Total: 626s
    #485148
  • Pipeline finished with Failed
    about 1 month ago
    Total: 292s
    #486752
  • Pipeline finished with Success
    about 1 month ago
    Total: 764s
    #486758
  • Pipeline finished with Success
    27 days ago
    Total: 498s
    #488417
  • 🇮🇹Italy mondrake 🇮🇹

    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.

  • Pipeline finished with Failed
    24 days ago
    #491130
  • 🇮🇹Italy mondrake 🇮🇹
  • Pipeline finished with Success
    24 days ago
    #491137
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Blocker is in

  • 🇮🇹Italy mondrake 🇮🇹

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

  • Pipeline finished with Success
    24 days ago
    Total: 2585s
    #491994
  • 🇬🇧United Kingdom catch

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

  • 🇮🇹Italy mondrake 🇮🇹

    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 .

  • Pipeline finished with Failed
    23 days ago
    Total: 608s
    #492989
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

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

  • Pipeline finished with Failed
    18 days ago
    Total: 684s
    #496345
  • Pipeline finished with Success
    18 days ago
    Total: 568s
    #496726
  • 🇮🇹Italy mondrake 🇮🇹

    removed duplicate @group #slow annotations

  • Pipeline finished with Failed
    15 days ago
    Total: 599s
    #499594
  • 🇮🇹Italy mondrake 🇮🇹

    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.

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    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.

  • Pipeline finished with Success
    8 days ago
    Total: 494s
    #504824
  • 🇮🇹Italy mondrake 🇮🇹

    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.

    • catch committed 4193887a on 11.2.x
      Issue #3497431 by mondrake, catch, larowlan, godotislate: Deprecate...
    • catch committed f955ab18 on 11.x
      Issue #3497431 by mondrake, catch, larowlan, godotislate: Deprecate...
  • 🇬🇧United Kingdom catch

    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!

  • 🇬🇧United Kingdom catch

    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 committed b236cc00 on 11.2.x
      Revert "Issue #3497431 by mondrake, catch, larowlan, godotislate:...
    • catch committed 75bf2e53 on 11.x
      Revert "Issue #3497431 by mondrake, catch, larowlan, godotislate:...
  • 🇮🇹Italy mondrake 🇮🇹

    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.

  • Pipeline finished with Success
    8 days ago
    Total: 364s
    #505165
    • catch committed 749a119e on 11.2.x
      Issue #3497431 by mondrake, catch, larowlan, godotislate: Deprecate...
    • catch committed 64212ca3 on 11.x
      Issue #3497431 by mondrake, catch, larowlan, godotislate: Deprecate...
  • 🇬🇧United Kingdom catch

    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.

  • 🇬🇧United Kingdom catch

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

  • 🇮🇹Italy mondrake 🇮🇹

    Do we need publishing the CR?

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Change record appears to already be published 👍

  • 🇮🇹Italy mondrake 🇮🇹

    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.

  • 🇮🇹Italy mondrake 🇮🇹
  • 🇮🇹Italy mondrake 🇮🇹
  • 🇪🇸Spain fjgarlin

    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

    • catch committed aa346e2e on 11.2.x
      Revert "Issue #3497431 by mondrake, catch, larowlan, godotislate:...
    • catch committed b281a7ae on 11.x
      Revert "Issue #3497431 by mondrake, catch, larowlan, godotislate:...
  • 🇬🇧United Kingdom catch

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

  • 🇮🇹Italy mondrake 🇮🇹

    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.

  • 🇬🇧United Kingdom catch

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

  • Pipeline finished with Canceled
    1 day ago
    Total: 130s
    #510197
  • Pipeline finished with Failed
    1 day ago
    Total: 146s
    #510198
  • 🇫🇷France andypost

    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

  • Pipeline finished with Failed
    1 day ago
    Total: 214s
    #510250
  • Pipeline finished with Failed
    1 day ago
    Total: 153s
    #510254
  • Pipeline finished with Failed
    1 day ago
    Total: 270s
    #510260
  • 🇮🇹Italy mondrake 🇮🇹

    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.

  • Pipeline finished with Success
    1 day ago
    Total: 454s
    #510369
  • 🇮🇹Italy mondrake 🇮🇹

    Solved the CSpell problem.

  • 🇮🇹Italy mondrake 🇮🇹

    Done #58.

Production build 0.71.5 2024