coveragepy
coveragepy copied to clipboard
nedbat
Report covered 'no-cover' lines
Originally reported by Anonymous
It is often the case that a feature will be added in one commit, but there isn't any client code that calls a particular method yet. As such, the method is marked
pragma: no cover
coverage passes and everyone continues along their merry way.
Later, when client code is added that exercises that code path, it would be nice if coverage.py could complain saying something like "WARNING: Block marked 'no cover' actually covered. You may want to remove the pragma." That way, we could continually make sure that the minimum amount of code is under 'no cover' pragmas.
- Bitbucket: https://bitbucket.org/ned/coveragepy/issue/251
Original comment by Toshio Kuratomi (Bitbucket: toshio, GitHub: toshio)
(Just FYI: I am using exclude_lines similar to jayvdb, I want to exclude lines that can only run on Python2 or only run on Python3 when the Python interpreter running them is the opposite version. These lines are essentially dead code on that version of Python. I want coverage to warn me if that code is being executed on that Python version as it means either my code is wrong or my marking of it as dead code on python2/3 is wrong.)
Original comment by Toshio Kuratomi (Bitbucket: toshio, GitHub: toshio)
Looking at the code handling exclusions, it appears that exclusions are recorded and tracked all the way up to Coverage.Analysis but the excluded lines are being removed from executed statements at a much lower level, when we parse the python file. To implement this, I think we need to keep excluded lines in the executed statements until much later. I think Analysis makes sense for that as it is where other statistics get combined and calculated before the Reporter phase.
Original comment by Andrei Fokau (Bitbucket: andreif, GitHub: andreif)
Wonderful idea, all exclude_lines number should be reported.
Maybe even omitted *.py files as well.
Original comment by John Vandenberg (Bitbucket: jayvdb, GitHub: jayvdb)
In my experience, #pragma: no cover is most often used for OS specific paths.
These proposed alerts would be a good way to advertise that platform specific code can be properly required when the tests are being run on the respective hosts using env vars https://bitbucket.org/ned/coveragepy/issues/563
The only complexity is whether this alert is only added for the default #pragma: no cover, in which case you would want to add another default which doesnt trigger the alert, and/or a way to turn off the warning (but imo the ecosystem is improved if the warnings cant be easily disabled, as it is important that coders limit when the block isnt covered to specific conditions).
Hmm, I'm not sure I approve of pragma'ing away code that you intend to cover eventually, but it's an interesting idea to alert the user to unneeded pragmas.
Did anything move forward on this topic? I would really like this feature for the exact same reason as the one originally reported here.
Hi @nedbat No problem, thanks for the answer, it was just to be sure I did not miss something. I am quite surprise that this feature is not much requested while it seems very important to me :)
Thinking about this more, there's a subtlety here. Let's say I have some code that is only for Windows and I only run my tests on Linux. I might put this in my configuration file:
[report]
exclude_lines =
if WINDOWS:
This has the advantage that the entire clause beneath the if is excluded:
blah_bla() # line 1
if WINDOWS: # line 3
windows_thing() # line 4
another_windows_thing() # line 5
more_blah() # line 7
Here, lines 3,4,5 will be excluded. But line 3 is executed, when the test for WINDOWS happens, and is false.
Simply reporting on the excluded lines that were executed would report line 3, which is not interesting.
Do other people use exclude_lines like this? Ideally, this should report that no excluded lines were executed.
As mentioned in #1482, my idea to help resolve this is to have a configuration option that behaves similarly to MyPy's warn_unused_ignore which, if true, emits a warning if a line or file contains a type: ignore comment even though the line contains no typing errors.
In the case of Coverage, the behavior of an analogous warn_unused_exclude option (defaults to false) would be to, if true, emit a warning when a line or branch is excluded from coverage reports even though that line or branch is in fact fully covered. Ideally, this warning would cause Coverage to return a non-zero exit code, but I could live with that not being the case.
The main complication is that for users who have customized exclude_lines, setting warn_unused_exclude to true could produce false-positive warnings. That is, when users exclude code from coverage it is either because they don't care whether those sections of code have coverage, or because they would like those sections to be covered, but were unable to for some reason - warn_unused_exclude may produce false-positive warnings in the former case.
To resolve this complication, @nedbat suggested that warn_unused_exclude could be a list of regexes which are the exclusions to warn about. I like this idea, though to keep things simple it could also be added later under a separate configuration option if users turn out to need it.
Thanks for this update!
I have just 2 comments about this:
- IMO lines covered while they should not can be the result of a bug (it actually happened to me to discover a bug because I wondered why a line was covered while it had no reason to be), so I think we should be able to choose between warnings and errors.
- I am not sure I understand your point about the false-positives warnings. If people don't care whether a line is covered or not but this line is actually covered, why would they exclude this line? The line is covered and included and that's it, right?
I am not sure I understand your point about the false-positives warnings. If people don't care whether a line is covered or not but this line is actually covered, why would they exclude this line? The line is covered and included and that's it, right?
As an example, I exclude def __repr__ lines in my project (coverage.py's test suite!) because I don't test repr's. They are there to help me when debugging code. Some of them might accidentally get executed, and that's ok, but I don't want to be nagged about them if they aren't run.
Keep in mind, lines can be excluded by a regex in the [report] excluded_lines setting, not just with explicit comments on each excluded line. Maybe that distinction is useful in this idea?
Ah yeah yeah I see, thanks. I never use regex excludes, only explicit ones in code, that's why I was lost on this point.
But now I am wondering if we actually don't need 2 different features: one 'strict' feature to deal with the code that is not supposed to be covered (these lines raise an error if they are covered) and one less strict to deal with the code that we don't care whether it is covered or not (these lines could raise a warning or just nothing to not flood the logs). WDYT?
I think we should be able to choose between warnings and errors.
In my opinion it should always result in an error. Typically, coverage is checked in CI, so it's unlikely that people will actually act on the warnings when they happen. For example, in MyPy, even though the option is called warn_unused_ignore, occurrences during a run result in a non-zero exit code. So perhaps the option in Coverage would be more accurately called unused_exclude_error.
I am wondering if we actually don't need 2 different features...
I think this is effectively what @nedbat was suggesting earlier with the idea of having a list of line patterns that should result in an error if unused. On thinking further though, I believe that this option would almost always be a single entry containing pragma: no cover. Every case I can think of where I'd want to get notified of unused exclusions are those that I manually introduced by adding a pragma: no cover comment. If I customize exclude_lines, it's probably because there are predictable branches of code that I know I won't be testing.
Given this, perhaps the option we add could be called exclude_comment where any unused instance of your chosen comment would result in an error. Initially, I could see the default for exclude_comment being null, and exclude_lines remaining ['pragma: no cover'] as it is now. Users who want to opt-in to these new "unused errors" would configure exclude_comment to be pragma: no cover. Then, assuming feedback from early adopters is positive, the defaults for the two options could switch to exclude_comment='pragma: no cover' and exclude_lines=[], thus turning on unused errors by default. An additional benefit is that, after this change in default values, you won't need to repeat pragma: no cover in exclude_lines anymore.
Given this, perhaps the option we add could be called exclude_comment where any unused instance of your chosen comment would result in an error. Initially, I could see the default for exclude_comment being null, and exclude_lines remaining ['pragma: no cover'] as it is now. Users who want to opt in to these new unused errors would configure exclude_comment to be pragma: no cover. Then, assuming feedback from early adopters is positive, the defaults for the two options could switch to exclude_comment='pragma: no cover' and exclude_lines=[], thus turning on unused errors by default. An additional benefit is that, after this change in default values, you won't need to repeat pragma: no cover in exclude_lines anymore.
This solution looks great! :star_struck:
I do think @rmorshea's solution is a good one.
I'd like to suggest that annotations (e.g. pytest-cov's --cov-report=annotate option, which I understand to be implemented through this package) should have a new symbol for "excluded but covered anyway" regardless of which exclusion is used. (I'd suggest '+', since that would keep the annotation file diff-format friendly.)
I like the idea of a new setting indicating lines that are expected to be unexecuted, and will warn if they are executed. Like exclude_lines, it would be a regex that matches any part of the line (though clearly comment contents would be the most likely use).
I'd like to suggest that annotations should have a new symbol...
It's very unlikely that the annotations command will get new symbols. At the very least, it would not be in the first implementation.
Like exclude_lines, it would be a regex that matches any part of the line
If this is how we want the option to work, perhaps it could be named no_cover_lines in contrast to exclude_lines. The implication of no_cover_lines is that you expect these patterns to not be covered, and it is thus deserving of a warning if they are. In contrast, the exclude_lines option name would be meant to suggest that lines matching these patterns are literally excluded from analysis and treated as if they don't exist.
Another option: warn_if_run_lines, which is more direct. Also, I'm wondering about which command should issue the warning. coverage report is a natural choice, but there are other reporting commands that could also do it: html, xml, json, annotate, lcov. But the warning is also quite distinct from any reporting those commands do, and I wouldn't want to add it to all of them. Should it be a new command? Or an option on one/some/all of the reporting commands?
The way I'm thinking about this option is that it should cause both coverage of the given lines to be treated as excluded, and a warning if they are covered. The idea being that, the lines from this option and the lines from exclude_lines could be mutually exclusive. That is, at some point, the default for exclude_lines could be [], and the default for this new option could be ['pragma: no cover']. If this is how the option is to work, then the issue with the warn_if_run_lines name is that it suggests that a warning will be produced if the lines are covered, but not that the lines will be marked as excluded. In this case, no_cover_lines seems like a more appropriate name.
If instead the option should only cause the given lines to produce a warning when covered, then warn_if_run_lines would be an apt choice of name. The result being that warn_if_run_lines and exclude_lines would control discrete behaviors - warn_if_run_lines controls warnings on line coverage, and exclude_lines causes lines to be marked as excluded in reports. However, I can't think of a case where you'd want to specify a line in warn_if_run_lines but not in exclude_lines. Thus my instinct to have a no_cover_lines-like option that causes both coverage warnings and exclusion markings in reports.
But the warning is also quite distinct from any reporting those commands do...
The --fail-under option, which is present for of all the reporting commands, seems somewhat similar in that the desire would be for these warnings to cause a failure either optionally or by default, as was mentioned in this comment.
@rmorshea I must respectfully disagree with your suggestion to exclude these lines from reports (although this could be an optional behaviour for those who desire it). It's especially valuable that the annotate format has exactly the same number of lines as the source file, and can be scroll-bound with the original source (to use the VIm term).
I've amended the language of my comment to more accurately reflect my intended meaning. Instead of saying that lines should be "excluded from reports" I now say that they should be "marked as excluded" or "treated as excluded".
I've amended the language of my comment...
I apologize for misunderstanding. Thanks for the clarification!
Thinking about how to do this some more... First, I wish there were a good word to mean, "lines I thought wouldn't execute, but did," like we have "unexecuted" for "lines I thought would execute, but didn't." In this comment, I'll call them surprises.
Reporting on surprises is not like reporting fail-under, because there a good deal of text to output for surprises. It will be like report --missing: we need to list files, and for each file, some line numbers that executed unexpectedly. I don't want the every report command to also output a text table of surprises.
So i think at first this should be limited to the coverage report command. We can add --show-surprises to parallel --show-missing. Surprise line numbers can be indicated with an asterisk or exclamation point. The --skip-covered option should become --skip-ok, meaning, don't show files that have nothing wrong with them.
Thoughts?
That's tough. Perhaps: unpredicted, unexpected, unforecast, unforeseen, or unanticipated?
IMO it has to be tough. We are speaking about lines that really have no reason to be executed, so if they are it means that there is an issue somewhere.
I would even go for something tougher, like illegally-executed.
I suggest "poseur". I like what it implies and I think it'd be my choice. Plus, it's nice and short.
Other possibilities are stowaway, overavid, impostor, or cosplayer.
Or borrow from some other language. (Besides French. Insert joke about the French here. ;-) Something like "sasaeng" (Korean), meaning: "obsessive fan who stalks or engages in other behavior constituting an invasion of the privacy of Korean idols". (From wikipedia.)
Other words that might shake something useful out of the trees are: stalker, gaslighter, erotomaniac (ha!), yandere (Japanese), Zersetzungist (from the German Zersetzung),.
