Problem/Motivation

The PHPUnit coding standards for Drupal 8 allow us to use @covers and @coversDefaultClass annotations for two purposes:

  1. Reflect the coverage of the test in question to a coverage report generator.
  2. Document the coverage of the test in question.

A giant hunk of Drupal 8's unit tests are annotated as covering the wrong thing, or have syntax errors in their coverage annotation.

If we could rig the testbot to catch such errors, that would be great; it would be a documentation win as well as a code quality win.

Best Solution

Ideally, we'd just add --coverage-text to the PHPUnit script, and be done. But that's not available to Drupal.

Change SimpleTest?

SimpleTest runs the PHPUnit based tests individually as it encounters them, as an exec(). This means that trying to generate a coverage report (and thus trigger @covers errors), while possible, is hugely time-consuming. Adding --coverage-text to the command forked by SimpleTest turns a single 6-second test into a 6-minute one. Mulitply this by the ~7000 unit tests and you get a bad situation.

Change \Drupal\Tests\TestCase?

Adding a check within \Drupal\Tests\TestCase seems like a good idea, until you try and figure out where it should go. If you make it part of setUp(), then all child test classes have to call parent::setUp() within their own setUp() override. PHPUnit allows us to use before() and beforeClass(), but that leads to the next problem: Not all Drupal 8 tests subclass TestCase.

Use PHPUnit's Listener

That leaves us with PHPUnit's listener architecture. A listener class can be configured in phpunit.xml.dist, and this listener can perform the annotation check. It can also provide future standards-related services which we might use.

Scope Issues

The listener solution is good as long as SimpleTest runs each test individually. The listener solution immediately becomes terrible if we have a better testbot which can try to generate a coverage report.

Proposed resolution

Implement a listener for PHPUnit which can examine all code for broken @covers and @coversDefaultClass.

Remaining tasks

Figure out how to break out the fixes to current code so they're manageable.

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Unfrozen changes Unfrozen because it improves automated testing.
Disruption Disruptive because it will show us that we have a lot of broken annotations in unit tests, and we'll have to fix them.
CommentFileSizeAuthor
#29 interdiff.txt1.36 KBMile23
#29 2415441_29_pass.patch6.54 KBMile23
#29 2415441_29_fail.patch6.54 KBMile23
#24 interdiff.txt2.21 KBMile23
#24 2415441_24_pass.patch6.48 KBMile23
#24 2415441_24_fail.patch6.47 KBMile23
#21 2415441_21_pass.patch6.27 KBMile23
#21 interdiff.txt11.47 KBMile23
#21 2415441_21.patch6.26 KBMile23
#15 interdiff.txt681 bytesMile23
#15 2415441_15_pass.patch5.67 KBMile23
#13 interdiff.txt4.35 KBMile23
#13 2415441_13.patch5.67 KBMile23
#10 interdiff.txt1.62 KBMile23
#10 2415441_10.patch6.5 KBMile23
#5 2415441_5_pass.patch6.37 KBMile23
#5 2415441_5_phail.patch6.37 KBMile23
#1 2415441_do_not_test.patch4.22 KBMile23
#1 covers_errors.txt49.8 KBMile23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Active » Needs review
FileSize
covers_errors.txt49.8 KB
2415441_do_not_test.patch4.22 KB

Here's some data and a patch.

The text file is a log of the output of a plain-vanilla PHPUnit run. Note that it's a little unfair since it reports per test method run. Test methods can be run multiple times given a @dataProvider.

The patch changes Drupal to provide this kind of log by implementing a PHPUnit listener.

