Problem/Motivation

@requires module xxxxx is not a supported syntax in PHPUnit 10, and its parser no longer processes it.

The valid types of @requires are described here, https://docs.phpunit.de/en/10.5/annotations.html#requires

We can no longer rely on PHPUnit internal Test::parseTestMethodAnnotations() method (which is no longer existing BTW), nor on its new Metadata API, to fetch this information.

Steps to reproduce

Apparently the only test using @requires module xxxx is Drupal\Tests\jsonapi\Functional\FileUploadTest, but there are references to it in KernelTestBase and in some fixtures.

Proposed resolution

tbd

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3418379

Command icon Show commands

Start 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:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake opened merge request !6412

mondrake’s picture

mondrake
Location 🇮🇹
commented

One proposal could be to replace the annotation @requires module xxxx with an attribute, say #[RequiresDrupalModules(['xxxx']), and usual deprecation dance.

However, there only are 3 tests using this feature:

  • KernelTestBaseTest
  • BrowserTestBaseTest
  • FileUploadTest

Two are to test the feature itself, and for the third (FileUploadTest) the check is actually not even triggered, not sure why - but in any case it can be replaced with a simple markTestSkipped.

So not sure - is it something we need to keep supporting, or can we get rid of it altogether?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Related issues: +#3261817: TestRequirementsTrait::checkRequirements() does not work as expected

This has been broken for some time, see #3261817: TestRequirementsTrait::checkRequirements() does not work as expected

mondrake’s picture

mondrake
Location 🇮🇹
commented

Well two years not working means no one needs it. I propose to deprecate for removal.

spokje’s picture

spokje
Primary language Dutch
commented

Well two years not working means no one needs it. I propose to deprecate for removal.

Valid point IMHO, deprecation dance++

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Title: '@requires module xxxx' is not parsed in PHPUnit 10 » Deprecate testMethodRequiresModule and testRequiresModule
Status: Needs review » Reviewed & tested by the community

Deprecation seems good and has test coverage.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Deprecate testMethodRequiresModule and testRequiresModule » Deprecate Drupal\Tests\TestRequirementsTrait::checkRequirements and ::checkModuleRequirements

More accurate title.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Updated CR

spokje’s picture

spokje
Primary language Dutch
commented

After a night sleep (I know, I shouldn't do that...), I'm a bit worried to drop this functionality.

Core doesn't use it, that's clear by now, and it's broken, but contrib uses it (a lot): http://codcontrib.hank.vps-private.net/search?text=%40requires%20module&...

Not sure if since it's not working, fixing it/replacing it with an annotation could/would mean contrib tests will break, or is this long superseded by adding the modules-on-which-to-depend in the require-dev section of the modules composer.json and is this also dead code in contrib?

mondrake’s picture

mondrake
Location 🇮🇹
commented

This has probably been broken for 4 years now, since when #2950132: Support PHPUnit 7 optionally in Drupal 8, while keeping support for ^6.5 was committed.

Personally in the contrib modules I maintain I add the test required modules in the require-dev of the composer.json file. In fact it’s a must have if you want the CI tools to install them.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Given there was little activity over in #3261817: TestRequirementsTrait::checkRequirements() does not work as expected I also suspect this has been broken since then but nobody in contrib land has noticed, so to me that means it's not worth maintaining (or in this case, refactoring for PHPUnit 10), and so I agree this should be deprecated for removal.

spokje’s picture

spokje
Primary language Dutch
commented

Personally in the contrib modules I maintain I add the test required modules in the require-dev of the composer.json file. In fact it’s a must have if you want the CI tools to install them.

(Emphasis mine)

Thanks, there we have it, kill it with fire!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue tags: +Needs followup

We should have a followup though to clean up FileUploadTest, as those are six test methods that we should skip entirely if we can.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs followup

I think better to that here, it’s an unusual deprecation and leving things around won’t be good. Thre’s a comment somewhere, too.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Done.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Reviewed & tested by the community

Clean up makes sense, nice catch in core/tests/Drupal/KernelTests/KernelTestBase.php

quietone’s picture

quietone commented

The wiki has a reference to using @requires module that should be removed.

https://www.drupal.org/docs/automated-testing/phpunit-in-drupal/phpunit-...

spokje’s picture

spokje
Primary language Dutch
commented

Thanks @quietone, removed section from the wiki page.

  • longwave committed 614ff9d2 on 11.x 
    Issue #3418379 by mondrake, Spokje: Deprecate Drupal\Tests\...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Committed 614ff9d and pushed to 11.x. Thanks!

Also published the change record.

mondrake closed merge request !6412

Status: Fixed » Closed (fixed)

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