Maximize code coverage in test suite #914
Conversation
|
Whatever you do, do not, I repeat NOT make code coverage into a metric, or goal, or prerequisite for accepting PRs, or cause of failure of CI or anything else. Code coverage is OK as a mild hint where your tests might be improved, but it quickly becomes a pernicious and counterproductive waste of time and destroyer of good code, if taken too seriously. And anything beyond gentle and ignorable hints is too serious. TLDR: people forced to jump through hoops to satisfy code coverage requirements are almost guaranteed to reduce the quality of code and tests, while giving a false sense of achievement and security. |
|
Thanks for the advice. I was trying to automate this process because it is extremely easy for untested code to go through unnoticed. I 100% agree that it should not be a measure of the quality of the test suite and I don't intend to use it as such. Admittedly, this PR conveys the opposite message. However, I want to help reviewers know if they have missed anything during code reviews. This would be just a tool to partially assess that. As you say, I went too far by setting it as a criterion for PR acceptance. After fighting with GHA, I didn't find a way of doing it such that the result is visible enough but doesn't create barriers. I will keep trying. |
|
Indeed, GHA has some very serious (and, in many cases, long-standing) shortcomings in these sorts of areas. It would be good to see the coverage report at the top level, without it blocking anything. It does not surprise me if you're struggling to achieve that. May The Force be with you. I'll try to look into myself it if I find some time. |
gonzaponte
force-pushed
the
coverage
branch
2 times, most recently
from
bb2acb2 to
078aedf
Compare
This is already done by the check_annotations decorator
If other nodes are present, these ones must too
This is not a matter of this function, but in general of the `check_annotations` decorator
This was shadowing another test with the same name
The refactor avoids creating if/else branches within a test function that might not be picked up by hypothesis. While there are still if/else branches, those are ensured to be picked up by construction.
For some reason this line shows up in the coverage report even though it is actually executed. Thus, we avoid it explicitly.
gonzaponte
force-pushed
the
coverage
branch
2 times, most recently
from
be6ed8d to
a1d0207
Compare
For now I keep the failure, to see how it works
Our test suite does not execute 100% of the code. Although 100% coverage does not guarantee good testing, it helps us detect untested parts of our code.
The most typical cases I've found are missing branches of
if/elseortry/exceptclauses. I've also noticed many tests that checked different things depending on the output; I've switched them to use different inputs instead. The bits of code that are not meant to be executed (for different reasons) are excluded explicitly.This PR rewrites some tests, adds others, removes deprecated stuff, and fixes various other things trying to achieve 100% code coverage. It also introduces a new step in the GitHub workflow to ensure 100% code coverage1. The idea is to detect untested areas of the code before merging to avoid surprises.
Note: the GHA should fail at the moment, as I've intentionally left one bit of code untested to demonstrate the idea. This will be addressed in a separate PR and when rebased, the GHA will give the green light.
Footnotes
The coverage report will only be executed if all tests pass. ↩