Problem/Motivation
PHPUnit's code coverage tool is great, but unfortunately there is code that is:
a) either not unit testable [ procedural function call, external dependency, ... ]
b) a false positive like the ending line of an array according to Drupal's code standards.
As of now there are 2 functions using this annotation in Core.
Proposed resolution
Allow usage of @codeCoverageIgnore annotations on:
- classes
- class functions
- individual lines (for false positives)
Remaining tasks
- Discuss
- Change coding standards (if we want to do this)
User interface changes
- None
API changes
- None
Comments
Comment #1
tim.plunkettIn contrib I use the block-level @codeCoverageIgnore for one line methods that wrap functions like drupal_set_message.
There are a couple in HEAD on methods in MenuTreeParameters.
What I think is weird is something like
continue; // @codeCoverageIgnoretaken from #2395143-113: YAML parsing is very slow, cache it with FileCache.Mostly because that seems like a completely testable line of code...
Comment #2
neclimdulAm definitely for allowing them as well.
I'm not really sure we need to encourage them on trailing array lines though. I'm aware there was some weirdness around this at some point. Maybe it still exists but in the coverage I just ran of
\Drupal\Core\Form\ConfigFormBaseeverything was fine. We should only be excluding actually non-testable code.Comment #3
mile23tl;dr: Mark up the test as much as possible rather than the code under test. Not a fan of
@codeCoverageIgnore.You *can* unit-test a procedural function, as long as it has dependencies you can mock.
If you want to test code that depends on global functions without covering those functions, then it's time to use
@uses. https://phpunit.de/manual/current/en/appendixes.annotations.html#appendi... In fact, this helps with--strict-coverage, and if we care about coverage at this level, we should re-investigate adding strict coverage to our test suite.We should use
@coversNothingin test method docblocks that aren't unit-oriented, because it spells out the intent of the test. https://phpunit.de/manual/current/en/appendixes.annotations.html#appendi...Beyond that, I'm not sure whether there's a justification for code declaring itself exempt from coverage and complexity reports. That seems like it defeats the purpose.
For instance,
MenuTreeParameters::setMaxDepth()(since its mentioned above). What do we gain by avoiding coverage there?setMaxDepth()is a simple setter. Is it an annotation that's supposed to say, "Don't unit test this, it's only two lines?"False positives might be a valid reason for
@codeCoverageIgnore, but really that's an upstream problem, and if its fixed then we're potentially stuck with the opposite problem.Comment #17
smustgrave commentedThank you for creating this issue to improve Drupal.
We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.
Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!
Comment #18
neclimdulhttps://docs.phpunit.de/en/12.0/code-coverage.html#ignoring-code-blocks
Supported in future versions of phpunit so seems its still relevant.
Comment #19
smustgrave commentedThanks, going to keep the tag for now for stats