PHPUnit 10 (Feb 2023) introduced attributes to replace the annotations that were previously used to indicate test configuration to PHPUnit. PHPUnit 11 is triggering unsilenceable deprecations if it detects tests that use annotations. PHPUnit 12 (Feb 2025) will no longer parse annotations.
Drupal's TestDiscovery class, that builds the tests to be executed by run-tests.sh, mandates tests to use @group annotations. It fails if only #[Group(...)] attributes are used.
Refactor TestDiscovery to allow usage of #[Group(...)] attributes, and deprecate code that parses @group annotations, for removal in the next Drupal's major.
nope
nope
nope
Drupal tests now can use PHPUnit's 10 attributes instead of the legacy annotations. New tests MUST use attributes.
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
mondrakeComment #4
mondrakeTests need fixing, but I think the concept is reviewable.
Getting rid of custom code and just relying on PHPUnit to discover tests is a bit too far and actually PHPUnit itself only recently changed format for
--list-tests-xmloutput, and modified test list filtering options, in PHPUnit 11.1. So better wait to be on that before trying it.For now just extended TestDiscovery to look into attributes of test classes, falling back to annotations if they do not exist (like PHPUnit itself is doing).
Will fix tests tomorrow.
Comment #5
mondrakeTests are green now.
Comment #6
mondrakeAdded draft CR.
Comment #7
mondrakeFWIW, I am pretty close to having an alternative implementation using PHPStan's BetterReflection.
This would have the benefit of not loading all test classes via PHP's Reflection.
But two drawbacks:
1) it will be much slower
2) it would require, at least for now, #3441353: Downgrade (temporarily) nikic/php-parser to ^4
I do not think that's worth doing. In the future, I'd suggest to switch TestDiscovery to get the test list by executing PHPUnit's CLI with the
--list-tests-xmloption and parsing the resulting XML. That would effectively mean delegating 'how' test classes are discovered entirely to PHPUnit, in a separate process so that PHP's own allocated memory allocated would not be a constraint (if it is right now, today, I doubt anyway).Comment #9
andypostJust a question why new conflict added to composer?
Comment #10
mondrakeMR!8054 uses PHP reflection.
MR!8125 implements #7, uses BetterReflection (static reflection, no class loading in memory), and requires downgrading nikic/php-parser to 4, for the reasons discussed in #3441353: Downgrade (temporarily) nikic/php-parser to ^4.
We need to decide on which horse to bet.
Comment #11
andypostAs I get https://github.com/Roave/BetterReflection/pull/1387 already fixed and BetterReflection should work with v5 too
Comment #12
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #13
mondrakeCan I ask, please, to get a review of the two options (see #10) and guidance on which one (or others) to follow? Not very keen in keeeping two alternative approaches rebased just to keep the NR; the second option would require rebasing anytime a dependency is bumped.
Comment #14
mondrakeRebased MR!8054
Comment #15
mondrakeComment #16
mondrakeRebased MR!8054
Comment #17
mondrakeRebased, and fixed for new CS rules
Comment #19
mondrakeRebased 8054, and hidden 8125.
Comment #20
mondrake.
Comment #21
smustgrave commentedShould all @group annotations be replaced here?
Comment #22
mondrakeNo, there are thousands, will have to happen in follow ups… they will have to go before we bump PHPUnit to 11, though.
Here we are only allowing run-test.sh to find test defined by Group attributes along with those defined by @group annotations. So we can migrate bit by bit.
Comment #23
mondrakeThe first follow up, to break the ice, would be #3446693: Convert test annotations to attributes in Drupal/Test/Component
Comment #26
catchDid a cursory review of the code and looks OK - not in-depth yet.
This could use an issue summary update and a change record to indicate that attributes will be supported. We'll need to decide if we backport this to 10.x so that contrib tests using group attributes, I think that has to be a yes if we want people to use it.
Also should we have a follow-up to try to use phpunit's discovery or did you end up deciding that's too far off?
Comment #27
mondrake@catch
can't do that alas - attributes are a PHPUnit 10+ thing, and D10 is testing with PHPUnit 9.
Comment #28
catchBut if we're doing the discovery, does PHPUnit 9 care? (apologies if this is a silly question). Is the problem that the attribute class wouldn't exist?
Comment #29
mondrakeJust noticed CR is there already, https://www.drupal.org/node/3447698; please review
Comment #30
mondrake#28 actually that's true: https://www.drupal.org/project/drupal/issues/3445106#comment-15587547 you can have attributes and annotations in the same test in PHPUnit 9, apparently. Need probably to decide what to use where though, to avoid duplication and possible mistakes.
EDIT - however, discovery would still be based on annotation in D10/PHPUnit 9; without the class existing, the attribute syntax is just a comment IIRC
Comment #31
mondrakeRe #26 I think now we have the tools to do a
see #3497431: Deprecate TestDiscovery test file scanning, use PHPUnit API instead. That is an alternative approach to the one here, and simpler.
Comment #32
mondrakeI think #3497431: Deprecate TestDiscovery test file scanning, use PHPUnit API instead is better than this, and this is now just a duplicate. Feel free to reopen in case of disagreement.