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

Add the codecov badge #1445

Merged
merged 1 commit into from
Apr 15, 2020
Merged

Add the codecov badge #1445

merged 1 commit into from
Apr 15, 2020

Conversation

sylvestre
Copy link
Sponsor Contributor

No description provided.

@sylvestre sylvestre requested a review from rivy April 8, 2020 17:05
@rivy
Copy link
Member

rivy commented Apr 8, 2020

I'm for this ... but, code coverage calculation is, at the moment, sub-par.

I'm experimenting with grcov to improve cross-platform coverage, but I'm stuck trying to get coverage for the workspace packages. Right now, only uutils gets any coverage information.

I think I'm going to have to include the workspace packages as modules to get coverage information generated for them. Any ideas?

@sylvestre
Copy link
Sponsor Contributor Author

@rivy my team at Mozilla developed grcov. If you have more details, they can probably help :)

@rivy
Copy link
Member

rivy commented Apr 9, 2020

I've got code coverage building and sending coverage reports for macos, windows, and unix platforms (see https://github.com/rivy/rs.coreutils/runs/572806040?check_suite_focus=true; built from f695cdc). But, it's only generating results for uutils (the [[bin]] for the crate).

I believe than the various utility packages in the workspace will have to somehow be pulled in as modules before any code coverage can be accomplished on them. I'm not sure how to do that and preserve the current build framework.

@rivy
Copy link
Member

rivy commented Apr 10, 2020

After hacking on this for a day or so, I've discovered that the coverage instrumentation needs source files to be in the src directory (even for the workspace sub-packages) in order to actually evaluate the lines. I don't know whether it's the compiler instrumentation or grcov that has the problem, but at least it's a workable solution.

I'm, currently, in the midst of refactoring all the sub-package files to be in their respective src directories onto a test branch. I'll keep you updated on the progress and hopefully success of this change. Timeline is probably towards the end of the weekend.
🤞

@rivy
Copy link
Member

rivy commented Apr 11, 2020

After a reorganization of the source files into more "standard" locations and hacking the coverage filenames, I'm getting an improved coverage report.

original @ https://codecov.io/gh/rivy/rs.coreutils/tree/7c0132354576701771dd29bad404b4106a50669b
re-org'd @ https://codecov.io/gh/rivy/rs.coreutils/tree/528d14212cf83542bb623c5011bc37ee4ba1bf2c

Would you mind taking a look and commenting?

@rivy
Copy link
Member

rivy commented Apr 12, 2020

Argh! That was a day-long hack task!
But I finally have reasonable, working, code coverage branch.
🎉

Take a look at rivy:rust.coreutils/rf.reorg-codecover and the corresponding CodeCov report.

My plan is to release a PR for it Monday right after I merge #1449.
We can then merge it and this on top of it.

Sound good?

@sylvestre
Copy link
Sponsor Contributor Author

this is great, bravo :)

I would ignore tests/ as it isn't super interesting info.

@rivy
Copy link
Member

rivy commented Apr 12, 2020

I was going to take out the "tests" directory, but then I found a bug by reviewing the uncovered lines. The uncovered section shown at https://codecov.io/gh/rivy/rs.coreutils/src/7f81bf3de1a2df3362f9fe2506ad7d00e8b7feb3/tests/test_du.rs#L16...32 points out that test_du_basics() is actually failing to call _du_basics(). So, I think it could be useful.

grcov is close to merging a PR that can exclude line or branch coverage for specific lines/sections (see mozilla/grcov#416). When that merges, I should be able to ignore the internal branch coverage errors in the assert functions, and we could shoot for 100% coverage in tests. Once achieved, anything less could be seen as a testing bug.

@sylvestre
Copy link
Sponsor Contributor Author

55% coverage, well done :)

@sylvestre sylvestre merged commit 4845ac7 into uutils:master Apr 15, 2020
@rivy
Copy link
Member

rivy commented Apr 15, 2020

👍

I'm working on a quick PR to condense redundant CI testing on AppVeyor and Travis to reduce the total time to success/failure for a given commit. It should come down to a total of about 15 minutes (median) time to success/fail (from the median of about an hour that it's taking now).

I'm also attempting a CodeCov configuration change to make it informational (ie, never fail the build).

@rivy
Copy link
Member

rivy commented Apr 15, 2020

I added the commits for condensed CI and fixed CodeCov to my already open PR #1480.

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.

2 participants