Deprecate Test Suites, no longer available in PHPUnit 10

The deprecations only affect the test of the feature itself.

Left a question in the comment, but I think this is ready for inspection by the core committers.

I'm not sure I agree with the understanding of the issue.

1. run-tests is _one_ way to run tests against Drupal. The existence and documentation of the use of core/phpunit.xml.dist is the other. Run-tests is so clunky, awkward, and slow I haven't used it in 10 years or more at this point? It works for testbot but I can't figure out how anyone would use it regularly.
2. moving away from exposing our tests to phpunit blocks deeper integration into the phpunit ecosystem which I feel like is _not_ where we want to be heading. Not running tests in your IDE natively, not being able to look at paratest, etc.

So we don't need to deprecated it, we need to replace it.

#5. Let me try clarifying since it's not clear.

• This issue has nothing to do with run-tests.sh. Test suites are not used by it.
• The 'test suite' in PHPUnit 10 is configured in phpunit.xml.dist only by identifying directories to be included in a suite, https://docs.phpunit.de/en/10.4/organizing-tests.html#composing-a-test-s..., no longer by passing a file that implement classes extending from \PHPUnit\Framework\TestSuite, which contain a static function suite() that explode the supported directories. This makes Drupal's implementation of the *TestSuite classes unproductive in PHPUnit 10. That's at least my understanding of https://github.com/sebastianbergmann/phpunit/commit/36354a9cdc69ecabd20a...

So the replacement in PHPUnit 10, independently from run-tests.sh, would be to remove the cruft implementation classes and replace (if possible) with a direct indication of the directories to scan in phpunit.xml.dist.
Hence deprecation here.

We originally tried to do this in directory definitions but eventually had to give up because it was so dynamic and varied with the structures Drupal can exist with. PHPUnit is also pretty limited in how you can define these. I don't think that requirement hasn't really changed so I don't think we can deprecate without a replacement.

I don't think this is a problem though, if we're clever, we can probably move the logic into the events available in PHPUnit 9+ with probably opens up a lot better functionality and growth. We should even be able to do a deprecation in Drupal 10 without converting fully to PHPUnit 10. Just triggered if the static method is called directory and then pointing users to update their local configuration along the timelines of the PHPUnit 10 to what ever the event system looks like.

Time will tell. Anyway we agree these classes will have to go away, right?

So yeah, I was hopeful about the event and extensions but after tracing up and down there's not much to be done. The classes need to go away, just not sure what the path forward for supporting our various testing logic. Mostly discovering tests in submodules and contrib.

Added the original that added discovery logic to summary.

MR still applies, up for review again then

The deprecations need a change record to point to.

Draft CR @ https://www.drupal.org/node/3405829 →

Adjusted references to CR and rebased.

I'm not sure I was being clear since this is back to RTBC but my concerns aren't addressed.

Because of the way this is structured and the way the message is worded this looks like we're deprecating phpunit entirely.

Remaining self deprecation notices (2)

  1x: \Drupal\Tests\TestSuites\UnitTestSuite is deprecated in drupal:10.3.0 and is removed from drupal:11.0.0. There is no replacement. See https://www.drupal.org/node/3405829

  1x: Drupal\Tests\TestSuites\TestSuiteBase is deprecated in drupal:10.3.0 and is removed from drupal:11.0.0. There is no replacement. See https://www.drupal.org/node/3405829
    1x in UnitTestSuite::suite from Drupal\Tests\TestSuites

The problem is, there's no replacement for the class, but we should have a replacement in PHPUnit 10 for running PHPUnit tests.

I was asking that we figure out what that replacement is, then provide that explanation in the message and CR instead of just saying there's no replacement.

NW then.

Maybe to limit the scope, keep this clean, and not build a circular dependency between this and the larger PHPUnit upgrade we can do s/There is no replacement/There is no replacement and test discovery will be handled differently in PHPUnit 10./ and then the CR has some block about following the PHPUnit 10 upgrade issue. At least if that's ok with committers.

We can then clarify everything when finalize the upgrade path.

I think #18 is OK, the change record can point out this doesn't affect running individual tests locally (or run-tests.sh on gitlab) etc.

Do we need a new follow-up issue to figure out what the replacement is or is one already open? There's quite a few notes in the issue summary of 🌱 [meta] Support PHPUnit 10 in Drupal 11 Active which could be copied over along with findings from here.

Changed deprecation message.

Seems to have phpcs failures.

Spell-checking… I cannot explain the job failure though, seems related to git branches, not to real spelling errors

Rebase the branch against 11.x; the spellchecking errors have been solved in the core repo but GitLab CI runs the scripts from when the MR was branched.

Just did that ^
:)

All green now (as @longwave already predicted above apparently ;)

Thanks @Spokje! Mind sharing what commands did you use to get that?

The rebase button/link has disappeared in MRs, but just adding /rebase as a comment on the MR (so in this case: https://git.drupalcode.org/project/drupal/-/merge_requests/5452) will auto-rebase if there aren't any conflicts GitLab can't solve.

TIL^^

Thanks!!

I also was not aware of that thanks!

I believe feedback has been addressed though

Note on the TIL: You must have push-access on the MR for the magic to work.

Committed dccd322 and pushed to 11.x. Thanks!

Also published the change record. See you all in the next PHPUnit 10 issue!

longwave → closed merge request !5452

This broke the performance test job on gitlab: https://git.drupalcode.org/project/drupal/-/jobs/788445 Not sure exactly why yet but reverted for now.

I think that’s because those tests are run directly via Phpunit CLI —group and not via run-tests.sh.

That triggers PHPUnit’s own test discovery that loads all the classes and therefore triggers the deprecation errors. run-tests uses its own test discovery and then runs test on a file-by-file basis, so that doesn’t occur.

Not sure what is the best way forward. Maybe just avoid the @trigger_error of the deprecation of the classes so we don’t get this issue with autoloading.

mondrake → reopened merge request !5452

This could do, I think.

We remove @trigger_error on classes, that fires on class autoloading, leaving the @deprecate annotations. That's enough to have PHPStan detect usage of deprecated classes, in case - that's demonstrated by the fact that we need to add to PHPStan baseline the extension of TestSuiteBase by its child classes.

I've unpublished the CR

So what was the solution for the problem seen in #36? Or still being determined?

#37 is the explanation, and #39 is the solution. Is that not clear enough?

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.

Come on, bot…

Looks good to me, will commit later if no comments since this already landed once.

Needs a rebase.

rebased, no clashes

Committed/pushed to 11.x and 10.3.x, thanks!

mondrake → closed merge request !5452

Published the CR. As to ‘what now, then?’, the MR in 📌 Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency Fixed has now a proposal to include the files to be included in the suites, that covers core lib tests, core module tests, and tests for modules that are placed in the /module subdirectory. I think the CR will have to be updated once that eill be confirmed to be the final approach for D11/PHPUnit 10.

Automatically closed - issue fixed for 2 weeks with no activity.