This is POC. Please discuss and improve.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Related issues: +#2409587: Incorrect documentation for @covers, +#2358657: Wrong @covers definitions in Drupal project, +#2328919: Remove () from a bunch of @covers definitions in PHPUnit, +#2364555: Add @covers annotation, fix some --strict for PHPUnit, +#2404489: CacheTagsInvalidatorTest::testInvalidateTags() has wrong @covers, +#2229123: Wrong @covers definition in MenuTreeTest, +#2275145: @covers is out of date in the path module, +#2258333: Invalid @covers annotation for TokenUnitTest, +#2219913: Incorrect @covers/@coversDefaultClass breaks coverage reports., +#2245185: Fix invalid @covers usage, +#2366645: Drupal\Tests\Core\Controller\AjaxControllerTest has wrong @covers., +#2413941: BubbleableMetadataTest::testApply has wrong @covers, +#2409655: General Cleanup of AjaxCommandsTest

Just a few current and past @covers issues. :-)

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Issue summary: View changes
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Issue summary: View changes
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
FileSize
2415441_5_phail.patch6.37 KB
2415441_5_pass.patch6.37 KB

Here's a more review-worthy patch.

The 'phail' patch will fail because it works, and fails @covers coding standards errors.

The 'pass' patch will pass because it comments out the line that adds the failures.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

Hmm. OK. :-)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Ideally, we'd just add --coverage-text to the PHPUnit script, and be done. But that's not available to Drupal.

Can you elaborate why not?

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

In the paragraph after that one... SimpleTest runs each test individually as its own test run, by calling exec() with the phpunit binary. If you add --covers-text to that run, phpunit generates a report for the whole project, rather than just the test you ran. That means a 6-second test takes 6 minutes to finish, per test, times the number of phpunit tests.

Also, even if you added --covers-text, you still wouldn't catch all the documentation errors. You'd have to add --strict, which would then lead to a bunch of errors related to @use, which we don't have a policy about.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Related issues: +#2418237: Fix incorrect @covers in PHPUnit tests

Using this patch, I made this: #2418237: Fix incorrect @covers in PHPUnit tests

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
FileSize
2415441_10.patch6.5 KB
interdiff.txt1.62 KB

New and improved.

This allows you to test an interface, and never checks if global functions exist.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work

@Mile23: Good work on this issue! Sorry for my late response.

