TestDiscovery expects @group annotations, fails with #[Group()] attributes

Tests 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-xml output, 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).

Tests are green now.

Added draft CR.

FWIW, I am pretty close to have 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, 📌 Downgrade (temporarily) nikic/php-parser to ^4 Closed: won't fix

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

Just a question why new conflict added to composer?

"nikic/php-parser": ">=5"

MR!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 📌 Downgrade (temporarily) nikic/php-parser to ^4 Closed: won't fix .

We need to decide on which horse to bet.

As I get https://github.com/Roave/BetterReflection/pull/1387 already fixed and BetterReflection should work with v5 too

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

Can I ask, please, to get a review of the two options 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.

Rebased MR!8054

Rebased MR!8054

Rebased, and fixed for new CS rules

mondrake → changed the visibility of the branch 3446705-using-BetterReflection to hidden.

Rebased 8054, and hidden 8125.

.

Should all @group annotations be replaced here?

No, 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.

The first follow up, to break the ice, would be 📌 Convert test annotation to attributes in Drupal/Test/Component Needs work

mondrake → closed merge request !8054

mondrake → reopened merge request !8054

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?

@catch

backport this to 10.x

can't do that alas - attributes are a PHPUnit 10+ thing, and D10 is testing with PHPUnit 9.

should we have a follow-up to try to use phpunit's discovery or did you end up deciding that's too far off

1. Must do prerequisite is 🐛 Ensure run-tests.sh and PHPUnit CLI run with the same list of tests to be executed Active to make sure we are testing the same things
2. It's not far ( 📌 Refactor PhpUnitTestRunner and TestRun internal classes Active latest revision is doing that already), but it's ... slow. The logic in that issue is to run PHPUnit's CLI to build the test list in a xml file, then parse that file to build the internal list in PHP. That takes time, also considering that PHPUnit invokes dataproviders to build the full testcases list. It's not dramatic in CI context but locally the difference is visible.

can't do that alas - attributes are a PHPUnit 10+ thing, and D10 is testing with PHPUnit 9.

But 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?

Just noticed CR is there already, https://www.drupal.org/node/3447698; → please review

#28 actually that's true: https://www.drupal.org/project/drupal/issues/3445106#comment-15587547 📌 Replace @dataProvider annotations with #[DataProvider()] attributes Postponed 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.