Include functional tests in code coverage #899
Conversation
Use the default coverage report file `.coverage` for unit and functional tests, running Augur in functional tests through an alias that runs coverage in append mode. Then, use the coverage xml command to generate the XML report needed by the codecov tool.
Codecov Report
@@ Coverage Diff @@
## master #899 +/- ##
===========================================
+ Coverage 34.62% 59.51% +24.89%
===========================================
Files 42 43 +1
Lines 6007 6062 +55
Branches 1538 1557 +19
===========================================
+ Hits 2080 3608 +1528
+ Misses 3854 2199 -1655
- Partials 73 255 +182
Continue to review full report at Codecov.
|
tests/builds/zika.t
Outdated
| @@ -7,7 +7,8 @@ Running from the test data directory allows us to use relative paths that won't | |||
| $ TEST_DATA_DIR="$TESTDIR/zika" | |||
| $ mkdir -p "$TMP/out" | |||
| $ pushd "$TEST_DATA_DIR" > /dev/null | |||
| $ export AUGUR="../../../bin/augur" | |||
| $ export COVERAGE_FILE="$TESTDIR/../../.coverage" | |||
| $ export AUGUR="coverage run -a --rcfile=$TESTDIR/../../.coveragerc ../../../bin/augur" | |||
There was a problem hiding this comment.
Instead of defining AUGUR this way with coverage run … in every cram test file, can we instead run cram itself under coverage instead in ci.yaml?
This would avoid the increased boilerplate in each cram test file but also mean that coverage isn't observed for local cram runs unless someone opts into it.
There was a problem hiding this comment.
I tried this originally, but I found it only produced coverage of cram itself. It's likely I was not invoking coverage properly though. I tried something like this first:
coverage run \
--rcfile=.coveragerc \
`which cram` \
--shell=/bin/bash \
tests/functional/parse.t tests/functional/ancestral.t
And then coverage report produces:
Name Stmts Miss Branch BrPart Cover
-------------------------------------------------------------------------------------
/Users/jlhudd/miniconda3/envs/nextstrain/bin/cram 6 1 0 0 83%
-------------------------------------------------------------------------------------
TOTAL 6 1 0 0 83%
I also tried forcing a report on augur alone by adding --source augur to the coverage command above, but this produced a warning of Coverage.py warning: No data was collected. (no-data-collected).
Another way we could remove some of the above boilerplate code would be to export absolute paths to COVERAGE_FILE and COVERAGE_RCFILE outside of Cram and then we'd be back to one line like this which isn't too bad:
$ export AUGUR="coverage run -a ../../../bin/augur"
There was a problem hiding this comment.
Hmm. Reducing boilerplate is nice, and +1 for pulling out the coverage args into env vars set outside of cram, but my main point is to not to bake in coverage to the tests but have it be a separate concern (e.g. CI's concern).
If AUGUR was defined in the cram tests as:
export AUGUR="${AUGUR:-../../bin/augur}"
then it could be overridden by ci.yaml to include coverage run … when invoking cram.
If you'd rather not go down this path, that's fine. This isn't a merge-blocking suggestion. I bring this up only because in my experience it's better to have coverage tracing happen outside of tests (i.e. the tests are unaware) rather than as integrated into the tests themselves, so this stood out.
There was a problem hiding this comment.
Ah, I totally agree! I just couldn't figure out how to actually accomplish that separation of concerns. That's part of why I asked for @victorlin's help here, but happy to have any other help we can get!
There was a problem hiding this comment.
Ok! Since I had already thought about this above, I went ahead and pushed a commit to separate these out. We'll see if it works…
There was a problem hiding this comment.
I don't think I would look for version-specific code coverage, though.
Nod. It's less about looking for it explicitly and more that if we don't test version-specific code paths than coverage has some upper limit that's not 100%. :-)
There was a problem hiding this comment.
Not sure. Nothing obvious from a simple grep on sys.version_info, though that's not comprehensive. But probably not any now, and not very likely in the future (though not unreasonable)?
There was a problem hiding this comment.
@victorlin Do you want to apply the diff above before we merge? I don't feel strongly either way...
There was a problem hiding this comment.
@huddlej I'll put it in a new PR since this one looks good, and we can compare the CI differences a bit easier that way.
…I workflow This avoids baking in coverage to the tests and lets it remain a separate concern (e.g. CI's concern). In my experience it's better to have coverage tracing happen outside of tests (i.e. the tests are unaware) rather than as integrated into the tests themselves. Partially reverts commit 723d609, which added code for coverage to each cram test file. I chose to use the github.workspace context var in the CI workflow so I could use the "env:" block for the job step. Alternatively, we could use `realpath` to construct an `env AUGUR=… …` call chaining to the `cram …` invocation, but that seemed slightly messier to me.
|
@huddlej good to merge based on Slack discussion? |
Description of proposed changes
Use the default coverage report file
.coveragefor unit and functionaltests, running Augur in functional tests through an alias that runs
coverage in append mode. Then, use the coverage xml command to generate
the XML report needed by the codecov tool.
Related issues
Fixes #688