I have a few questions:
- If we use the patch from this issue it must generate fails (see #2418237: Fix incorrect @covers in PHPUnit tests). But the testbot is happy. Is this something temporary or not. It would be nice if new tests must pass this issue before the testbot gives is a pass.
- 

+++ b/core/tests/Drupal/Tests/DrupalStandardsEnforcementListener.php
@@ -0,0 +1,186 @@
+        interface_exists($default_class, TRUE)) {

Do we want phpunit tests for interfaces?
- This patch allows tests to have no @coversDefaultClass. Is this something we want. Should this not be mandatory?
- This patch allows test-class to test different classes with the construction @covers SomeClass::someMethod. Is this something we want?
- This patch allows methods to have no @covers. Is this something we want?
- Should we not test if there is a @coversDefaultClass when there are @covers annotations without a class reference.
- The code from the patch looks good to me.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

If we use the patch from this issue it must generate fails (see #2418237: The Mother Of All @covers Issues). But the testbot is happy.

I'm still not sure why the pass/phail patches in #5 both passed. More research required. :-) It fails tests when you run phpunit from the command line:

[.....]

3367) Drupal\Tests\views\Unit\ViewExecutableUnitTest::testAttachDisplays
@covers method does not exist\Drupal\views\ViewExecutable::attachDisplays(): Drupal\Tests\views\Unit\ViewExecutableUnitTest::testAttachDisplays

/Users/paulmitchum/projects/drupal8/core/tests/Drupal/Tests/DrupalStandardsEnforcementListener.php:27
/Users/paulmitchum/projects/drupal8/core/tests/Drupal/Tests/DrupalStandardsEnforcementListener.php:113
/Users/paulmitchum/projects/drupal8/core/tests/Drupal/Tests/DrupalStandardsEnforcementListener.php:127
                                                              
FAILURES!                                                     
Tests: 7118, Assertions: 39174, Failures: 3367, Incomplete: 2.
Do we want phpunit tests for interfaces?

That is, of course, up for discussion. I found that there was a test that was trying to cover an interface, so I just allowed it.

  • This patch allows tests to have no @coversDefaultClass. Is this something we want. Should this not be mandatory?
  • This patch allows test-class to test different classes with the construction @covers SomeClass::someMethod. Is this something we want?
  • This patch allows methods to have no @covers. Is this something we want?

These are all matters of policy that can be changed pretty easily. That said: Not all test methods cover a class method or even a function. The main goal was to find bad (), missing ::, and non-existent classes and methods.

Should we not test if there is a @coversDefaultClass when there are @covers annotations without a class reference.

It's legal to test a global function. The current patch doesn't check if global functions exist because sometimes the test has to load them. For this reason I need to change it to listen to endTest() instead of startTest().

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs work » Needs review
FileSize
2415441_13.patch5.67 KB
interdiff.txt4.35 KB

Move the listener to endTest() so we can check global functions.

Also switch to \PHPUnit_Framework_BaseTestListener so we don't have to make a bunch of stub methods for the interface.

It would be nice if this causes some failures, but we'll see.

Status: Needs review » Needs work

The last submitted patch, 13: 2415441_13.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs work » Needs review
FileSize
2415441_15_pass.patch5.67 KB
interdiff.txt681 bytes

Ah good deal. Failures in #13 mean it's failing other tests for violating the standard.

Here's the same thing again without registering any of the failures, to show that this code passes other tests.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work 
  1. +++ b/core/tests/Drupal/Tests/DrupalStandardsEnforcementListener.php
@@ -0,0 +1,133 @@
+    $annotation = $test->getAnnotations();

    Can we rename $annotation to $annotations. The function returns an array of annotations.

  2. In the phpunit manual there is the construction "@covers ClassName" possible. Is that something that we also want to support?
  3. @Mile23: How do you want to tackle the problem of the 3157 fails in other tests?
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
  1. $annotations is better. I'm not in a position to make a patch right now though.
  2. I think what matters is what our standards are, since we're using these for documentation as well as for coverage reports.
  3. The 3175 fails are fixed by #2418237: Fix incorrect @covers in PHPUnit tests. I made this patch to make that patch.
daffie’s picture

daffie CreditAttribution: daffie commented

@Mile23: I need the "@covers ClassName" for #2409655: General Cleanup of AjaxCommandsTest. Unless you have a better idea for that issue.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

OK. @covers class::method should be allowed, unless we want to completely refactor AjaxCommandsTest, which we probably don't. :-)

Update: Oh yeah, it already is.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

@Mile23, I think @daffie is saying that AjaxCommandsTest wants to just have @covers class, as in it will only cover the methods in that class, but any method in the class.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs work » Needs review
FileSize
2415441_21.patch6.26 KB
interdiff.txt11.47 KB
2415441_21_pass.patch6.27 KB

Re: #16:

  • Changes $annotation to $annotations.
  • Allows for @covers ClassName.

Also moved things around quite a bit. This test now lives in the \Drupal\Tests\Standards namespace and the class name doesn't ring of fascism.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented 
  1. +++ b/core/tests/Drupal/Tests/Standards/DrupalStandardsListener.php
@@ -0,0 +1,151 @@
+ * @file
+ * Listener for PHPUnit tests, to enforce various coding standards.
...
+ * Enforces coding standards within test runs.

    Merge these two lines, the @file doc block should be "Contains \Drupal\..." 

  2. +++ b/core/tests/Drupal/Tests/Standards/DrupalStandardsListener.php
@@ -0,0 +1,151 @@
+        if ('' == trim($covers)) {
...
+        if (FALSE !== strpos($covers, '()')) {
...
+        if(FALSE !== strpos($covers, '::')) {

    First line: Might as well use === here
    Third line: missing space before (

    All three: We don't use Yoda conditions anywhere else, please reverse the comparisons

The last submitted patch, 21: 2415441_21.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
FileSize
2415441_24_fail.patch6.47 KB
2415441_24_pass.patch6.48 KB
interdiff.txt2.21 KB

Fixes coding standards issues from #22, plus others.

The last submitted patch, 24: 2415441_24_fail.patch, failed testing.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work 
  1. +++ b/core/tests/Drupal/Tests/Standards/DrupalStandardsListener.php
@@ -0,0 +1,157 @@
+              $this->fail($test, "@covers invalid syntax: Needs '::' in $covers");

    There are two possibilities why this error occurs. The first one you have and the second one is that it is a class name and that it does not exists. 

  2. +++ b/core/tests/Drupal/Tests/Standards/DrupalStandardsListener.php
@@ -0,0 +1,157 @@
+              $this->fail('@covers class does not exist ' . $class);

    I think that the variable $test needs to be added to the function fail(). 

  3. +++ b/core/tests/Drupal/Tests/Standards/DrupalStandardsListener.php
@@ -0,0 +1,157 @@
+    return class_exists($class, TRUE) || trait_exists($class, TRUE);

    @Mile23: Just to be sure: You have removed support for interface tests, but in comment #12 you have said that there are tests for interfaces.

daffie’s picture

daffie CreditAttribution: daffie commented

I have tested the patch 2415441_24_fail.patch with the function classExists changed to 

protected function classExists($class) {
  return class_exists($class, TRUE) || trait_exists($class, TRUE) || interface_exists($class, TRUE);
}

and the patch added from #2418237: Fix incorrect @covers in PHPUnit tests. The result from the testbot is green. So we are almost there!

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Parent issue: » #2423241: Merge ConfigurablePluginCollectionTest into DefaultLazyPluginCollectionTest.

Oh, I neglected to write down the reason for not allowing interfaces. I changed that but meant to change it back for the patch.

I originally allowed interfaces because I ran into a test that wanted to cover an interface, specifically ConfigurablePluginCollectionTest.

I happen to think we shouldn't allow @coversDefaultClass for interfaces, so I made this issue about it: #2423241: Merge ConfigurablePluginCollectionTest into DefaultLazyPluginCollectionTest. Regardless of how we stand on interfaces here, that issue fixes a poorly-formed test.

I'll make another patch that allows interfaces, and then we can argue about it after it's in. :-)

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs work » Needs review
FileSize
2415441_29_fail.patch6.54 KB
2415441_29_pass.patch6.54 KB
interdiff.txt1.36 KB

#26.1: Oops. Really no way to tell which error we're seeing, so I changed the error message.

#26.2 Nice catch.

#26.3 @coversDefaultClass for interfaces is allowed. Not sure they should be, but we can discuss that.

The last submitted patch, 29: 2415441_29_fail.patch, failed testing.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Postponed

It all looks good to me.
This patch is postponed on #2418237: Fix incorrect @covers in PHPUnit tests.
After that patch lands this issue get a RTBC from me.
Good work @Mile23.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Postponed » Needs review

The issue #2418237: Fix incorrect @covers in PHPUnit tests has landed. The patch 2415441_29_fail.patch should now pass after a retest.

daffie queued 29: 2415441_29_fail.patch for re-testing.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

It all looks good to me.
The documentation is in order.
This issue is about documentation and tests. So it is allowed for beta.
For me it is RTBC.

For the commiter: it is this patch 2415441_29_fail.patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Tempted to mark this needs tests :) - could have used https://stackoverflow.com/questions/3917909/how-to-indicate-that-a-phpun...

Committed 1a82911 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 1a82911 on 8.0.x 
    Issue #2415441 by Mile23: Automate finding @covers errors
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: +DX (Developer Experience)

Looks like this will be very helpful :)

Status: Fixed » Closed (fixed)

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