Skip to content

Conversation

dwhswenson
Copy link
Member

Back in March, I saw a tweet from Ned Batchelder linking to this:

https://nedbatchelder.com/blog/202008/you_should_include_your_tests_in_coverage.html

Since then, I've been considering including test suites in coverage. This PR adds the test suite to the coverage data. Consider this a proposal, because I'm not necessarily wedded to the idea we have to do this. But please read Ned's blog post, and consider if doing this affects your workflow in any way.

My own thoughts on the concerns Ned's post mentions:

  • It skews my results: This is part of why I don't do it on the OPS library (because we should be ashamed of low coverage! 80% is shame in a way 90% is not!) However, on packages that already have 100% coverage, there's nothing to skew -- so I'm in favor of including tests in coverage here.
  • It clutters the output: I mainly look at coverage via CodeCov's web interface now (or, locally, I restrict coverage output to a certain subset of files). In CodeCov, it's easy enough for the get past the whole tests/ directory either in sunburst or in file browser mode (the two I typically use).
  • I don't intend to run all the tests: I don't see this as an issue. When running coverage locally, it's easy enough for my to only look at coverage of the files I'm interested in. When working on a subpackage (e.g., the wizard) I check coverage locally by only running the tests for that package and only generating coverage for that package (pytest --cov=paths_cli/wizard paths_cli/tests/wizard/).

The only other issue I see is that I consider "non-test statements" to be a good figure for a minimal lines-of-code estimate. This number is trivial to get from the current setup. It's a little harder to get if test are included in coverage (need to click on the tests/ directory in the file browser view of CodeCov, then at the bottom do the math on "Project totals - Folder totals"). I think Ned's arguments in favor including tests in coverage outweigh this minor inconvenience. It looks like this points out a few corners of code that are missing coverage, so it might be worth it. (There are also areas that probably can be ignored in coverage, like RuntimeError('pytest went crazy')).

If there's general support for this approach, I'll move forward with bringing this PR back up to 100% coverage.

@codecov
Copy link

codecov bot commented Nov 12, 2021

Codecov Report

Merging #69 (f0d946b) into main (f9cd13d) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##              main       #69     +/-   ##
===========================================
  Coverage   100.00%   100.00%             
===========================================
  Files           58       110     +52     
  Lines         2314      5415   +3101     
===========================================
+ Hits          2314      5415   +3101     
Impacted Files Coverage Δ
paths_cli/compiling/core.py 100.00% <ø> (ø)
paths_cli/tests/commands/test_md.py 100.00% <ø> (ø)
paths_cli/tests/commands/utils.py 100.00% <ø> (ø)
paths_cli/tests/compiling/test_core.py 100.00% <ø> (ø)
paths_cli/tests/test_file_copying.py 100.00% <ø> (ø)
paths_cli/tests/test_utils.py 100.00% <ø> (ø)
paths_cli/tests/utils.py 100.00% <ø> (ø)
paths_cli/tests/wizard/mock_wizard.py 100.00% <ø> (ø)
paths_cli/tests/wizard/test_load_from_ops.py 100.00% <ø> (ø)
paths_cli/tests/wizard/test_parameters.py 100.00% <ø> (ø)
... and 56 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9cd13d...f0d946b. Read the comment docs.

@sroet
Copy link
Member

sroet commented Nov 12, 2021

If there's general support for this approach, I'll move forward with bringing this PR back up to 100% coverage.

I am in favor of coverage for the tests. It would not be the first time I copied a working test and forgot to change the name

@dwhswenson
Copy link
Member Author

Success! We now have 100% coverage, including all 3101 lines of tests.

This was very much a worthwhile exercise. There were pieces of functionality that were not being tested, generally due to tiny errors in the test suite (in one case, a parametrize used the same parameter twice; in another case, a test checked for a parameter string "named" instead of "name"). I'd been thinking about doing this based on the principle of the thing, but now I'd say it has a lot of value in practice, too.

Ready for review.

Copy link
Member

@sroet sroet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, feel free to merge

@dwhswenson dwhswenson merged commit 600d361 into openpathsampling:main Dec 10, 2021
@dwhswenson dwhswenson deleted the include-tests-coverage branch December 10, 2021 15:33
@dwhswenson dwhswenson mentioned this pull request Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants