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

Change configs for source-base code coverage #708

Merged
merged 2 commits into from
Sep 2, 2022

Conversation

wszdexdrf
Copy link
Contributor

@wszdexdrf wszdexdrf commented Aug 9, 2022

Description

This also changes the code coverage front end to coveralls instead of codecov, which had some issues with other changes in the PR. This will provide better and more accurate code coverage reports.

Notes to the reviewers

The tests run before generating the report are not exhaustive (not exhaustive earlier too, but I added as many as I could), and hence the report won't be 100% accurate.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

Concept ACK.. I am wondering should we have separate code coverage job and redo all the tests or should we just create coverage report in the cont_integration jobs itself to get coverage for the full test suit..

Wondering what others thinks about this..

@wszdexdrf
Copy link
Contributor Author

create coverage report in the cont_integration jobs itself to get coverage for the full test suit..

I actually tried that, but I couldn't get the reports from the separate (parallel) tests to merge, even though it should be possible according to the documentation for coveralls. I will try it some more and see if it works.

@afilini
Copy link
Member

afilini commented Aug 12, 2022

Maybe you could also upload the coverage report as an artifact so that it can be manually downloaded and inspected with a different tool if coveralls isn't accurate.

We have some code to upload artifacts in the "build nightly docs" job, you can take it from there.

@rajarshimaitra
Copy link
Contributor

In the last dev call we converged on approach of

  • Update the existing coverage tool from codecov to better stuffs.
  • Make the coverage test as exhaustive as possible..

Copy link
Member

@afilini afilini left a comment

Choose a reason for hiding this comment

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

Just one last small comment, sorry

- id: coverage
name: Generate coverage
uses: actions-rs/grcov@v0.1.5
run: cargo test --features default,minimal,all-keys,compact_filters,key-value-db,compiler,sqlite,sqlite-bundled,test-electrum,verify,test-rpc
Copy link
Member

Choose a reason for hiding this comment

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

You can probably add test-esplora and test-md-docs as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't add test-esplora in the same command as esplora fails to compile alongside other features. On the other hand, codecov doesn't seem to like it if I try to upload more than one lcov file from two different test runs. (To be specific, codecov reports less total lines in project, indicative of the fact that features are disabled in the tests).

@afilini
Copy link
Member

afilini commented Aug 25, 2022

Sorry, I thought it was gonna be easier to generate an html report from the coverage file, but it actually depends on many files in your current environment. I would suggest also generating a report and uploading that as well for convenience. You can do that with the genhtml command

@wszdexdrf
Copy link
Contributor Author

In the last dev call we converged on approach of

* Update the existing coverage tool from codecov to better stuffs.

* Make the coverage test as exhaustive as possible..

The problem with codecov was that it reported many extra lines (which are not present in the source file) which results in lower % coverage. I found coveralls to be more accurate.

@notmandatory
Copy link
Member

The problem with codecov was that it reported many extra lines (which are not present in the source file) which results in lower % coverage. I found coveralls to be more accurate.

Yes I think this is what @rajarshimaitra meant, that's it's fine to switch to a new tool like grcov as you're doing.

@notmandatory
Copy link
Member

this fixes #698

@wszdexdrf wszdexdrf force-pushed the codecoverage branch 2 times, most recently from 98c6ca5 to 0143624 Compare August 27, 2022 17:16
@notmandatory
Copy link
Member

With this PR looks like codecov report shows 90.87% coverage 🥇!

https://app.codecov.io/gh/wszdexdrf/bdk

Old config only showing 77.21%:

https://app.codecov.io/gh/bitcoindevkit/bdk

@wszdexdrf
Copy link
Contributor Author

wszdexdrf commented Aug 27, 2022

With this PR looks like codecov report shows 90.87% coverage 🥇!

https://app.codecov.io/gh/wszdexdrf/bdk

Old config only showing 77.21%:

https://app.codecov.io/gh/bitcoindevkit/bdk

I hate to say this, but that's not entirely correct. That was an older report I had generated while testing. That report has lesser number of total lines and hence it has higher percentage of coverage. The current version is
https://coveralls.io/github/wszdexdrf/bdk
Which is 79.18 % (81% if I can get it to remove examples from the report) :(

One good thing to note will be that even though the percentage is relatively unchanged, the number of tracked lines is almost doubled, so I think this should provide better tracking. And also, directly fixes the issue that some modules weren't being tracked...

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 0143624

Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

ACK 0143624

Whatever it is, its better than before.. So LFG!!

Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

utACK 0143624

@danielabrozzoni
Copy link
Member

Oh, I just noticed... You should change the badge in the README not to point to codecov anymore, but to point to coveralls

Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

Sorry wrong review..

The problem seems to be there still..

Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

ReACK 28dab48

@notmandatory
Copy link
Member

I just noticed on coveralls that it doesn't show the source code so there's no way to see what lines are covered or not, is there something you need to enable to get coveralls to show this info?

https://coveralls.io/builds/52019084/source?filename=src%2Fpsbt%2Fmod.rs

@wszdexdrf
Copy link
Contributor Author

I just noticed on coveralls that it doesn't show the source code so there's no way to see what lines are covered or not, is there something you need to enable to get coveralls to show this info?

https://coveralls.io/builds/52019084/source?filename=src%2Fpsbt%2Fmod.rs

That's weird. It does show the code for me...

Edit: I found this. I guess we could use the HTML report for line by line coverage. I am waiting for any idea about how to proceed now.

@notmandatory
Copy link
Member

Edit: I found this. I guess we could use the HTML report for line by line coverage. I am waiting for any idea about how to proceed now.

Ahh! OK I can confirm that once I login to coveralls with my github account I'm able to see the code and coverage details. As mentioned in the issue you found this is annoying, but I wouldn't say it's a show stopper.

@notmandatory
Copy link
Member

@wszdexdrf looks like this needs another re-base to pickup new CI tests.

Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

ACK 0010ecd

@afilini afilini merged commit 06310f1 into bitcoindevkit:master Sep 2, 2022
@afilini afilini mentioned this pull request Oct 7, 2022
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants