[meta] Support PHPUnit 11 in Drupal 10

Created on 31 January 2024, about 1 year ago

Problem/Motivation

PHPUnit 10 is released on Febryary 3, 2024. Drupal 10 will need to make changes to support it. While PHPUnit 10 is being worked on in 🌱 [meta] Support PHPUnit 10 in Drupal 11 Active , the bugfix support on that will end February 7, 2025, while Drupal 10 will be supported much longer. Also Drupal 11 would be launched with PHPUnit 11 support to ensure no required PHPUnit major upgrade to soon.

Proposed resolution

Postponed until 🌱 [meta] Support PHPUnit 10 in Drupal 11 Active is done.

Remaining tasks

TBD

Follow ups

TBD

🌱 Plan
Status

Postponed

Version

11.0 🔥

Component
PHPUnit 

Last updated about 10 hours ago

Created by

🇭🇺Hungary Gábor Hojtsy Hungary

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

Merge Requests

Comments & Activities

  • Issue created by @Gábor Hojtsy
  • 🇮🇹Italy mondrake 🇮🇹

    PHPUnit 11 dropped support for PHP 8.1, so with PHPUnit 10 already a major challenge for Drupal 10, not sure it makes sense to target also PHPUnit 11 for it.

  • 🇬🇧United Kingdom catch

    By the time PHPUnit 10 is out of support, we'll be in a position to stop running tests on PHP 8.1, so theoretically we can update as it's only a dev dependency. Having said that, whether we eventually run Drupal 10 tests against PHPUnit 11 is very secondary to whether we can with Drupal 11, so dealing with the deprecations (after we've dealt with the PHPUnit 10 ones) is the main thing anyway.

  • Merge request !6859PHPUnit 11 #3418267 → (Open) created by mondrake
  • Pipeline finished with Failed
    about 1 year ago
    Total: 214s
    #108546
  • Pipeline finished with Failed
    about 1 year ago
    Total: 172s
    #108549
  • Pipeline finished with Failed
    about 1 year ago
    #108552
  • Pipeline finished with Failed
    about 1 year ago
    #108556
  • Status changed to Active 11 months ago
  • Pipeline finished with Failed
    11 months ago
    Total: 187s
    #169029
  • Pipeline finished with Canceled
    11 months ago
    Total: 422s
    #169041
  • Pipeline finished with Failed
    11 months ago
    Total: 574s
    #169053
  • 🇮🇹Italy mondrake 🇮🇹

    Given test results, the only big thing to get is onto PHPUnit 11 is only the replacement of test annotations with PHP attributes, that throw thousands of deprecations. Actual errors and failures are just a few (functional ones seem actually error-free)

  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    Hmmm… but how to have a test that uses annotations compatible with D10 (on PHPUnit 9) and D11 (on PHPUnit 11) - just not care about the deprecations? I guess if you are going for D10 and D11 compatibility then you should be ignoring deprecations. So we just need to document that.

  • 🇬🇧United Kingdom longwave UK

    In #3445106-7: Replace @dataProvider annotations with #[DataProvider()] attributes @mondrake linked to an example where the file has both annotations (for PHPUnit 9) and attributes (for PHPUnit 10/11) at the same time: https://github.com/FileEye/MimeMap/blob/ba0b04e179976e7d6a487fdb166c5f1f...

  • 🇮🇹Italy mondrake 🇮🇹

    AFAICS we cannot ignore deprecations thrown by PHPUnit itself, anymore. PHPUnit is not using trigger_error for deprecations, but it's own event system. But we could apparently have both annotations and attributes and being compatible with PHPUnit all the way from 9 to 11 - see for instance https://github.com/FileEye/MimeMap/blob/master/tests/src/TypeTest.php#L7... in a separate project. AFAICS PHPUnit will throw a deprecation when no attributes exists AND annotation exist; if the test has attributes, PHPUnit 11 will rely on those and not check about existing annotations.

  • 🇦🇺Australia mstrelan

    But we could apparently have both annotations and attributes and being compatible with PHPUnit all the way from 9 to 11

    The attribute class would be missing in PHPUnit 9 though so that wouldn't work, or we'd have to alias them to some placeholder class.

  • 🇮🇹Italy mondrake 🇮🇹

    Note sure about #11. It doesn't seem to be an issue in the project I referenced. Maybe if the class is missing yes, me might have a problem with PHPStan reporting it, but runtime the attribute is just handled like a comment?

  • 🇦🇺Australia mstrelan

    Right, I thought the use statement to a non existent class would cause a problem but it seems it doesn't.

  • 🇮🇹Italy mondrake 🇮🇹

    I agree with #9 btw, and think in core we should just remove the annotations, and document the possible double standard for contrib.

  • 🇬🇧United Kingdom catch

    Using attributes in core but recommending both attributes and annotations for contrib seems good. We would need to work out the mechanics of it, but it would be good to run core tests on phpunit 11 by default for Drupal core (composer update maybe once we're compatible?) but lock to phpunit 10 for contrib (maybe with the option to upgrade too, or something like that). Thanks for doing the investigation here, really good to know it's not going to be as bad this time.

  • Pipeline finished with Failed
    9 months ago
    Total: 130s
    #236506
  • Pipeline finished with Failed
    9 months ago
    Total: 543s
    #236514
  • 🇺🇸United States xjm
  • Pipeline finished with Failed
    6 months ago
    #308668
  • Pipeline finished with Failed
    6 months ago
    Total: 254s
    #308672
  • Pipeline finished with Failed
    6 months ago
    Total: 708s
    #308676
  • Status changed to Needs work 3 months ago
  • 🇬🇧United Kingdom longwave UK

    Rebased against 11.x and updated to the latest PHPUnit 11. If we can ignore the deprecations for now we are not too far off landing this.

    PHPUnit expects parameter names to match in data providers, this was not strictly the case before and we had two mismatched, fixed here.

    The remaining issues are mostly

    Failed to open directory: Too many open files
    

    which I think might be caused by triggering way too many deprecations, and a handful of strange mocking errors:

    Error: Typed property MockObject_EntityBaseTest_2da9ae70::$__phpunit_state must not be accessed before initialization
    
  • 🇮🇹Italy mondrake 🇮🇹

    Since PHPUnit 11.3.3, PHPUnit's own deprecations are only reported if the --fail-on-phpunit-deprecation toggle is passed in.

    See https://github.com/sebastianbergmann/phpunit/blob/11.3.6/ChangeLog-11.3....

    PHPUnit deprecations do not use trigger_error for being reported (see #10). So if we move on like this it's OK but we need to be aware and conscious that all our tests will silently execute with deprecated PHPUnit code, and that code will be dropped in PHPUnit 12 that is one month away.

    The bigger part of PHPUnit 11 change - moving from using annotations to using attributes - still needs to be started in fact. 📌 TestDiscovery expects @group annotations, fails with #[Group()] attributes Active is the first step in that sense, waiting for a review, and 📌 [meta] Define a Rector rule to convert test annotations to attributes Active + 📌 Convert test annotation to attributes in Drupal/Test/Component Needs work would be the beginning of the following steps.

  • 🇮🇹Italy mondrake 🇮🇹

    I have added the --fail-on-phpunit-deprecation toggle to make the MR comparable to earlier results; we can remove that later if we decide to ignore PHPUnit's own deprecations for now. Or, alternatively, find a way to make PHPUnit's own deprecations reporting configurable in our CI.

  • 🇮🇹Italy mondrake 🇮🇹

    The IS seems quite off, now.

  • Pipeline finished with Failed
    3 months ago
    Total: 807s
    #383885
  • 🇬🇧United Kingdom longwave UK

    I think PHPUnit deprecations are OK to deal with in a followup, especially as there is a switch for them now that means we can ignore them for the time being. The sooner that we get onto PHPUnit 11 at all the sooner contrib can start to trying to catch up, and we can figure out the plan for migrating to attributes in the meantime.

  • 🇬🇧United Kingdom longwave UK

    Not sure what's up with the "too many open files", debugging it is tricky.

    Locally, DerivativeDiscoveryDecoratorTest::testInvalidDerivativeFetcher() does not fail but takes 18 seconds until I comment out the trigger_error in DebugClassLoader::checkClass():

                foreach ($deprecations as $message) {
                    @trigger_error($message, \E_USER_DEPRECATED);
                }
    

    After removing this it takes 16 milliseconds - over 1000 times quicker.

  • 🇬🇧United Kingdom longwave UK

    The issue is in PHPUnit's own error handler. For a given deprecation it tries to figure out whether it was caused in and/or by first party code as per the source map.

                if ($this->sourceFilter->includes($this->source, $trace[0]['file'])) {
                    $triggeredInFirstPartyCode = true;
                }
    

    SourceFilter::includes() creates a new SourceMap object which scans the entire filesystem as per the <source> tag in phpunit.xml.dist.

    However it looks like this code has largely existed since PHPUnit 10.1 so not sure why it is problematic now.

  • 🇬🇧United Kingdom longwave UK

    Seems it's related to this feature in PHPUnit 11.1: https://github.com/sebastianbergmann/phpunit/issues/5689

  • 🇬🇧United Kingdom longwave UK

    For some reason it's suffix checking that is the problem. If I remove

          <directory suffix=".api.php">./lib/**</directory>
          <directory suffix=".api.php">./modules/**</directory>
          <directory suffix=".api.php">../modules/**</directory>
    

    from phpunit.xml.dist then DerivativeDiscoveryDecoratorTest::testInvalidDerivativeFetcher() takes 2 seconds instead of 18.

    I'm not even sure why we ignore these, they should never be included for the purposes of code coverage anyway.

  • 🇮🇹Italy mondrake 🇮🇹

    How about adding a part to the SYMFONY_DEPRECATIONS_HELPER environment variable to indicate if PHPUnit's own deprecation should be reported or not? That variable name is legacy, but in fact it's only used by our own replacement of the PHPUnit-bridge, so IMHO we can change its syntax at our wish. Say something like

    SYMFONY_DEPRECATIONS_HELPER='ignoreFile=.deprecation-ignore.txt&failOnPhpunitDeprecation=false'

    with a default fallback value for failOnPhpunitDeprecation=TRUE if not indicated.

  • 🇬🇧United Kingdom longwave UK

    Or we could just add a separate environment variable?

  • 🇬🇧United Kingdom longwave UK

    Figured out three of the four fails. It feels like KeyValueEntityStorageTest is bending the rules a bit as - from what I can tell - we expect the entity storage to be able to create an instance of a mock class and it will just work, but I am not sure that is expected or should be allowed.

  • 🇮🇹Italy mondrake 🇮🇹

    #27

    could just add a separate environment variable?

    OK, on it - I would like to keep PHPUnit deprecation configuration things centralised in DeprecationHandler::getConfiguration(), though.

    Also, can 📌 Drop using sudo to execute run-tests.sh Active be committed? It'd make life easier.

  • 🇬🇧United Kingdom longwave UK

    Re the remaining test, I think this is OK, we create a new mock and explicitly force the new flag, instead of trying to reuse the previously-created entity to test the insert hooks.

  • Pipeline finished with Failed
    3 months ago
    Total: 719s
    #384046
  • 🇮🇹Italy mondrake 🇮🇹
  • Pipeline finished with Failed
    3 months ago
    Total: 560s
    #384061
  • Pipeline finished with Canceled
    3 months ago
    Total: 650s
    #384065
  • Pipeline finished with Failed
    3 months ago
    Total: 511s
    #384074
  • Pipeline finished with Failed
    3 months ago
    Total: 626s
    #384104
  • Pipeline finished with Failed
    3 months ago
    Total: 385s
    #384128
  • 🇮🇹Italy mondrake 🇮🇹

    We have this deprecation error, that we can ignore for now, adding an entry in .deprecation-ignore.txt:

    The "PHPUnit\Framework\TestCase::__construct()" method is considered final. It may change without further notice as of its next major version. You should not extend it from "Drupal\Tests\BrowserTestBase".

    However this is painful as this means that in the future we will not be able to override the TestCase constructor to force Kernel and Functional* tests to run in isolation even if the #[RunTestsInSeparateProcesses] attribute is not specified.

    Tests are green now.

  • Pipeline finished with Success
    3 months ago
    Total: 1692s
    #384201
  • 🇮🇹Italy mondrake 🇮🇹

    Filed 📌 Assert::isType() is deprecated Active to try and reduce the PHPStan baseline increase.

  • 🇬🇧United Kingdom longwave UK

    Re #32 I guess we have a little while to figure out a solution to overriding the constructor. I seem to remember that PHPUnit thought that our solution was okay when we asked for a way to enable isolation in a base class, maybe we have to follow that up again.

  • 🇮🇹Italy mondrake 🇮🇹

    #34 unfortunately not. I tried https://github.com/sebastianbergmann/phpunit/pull/5981 but that was rejected and AFAICS there is no guidance in https://github.com/sebastianbergmann/phpunit/issues/5838.

    What I was thinking instead is to add a #[RunTestsInSeparateProcesses] attribute to all Kernel and Functional* tests with the rector conversion of the tests, and supplement that wit a PHPStan custom rule that will fail if new tests are added without the attribute.

  • Pipeline finished with Success
    3 months ago
    Total: 1251s
    #384649
  • 🇮🇹Italy mondrake 🇮🇹

    Filed 🐛 Fix static call to instance methods in PHPUnit Active as a child issue.

  • 🇮🇹Italy mondrake 🇮🇹

    No longer a plan, this has a MR that can be merged.

    Updated IS.

  • Pipeline finished with Failed
    3 months ago
    Total: 471s
    #390249
  • Pipeline finished with Failed
    3 months ago
    Total: 588s
    #390269
  • 🇮🇹Italy mondrake 🇮🇹

    The test failure looks legit, and it'd unveil a PHPUnit 11 bug. Let's see if a workaround works, then I will file a bug upstream.

  • Pipeline finished with Failed
    3 months ago
    Total: 442s
    #390604
  • Pipeline finished with Failed
    3 months ago
    Total: 433s
    #390620
  • Pipeline finished with Failed
    3 months ago
    Total: 549s
    #390655
  • 🇮🇹Italy mondrake 🇮🇹

    Strange bug, on it working locally.

  • 🇮🇹Italy mondrake 🇮🇹

    OK, so it seems we have a type bug upstream when a @group annotation specifies a numeric string (first test failure), plus in PHPUnit 11 The format of the XML document generated using the `--list-tests-xml` CLI option has been changed.

  • 🇮🇹Italy mondrake 🇮🇹

    For the bug upstream, filed --list-tests-xml is broken if a numeric @group annotation is specified in PHPUnit's issue queue.

  • 🇮🇹Italy mondrake 🇮🇹

    Fixed.

  • Pipeline finished with Success
    3 months ago
    Total: 924s
    #390839
  • 🇮🇹Italy mondrake 🇮🇹

    PHPUnit 11.5.3, due shortly, fixes the upstream bug so IMHO it's worth waiting for it.

  • Pipeline finished with Failed
    3 months ago
    Total: 548s
    #392325
  • 🇮🇹Italy mondrake 🇮🇹

    Merged 📌 Remove redundant entries in phpunit.xml.dist Postponed in here.

  • Pipeline finished with Success
    3 months ago
    Total: 1096s
    #392938
  • 🇮🇹Italy mondrake 🇮🇹

    Bumped PHPUnit to 11.5.3, reverted change to @group 44 annotation, ready for review again.

  • Pipeline finished with Success
    3 months ago
    Total: 793s
    #394692
  • Pipeline finished with Failed
    3 months ago
    Total: 578s
    #403160
  • Pipeline finished with Failed
    2 months ago
    Total: 156s
    #421227
  • 🇮🇹Italy mondrake 🇮🇹

    Bumped to PHPUnit 11.5.7 and adjusted PHPStan baseline after commit of 🐛 Fix static call to instance methods in PHPUnit Active

  • Pipeline finished with Success
    2 months ago
    Total: 1326s
    #421231
  • 🇮🇹Italy mondrake 🇮🇹

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

  • Pipeline finished with Success
    about 2 months ago
    Total: 2814s
    #424589
  • Pipeline finished with Success
    about 1 month ago
    Total: 1621s
    #439159
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 170s
    #443062
  • Pipeline finished with Success
    about 1 month ago
    Total: 791s
    #443065
  • 🇦🇺Australia mstrelan

    I read through this issue and reviewed the MR. I pulled down the MR and tested locally and it worked fine. I tried to apply it to a client project but ran in to too many problems with composer dependencies. I ran a few contrib tests locally and they worked fine too.

    The issue summary needs an update with a release note and next steps for follow ups. We need a change record too, and possibly instructions for downgrading to PHPUnit 10. I think we still need to figure out #15, what happens in gitlab for contrib? Would it automatically get PHPUnit 11 or would that require a change in gitlab_templates?

  • 🇮🇹Italy mondrake 🇮🇹

    Maybe for now it would be better to do the other way round, i.e. keep PHPUnit 10 in the lock file, with relaxed constraints in composer.json, and allow opt-in to PHPUnit 11? So core can bump to PHPUnit 11 in its test pipelines, while we do not force everyone to downgrade from 11 to 10.

  • 🇬🇧United Kingdom catch

    #50 sounds like a good next step, can always be more aggressive later.

  • Pipeline finished with Failed
    25 days ago
    Total: 184s
    #453902
  • Pipeline finished with Failed
    25 days ago
    Total: 159s
    #453907
  • 🇬🇧United Kingdom longwave UK

    We already have the composer drupal-phpunit-upgrade command that should still work if the constraints are set correctly back from when we supported both PHPUnit 8 and 9.

  • 🇮🇹Italy mondrake 🇮🇹

    #52 blast from the past :)

  • 🇮🇹Italy mondrake 🇮🇹

    I am working on this.

  • Pipeline finished with Failed
    24 days ago
    Total: 367s
    #454305
  • Pipeline finished with Failed
    24 days ago
    Total: 155s
    #454311
  • Pipeline finished with Failed
    24 days ago
    Total: 199s
    #454321
  • Pipeline finished with Failed
    24 days ago
    Total: 250s
    #454339
  • Pipeline finished with Canceled
    24 days ago
    Total: 422s
    #454350
  • Pipeline finished with Canceled
    24 days ago
    Total: 258s
    #454354
  • Pipeline finished with Canceled
    24 days ago
    Total: 491s
    #454365
  • Pipeline finished with Canceled
    24 days ago
    Total: 141s
    #454367
  • 🇮🇹Italy mondrake 🇮🇹

    Done #52. Now PHPUnit 11 executes for Unit tests under PHP 8.5. It's a bit too conservative IMHO, but a first step.

  • Pipeline finished with Failed
    24 days ago
    Total: 680s
    #454370
  • 🇮🇹Italy mondrake 🇮🇹

    Updated IS, added a release note snippet, tests are green. CR still missing but let's agree this is good to go before writing one?

  • Pipeline finished with Failed
    24 days ago
    Total: 1540s
    #454418
  • 🇫🇷France andypost

    not clear why ordering has changes https://git.drupalcode.org/project/drupal/-/commit/0b61d1dacd6d8026d5eb1...

    and todo needs follow-up

  • Pipeline finished with Failed
    24 days ago
    Total: 1089s
    #454737
  • 🇮🇹Italy mondrake 🇮🇹

    #57 @andypost that's because in PHPUnit 10 the arguments coming from the DataProvider are passed to the test method as a list sequence (regardless of the array keys), whereas in PHPUnit 11 the array keys are used to pass each parameter to the corresponding named argument of the test method. So effectively PHPUnit 11 swaps arguments vis a vis PHPUnit 10 for the same test unless in PHPUnit 10 the sequence of passed-in arguments is aligned to the sequence of the arguments in the test method's signature.

    See

  • Pipeline finished with Success
    22 days ago
    Total: 559s
    #455865
  • 🇮🇹Italy mondrake 🇮🇹

    From #3515706-5: Switch the default test environment to PHP 8.4 :

    I wonder would it make sense to switch to PHPUnit 11 for 8.4 too? And let 8.3 and below onto PHPUnit 10? https://stitcher.io/blog/php-version-stats-january-2025

  • Pipeline finished with Success
    11 days ago
    Total: 511s
    #464776
Production build 0.71.5 2024