Add support for coverage exclusion comments #26
Conversation
| * isDefined true, but throw on `start` | ||
| */ | ||
| def safeStart(tree: Tree): Int = scala.util.Try(tree.pos.start).getOrElse(-1) | ||
| def safeEnd(tree: Tree): Int = scala.util.Try(tree.pos.end).getOrElse(-1) |
There was a problem hiding this comment.
Is this a bug in the Scala compiler API? It seems like pos.start should be OK if pos.isDefined, but the old version crashes on my project codebase. Worth raising upstream?
There was a problem hiding this comment.
Prob not worth raising as its changed slightly in 2.11
|
This is an excellent feature, but quite a major one so I want to test this locally. I'll merge once I've done that if that's ok. |
|
👍 |
|
Fine by me. Let me know if you have any questions. |
|
Ok changes look good. Can you rebase for merge? |
|
Thanks, have done. I also added a minor tweak: after using this for a while, I have found that not allowing explanatory comments on the same line as the "$COVERAGE-OFF$" marker is too restrictive, so I have loosened that restriction. I don't think there's much chance of a false positive match against a comment that unintentionally includes the string "$COVERAGE-OFF$". |
|
Still says I can't merge. You sure there's no conflicts? |
|
It looks like a fast-forward from here. I've just made another pull request to fix that unit test on that thread. If you accept it, then I'll need to rebase this again. |
|
I can't merge the other on my.phone for.some reason
|
|
Can you merge it from your PC? Should I resubmit as a new PR or something? |
Add support for coverage exclusion comments
Use case and justification
I'd like to add a new feature: coverage exclusion comments.
A somewhat strict but fairly common use pattern for coverage tools is for projects to insist on 100% code coverage of everything, but allow documented exclusions where covering tests are not possible (e.g. platform-specific code) or not worthwhile (low value code etc.)
This can also be handled by projects just setting a coverage target less than 100%, say 90%. However, the problem with that is that you can't tell by looking at the code whether it is covered or not, and code reviewers do not get the opportunity to decide whether a junior developer's decision to leave some code uncovered is justified or not.
With the 100% + explicit exclusion comments approach, code reviewers can double check that they agree with all new exclusions, and people browsing the code can tell at a glance whether code is covered or not.
This feature is mainly useful to process-heavy commercial teams ;-)
Implementation notes
I had some trouble caused by
ScoveragePluign#Optionsbeing mutable (and being invalid during plugin construction), so I refactored things so that Options is only injected into the plugin after it has been fully initialized. (This fix also allows the "excludedPackages" to be converted to Regexes just once, rather than each time they are used, which is probably more efficient.)At one point I wanted to pass a
Treeto theCoverageFilterclass, but it seems that is some kind of inner class of the plugin, and I couldn't figure out how to declare a method outside of that class which was type compatible. In the end this version works well without that, so never mind.I wondered about possibly including the "excluded" code in the instrumentation, but just excluding it from the final coverage percent in the reports, and perhaps shading it pale red or pale green in the coverage reports. In the end, I decided that would be too much work, so I did this version, which is far simpler.
I found the "excludedPackages" setting to be fairly confusingly named, but I haven't changed it so as to avoid a BC break; I just added some documentation on what it actually means.
I had some trouble with off-by-one errors with line numbers: different parts of the Scala compiler API treat line numbers as 0- or 1-based respectively. Do you think it's worth raising a bug upstream?