Problem/Motivation
The PHPUnit coding standards for Drupal 8 allow us to use @covers
and @coversDefaultClass
annotations for two purposes:
- Reflect the coverage of the test in question to a coverage report generator.
- 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
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. |
Comment | File | Size | Author |
---|---|---|---|
#29 | interdiff.txt | 1.36 KB | Mile23 |
#29 | 2415441_29_pass.patch | 6.54 KB | Mile23 |
#29 | 2415441_29_fail.patch | 6.54 KB | Mile23 |
Comments
Comment #1
Mile23Here'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.
Comment #2
Mile23Just a few current and past
@covers
issues. :-)Comment #3
Mile23Comment #4
Mile23Comment #5
Mile23Here'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.
Comment #6
Mile23Hmm. OK. :-)
Comment #7
tstoecklerCan you elaborate why not?
Comment #8
Mile23In 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.
Comment #9
Mile23Using this patch, I made this: #2418237: Fix incorrect @covers in PHPUnit tests
Comment #10
Mile23New and improved.
This allows you to test an interface, and never checks if global functions exist.
Comment #11
daffie CreditAttribution: daffie commented@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.
-
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.
Comment #12
Mile23I'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:
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.
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.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 ofstartTest()
.Comment #13
Mile23Move 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.
Comment #15
Mile23Ah 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.
Comment #16
daffie CreditAttribution: daffie commentedCan we rename $annotation to $annotations. The function returns an array of annotations.
Comment #17
Mile23$annotations
is better. I'm not in a position to make a patch right now though.Comment #18
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.
Comment #19
Mile23OK.
@covers class::method
should be allowed, unless we want to completely refactorAjaxCommandsTest
, which we probably don't. :-)Update: Oh yeah, it already is.
Comment #20
tim.plunkett@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.Comment #21
Mile23Re: #16:
@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.Comment #22
tim.plunkettMerge these two lines, the @file doc block should be "Contains \Drupal\..."
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
Comment #24
Mile23Fixes coding standards issues from #22, plus others.
Comment #26
daffie CreditAttribution: daffie commentedThere 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.
I think that the variable $test needs to be added to the function fail().
@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.
Comment #27
daffie CreditAttribution: daffie commentedI have tested the patch 2415441_24_fail.patch with the function classExists changed to
and the patch added from #2418237: Fix incorrect @covers in PHPUnit tests. The result from the testbot is green. So we are almost there!
Comment #28
Mile23Oh, 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. :-)
Comment #29
Mile23#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.Comment #31
daffie CreditAttribution: daffie commentedIt 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.
Comment #32
daffie CreditAttribution: daffie commentedThe issue #2418237: Fix incorrect @covers in PHPUnit tests has landed. The patch 2415441_29_fail.patch should now pass after a retest.
Comment #34
daffie CreditAttribution: daffie commentedIt 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.
Comment #35
alexpottTempted 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.
Comment #37
Wim LeersLooks like this will be very helpful :)