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

Overhaul coverage setup #2202

Merged
merged 4 commits into from
Dec 18, 2018
Merged

Overhaul coverage setup #2202

merged 4 commits into from
Dec 18, 2018

Conversation

mvines
Copy link
Member

@mvines mvines commented Dec 17, 2018

This PR gets coverage working again. Humans may run ./scripts/coverage.sh locally to generate a report.

CI runs ./scripts/coverage.sh on PRs and Pushes if any .rs files are affected. Results are attached to the buildkite job and uploaded to codecov.io.

cargo-cov usage has been removed, it's overly complex and unnecessary.

Note that in the buildkite job for this PR, there's a message at the top telling saying "Coverage skipped as no .rs files were modified" -- we skip coverage if the PR has no .rs file changes to help with CI load. Contrast that with the demo PR at #2211.

Fixes #433

@mvines mvines added the work in progress This isn't quite right yet label Dec 17, 2018
@mvines mvines mentioned this pull request Dec 17, 2018
@mvines mvines added the noCI Suppress CI on this Pull Request label Dec 17, 2018
@mvines mvines force-pushed the grcov branch 4 times, most recently from 42797f6 to 8727dc1 Compare December 18, 2018 01:29
@mvines mvines added CI Pull Request is ready to enter CI and removed noCI Suppress CI on this Pull Request labels Dec 18, 2018
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Dec 18, 2018
@mvines mvines force-pushed the grcov branch 2 times, most recently from fc5854c to 256c815 Compare December 18, 2018 02:54
@mvines
Copy link
Member Author

mvines commented Dec 18, 2018

This all works well on macOS, but blows up on Linux still. 🐧

@mvines
Copy link
Member Author

mvines commented Dec 18, 2018

The problem on linux appears to be that the gnco files from the different crates all end up getting written to the same build.gcno/lib.gcno file in the root of the workspace wheres on macOS each crate gets it's own unique set of gcno`s. Looks like it's time to download the cargo-cov source and figure out what's going on

@mvines mvines force-pushed the grcov branch 7 times, most recently from e56d93a to f73c3f1 Compare December 18, 2018 09:21
@mvines
Copy link
Member Author

mvines commented Dec 18, 2018

cargo-cov appears to be a dead end and even somewhat harmful. Getting really close to coverage working with plain old cargo nestled in a cozy, yet light, blanket of bash.

@mvines mvines force-pushed the grcov branch 5 times, most recently from 534bed3 to b348a13 Compare December 18, 2018 16:48
@solana-labs solana-labs deleted a comment from codecov-io Dec 18, 2018
@solana-labs solana-labs deleted a comment from codecov bot Dec 18, 2018
@solana-labs solana-labs deleted a comment from codecov bot Dec 18, 2018
@mvines mvines changed the title Upgrade grcov v2 Overhaul coverage setup Dec 18, 2018
@mvines mvines removed the work in progress This isn't quite right yet label Dec 18, 2018
@solana-labs solana-labs deleted a comment from codecov bot Dec 18, 2018
@mvines mvines requested a review from garious December 18, 2018 18:25
@solana-labs solana-labs deleted a comment from codecov bot Dec 18, 2018
@codecov
Copy link

codecov bot commented Dec 18, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@e720070). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2202   +/-   ##
=========================================
  Coverage          ?   56.56%           
=========================================
  Files             ?      109           
  Lines             ?     9047           
  Branches          ?        0           
=========================================
  Hits              ?     5117           
  Misses            ?     3930           
  Partials          ?        0
Impacted Files Coverage Δ
src/tpu_forwarder.rs 30.98% <ø> (ø)

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 e720070...e65c9d9. Read the comment docs.

Copy link
Contributor

@garious garious left a comment

Choose a reason for hiding this comment

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

Oh yeah!

@mvines mvines merged commit 3bfb052 into solana-labs:master Dec 18, 2018
@mvines mvines deleted the grcov branch December 18, 2018 18:48
echo "--- grcov"
./grcov target/cov/debug/deps/ > target/cov/lcov_full.info

echo "--- filter_non_local_files_from_lcov"
Copy link

Choose a reason for hiding this comment

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

You should be able to use the -s option of grcov to achieve the same result (ignore any file which is not in the repository)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh sweet, thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

💔

$ ./grcov target/cov/debug/deps/ -s . > out.lcov
thread 'main' panicked at 'Only one file in the repository should end with src/packet.rs (sdk/src/packet.rs and src/packet.rs both end with that)', src/path_rewriting.rs:143:13
note: Run with `RUST_BACKTRACE=1` for a backtrace.

Copy link

Choose a reason for hiding this comment

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

I'll fix this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome, thanks! If you can ping me here I'll be able to give it a test drive when ready

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @marco-c, what's generally the best way to contact you? Care to join our Discord forum? https://discord.gg/k8Kq58

behzadnouri pushed a commit to behzadnouri/solana that referenced this pull request Nov 6, 2024
* Adds the feature.

* Adds the feature gate logic.

* Adjusts tests.
Lichtso added a commit to Lichtso/solana that referenced this pull request Nov 7, 2024
* Adds the feature.

* Adds the feature gate logic.

* Adjusts tests.
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.

4 participants