- 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.
- Status changed to Active
11 months ago 6:46pm 9 May 2024 - 🇮🇹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.
- 🇦🇺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.
- Status changed to Needs work
3 months ago 1:23pm 2 January 2025 - 🇬🇧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. - 🇬🇧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 thetrigger_error
inDebugClassLoader::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 newSourceMap
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 likeSYMFONY_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.
- 🇮🇹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.
- 🇮🇹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. - 🇮🇹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.
- 🇮🇹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.
- 🇮🇹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 🇮🇹
PHPUnit 11.5.3, due shortly, fixes the upstream bug so IMHO it's worth waiting for it.
- 🇮🇹Italy mondrake 🇮🇹
Merged 📌 Remove redundant entries in phpunit.xml.dist Postponed in here.
- 🇬🇧United Kingdom catch
- 🇮🇹Italy mondrake 🇮🇹
Bumped PHPUnit to 11.5.3, reverted change to
@group 44
annotation, ready for review again. - 🇮🇹Italy mondrake 🇮🇹
Bumped to PHPUnit 11.5.7 and adjusted PHPStan baseline after commit of 🐛 Fix static call to instance methods in PHPUnit Active
- 🇮🇹Italy mondrake 🇮🇹
merged with HEAD after commit of 🐛 Extend PhpUnitTestDiscoveryTest to test also PHPUnit API Active
- 🇦🇺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.
- 🇬🇧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 🇮🇹
Done #52. Now PHPUnit 11 executes for Unit tests under PHP 8.5. It's a bit too conservative IMHO, but a first step.
- 🇮🇹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?
- 🇫🇷France andypost
not clear why ordering has changes https://git.drupalcode.org/project/drupal/-/commit/0b61d1dacd6d8026d5eb1...
and todo needs follow-up
- 🇮🇹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
- 🇮🇹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