Problem/Motivation

TestCase::checkRequirements used to be protect, but was made private in PHPUnit 7.0

https://github.com/sebastianbergmann/phpunit/commit/932238a6a3018cdfac6c...

How is this still working? This controls the @requires module annotation.

After digging, I see Drupal core did this to test the method, but that just tests the trait. It doesn't work.

  /**
   * Public access for checkRequirements() to avoid reflection.
   */
  public function publicCheckRequirements() {
    return parent::checkRequirements();
  }

Steps to reproduce

Add @requires module, run PHPUnit. It fails when module does not exist.

But Drupal core is passing?

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#23 interdiff-3261817-1.txt1.11 KBharshitthakore
#23 3261817-1.patch1.11 KBharshitthakore
#6 3261817-fail.patch588 byteslongwave

Issue fork drupal-3261817

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

mglaman created an issue. See original summary.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Issue summary: View changes
mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Issue summary: View changes

mondrake made their first commit to this issue’s fork.

mondrake’s picture

mondrake
Location 🇮🇹
commented

??? I have no idea how I triggered #4... I was just watching on mobile.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review
StatusFileSize
new 3261817-fail.patch588 bytes

Great catch. Drupal core never actually uses the trait properly - which is why the core test suite passes - but it doesn't actually work.

The test cases for the trait call the trait directly, which as you have discovered, isn't a valid test. There is an attempt to use it in JSON:API's FileUploadTest, but this also doesn't work; the six test methods are never skipped as proved by the attached patch. There are no other uses of it in core.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

I dug into it, and there are no extension points into PHPUnit for this. In my test, I called `checkRequirements` in my setUp before parent::setUp to determine if the test should be skipped.

Status: Needs review » Needs work

The last submitted patch, 6: 3261817-fail.patch, failed testing. View results

mglaman opened merge request !1762

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

Made a MR which has #6 and how I fixed our tests, but placed in BrowserTestBase.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs work » Needs review

Putting to NR. We have a failing test-only patch and a proposal. I don't know what's the best approach here.

checkRequirements is private, has been since PHPUnit 7.0: https://github.com/sebastianbergmann/phpunit/blob/master/src/Framework/T...

PHPUnit 9 has a Requirements class.

        $missingRequirements = (new Requirements)->requirementsNotSatisfiedFor(
            static::class,
            $this->name
        );

However, it's not extensible: https://github.com/sebastianbergmann/phpunit/blob/a5ad0256678687e86f3fa8...

So any hook which is before the tests run would be appropriate: https://github.com/sebastianbergmann/phpunit/blob/master/src/Framework/T...

So it seems like setUp would be appropriate. I don't think beforeClass is invoked with each method (I have no idea.)

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

We also have TestDiscovery::parseTestClassAnnotations() which seems to be unused in core but also parses these annotations, with these comments:

    // @todo Enhance PHPUnit upstream to allow for custom @requires identifiers.
    // @see \PHPUnit\Util\Test::getRequirements()
    // @todo Add support for 'PHP', 'OS', 'function', 'extension'.
    // @see https://www.drupal.org/node/1273478
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Another place to do this check might be DrupalListener::startTest()?

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

I tried reviewing \Symfony\Bridge\PhpUnit\Legacy\SymfonyTestsListenerTrait::startTest, which has more code than ours.

It looks like you can only mark the test as risky

    public function markAsRisky(): void
    {
        $this->status = BaseTestRunner::STATUS_RISKY;
    }
rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
commented

I came across this issue trying to understand why a test with "@requires module ..." was not skipped and I also found this draft CR.
https://www.drupal.org/node/2857979

I associated it with this issue so it can be properly published when this is fixed

rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
commented
Related issues: +#1273478: Implement @requires and @dependencies within TestBase, mark tests as skipped

Vighneshh made their first commit to this issue’s fork.

jonathan1055’s picture

jonathan1055 commented

[off-topic] @mondrake in #4 and #5 yes I have often seen auto-comments saying "(user) made their first commit to this issue’s fork" when there was nothing by them and they had no intention to actually commit. Should that be raised as another issue?

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

#17 appears to be a drive by rebasing. Removing credit please see https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... for how to receive credit.

Reviewing the MR. The changes make sense to me. Think this can remain in the 10.0.x branch

xjm’s picture

xjm
she/her
Primary language English
commented
Title: TestRequirementsTrait::checkRequirements does not work as expected » TestRequirementsTrait::checkRequirements() does not work as expected
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 10.0.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Needs work

Thanks for finding this... how are more things not broken?

I think we also need to update the documentation of TestRequirementsTrait::checkRequirements(), since it explicitly says something about overriding the parent method.

Since this apparently changed in PHPUnit 7, that means Drupal 9 is also affected. As a bugfix to test code, this can be backported to 9.5.x. The diff of the MR applies to 10.1.x and 10.0.x but not 9.5.x, so we will eventually need a 9.5.x version as well.

Thanks everyone!

xjm’s picture

xjm
she/her
Primary language English
commented

Fixing attribution.

harshitthakore’s picture

harshitthakore
Primary language English
Location Ahmedabad
commented
StatusFileSize
new 3261817-1.patch1.11 KB
new interdiff-3261817-1.txt1.11 KB

@xjm created patch for 9.5.x. I don't know the exact process to backport the patch for specific Drupal version, So I've created patch for 9.5.x without commit.

xjm’s picture

xjm
she/her
Primary language English
commented

My feedback in #21 about updating the docblock also still needs to be addressed. Thanks!

mondrake’s picture

mondrake
Location 🇮🇹
commented

This is on the critical path for PHPUnit 10, and just looks like cruft ATM. In #3418379: Deprecate Drupal\Tests\TestRequirementsTrait::checkRequirements and ::checkModuleRequirements, there is a proposal to deprecate with no replacement.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue tags: +PHPUnit 10
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Closed (won't fix)

This has been broken since PHPUnit 7 was adopted and nobody has cared enough to fix it. PHPUnit 10 means we have to remove this code entirely, so we have deprecated it in #3418379: Deprecate Drupal\Tests\TestRequirementsTrait::checkRequirements and ::checkModuleRequirements and therefore this issue is won't fix.