Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix generation of coverage for c++ code #1617

Merged
merged 1 commit into from Feb 13, 2017
Merged

Fix generation of coverage for c++ code #1617

merged 1 commit into from Feb 13, 2017

Conversation

betatim
Copy link
Member

@betatim betatim commented Feb 13, 2017

Addresses #1511 (comment) where @ctb noticed that we do not generate coverage for c++ code anymore.

  • Is it mergeable?
  • make test Did it pass the tests?

@codecov-io
Copy link

Codecov Report

Merging #1617 into master will decrease coverage by -25.23%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #1617       +/-   ##
===========================================
- Coverage   95.31%   70.09%   -25.23%     
===========================================
  Files          36       66       +30     
  Lines        2969     8902     +5933     
  Branches      445     3008     +2563     
===========================================
+ Hits         2830     6240     +3410     
- Misses         67     1041      +974     
- Partials       72     1621     +1549
Impacted Files Coverage Δ
lib/hllcounter.hh 100% <ø> (ø)
lib/traversal.hh 71.42% <ø> (ø)
lib/kmer_hash.cc 86.59% <ø> (ø)
lib/storage.hh 89.69% <ø> (ø)
lib/read_parsers.hh 65.62% <ø> (ø)
khmer/_cpy_nodetable.hh 45% <ø> (ø)
lib/labelhash.cc 52.47% <ø> (ø)
khmer/_cpy_smallcounttable.hh 45% <ø> (ø)
lib/read_aligner.hh 100% <ø> (ø)
lib/assembler.cc 52.87% <ø> (ø)
... and 20 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 42e3f88...bef16bb. Read the comment docs.

@ctb
Copy link
Member

ctb commented Feb 13, 2017

Well, something's working, although it looks a bit weird :).

Copy link
Member

@ctb ctb left a comment

Choose a reason for hiding this comment

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

I can only assume this is part of @betatim's master plan.

@betatim
Copy link
Member Author

betatim commented Feb 13, 2017

So it seems like make coverage-debug thinks there is nothing to be done after make test has been run. However without recompiling with -fprofile-arcs -ftest-coverage etc you don't generate the files that gcovr needs to calculate the coverage.

Compare for example: https://codecov.io/gh/dib-lab/khmer/tree/32fcf25137937de2dc52d214724f87312434c51a and https://codecov.io/gh/dib-lab/khmer/pull/1617/tree one of them ignores all the files that aren't .py.

@@ -136,6 +136,7 @@ clean: FORCE
rm -f diff-cover.html
rm -Rf build dist
rm -rf __pycache__/ .eggs/ khmer.egg-info/
-rm *.gcov
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a better way to pattern match these files that has less potential for accidentally deleting stuff that it shouldn't at some point in the future?

Copy link
Member

Choose a reason for hiding this comment

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

The risk for inadvertently destroying unrelated files is extremely low.

@betatim
Copy link
Member Author

betatim commented Feb 13, 2017

If someone can think of a nicer rm *gcov pattern I'll do that, and if a second pair of eyes and brains could think about if there is a nicer way of doing this that would also be good. Otherwise I think we should merge. And then spend our time trying to dig ourselves out of this loss of coverage :(

@standage standage merged commit 4f1f0cc into master Feb 13, 2017
@standage standage deleted the fix/gcovr branch February 13, 2017 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants