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.

Sign up for GitHub

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

Jump to bottom

Add coverage workflow #1217

Merged
merged 3 commits into from
Mar 15, 2024
Merged

Add coverage workflow #1217

merged 3 commits into from
Mar 15, 2024

Conversation

tbro
Copy link
Contributor

@tbro tbro commented Mar 8, 2024 •
edited
Loading

Adds coverage.yaml and ads coverage shell to flake. also addes code-coverage command to justfile.

Closes #683

@tbro tbro mentioned this pull request Mar 8, 2024
Closed
sveitser
sveitser reviewed Mar 11, 2024
View reviewed changes
justfile Outdated Show resolved Hide resolved
@sveitser
Copy link
Collaborator

I think we can definitely not run this on every PR. I'm trying to run it locally but it's taking ages.

For the CI to work we need to either run it with nix (then you get all the dependencies) or install some missing dependencies (at least foundry).

@sveitser
Copy link
Collaborator

sveitser commented Mar 11, 2024 •
edited
Loading

I have these two tests failing locally

failures:
    api::fs::generic_tests::state_signature_test_with_query_module
    api::sql::generic_tests::state_signature_test_with_query_module

test result: FAILED. 47 passed; 2 failed; 2 ignored; 0 measured; 0 filtered out; finished in 991.56s

Fails at

The application panicked (crashed).
Message:  assertion failed: client.get::<StateSignatureRequestBody>(&format!("state-signature/block/{}",
                        height)).send().await.is_ok()
Location: sequencer/src/api.rs:273

If we're lucky it's just a timing issue and we can retry.

@sveitser
Copy link
Collaborator

If I run them individually with this coverage configuration they both pass relatively quickly (about 10s) but running together they take 100 seconds. 🤔

@sveitser
Copy link
Collaborator

Hotshot actually runs the coverage workflow with tokio, not async_std.

@tbro
Copy link
Contributor Author

tbro commented Mar 11, 2024

Hotshot actually runs the coverage workflow with tokio, not async_std.

Yes, I've tried both w/out success. I'm just trying things 🤞

sveitser
sveitser reviewed Mar 13, 2024
View reviewed changes
justfile Outdated Show resolved Hide resolved
@tbro tbro marked this pull request as ready for review March 13, 2024 18:40
sveitser
sveitser reviewed Mar 14, 2024
View reviewed changes
justfile Outdated Show resolved Hide resolved
sveitser
sveitser reviewed Mar 14, 2024
View reviewed changes
flake.nix Outdated Show resolved Hide resolved
@sveitser
Copy link
Collaborator

@tbro roughly how long does it take for you when you run this locally? The test snark::tests::test_proof_generation has been running for multiple minutes.

sveitser
sveitser reviewed Mar 14, 2024
View reviewed changes
flake.nix Outdated
@@ -253,6 +254,28 @@
];
inherit RUST_LOG RUST_BACKTRACE RUSTFLAGS CARGO_TARGET_DIR;
};
devShells.coverage =
let
toolchain = pkgs.rust-bin.nightly.latest.minimal.override {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be the same as just pkgs.rust-bin.nightly.latest.minimal without override.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 5011948

sveitser
sveitser reviewed Mar 14, 2024
View reviewed changes
flake.nix Show resolved Hide resolved
sveitser
sveitser reviewed Mar 14, 2024
View reviewed changes
justfile Outdated
echo "Running code coverage"
nix develop .#coverage -c cargo test --all-features --no-fail-fast --release --workspace -- --skip service::test::test_
grcov . -s . --binary-path $CARGO_TARGET_DIR/debug/ -t html --branch --ignore-not-existing -o $CARGO_TARGET_DIR/coverage/
open $CARGO_TARGET_DIR/coverage/index.html
Copy link
Collaborator

Choose a reason for hiding this comment

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

I only have xdg-open 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shoot I thought I had found a command that would work across linux/mac. In that case I think its probably better to just output a message containing the location of the HTML, and the caller can open it as they like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 407d66b

@sveitser
Copy link
Collaborator

Hopefully last request. Can we add a coverage badge to the readme?

@sveitser
Copy link
Collaborator 

[![Coverage Status](https://coveralls.io/repos/github/EspressoSystems/espresso-sequencer/badge.svg?branch=main)](https://coveralls.io/github/EspressoSystems/espresso-sequencer?branch=main)

Should do the trick

@tbro
Copy link
Contributor Author

tbro commented Mar 14, 2024

@tbro roughly how long does it take for you when you run this locally? The test snark::tests::test_proof_generation has been running for multiple minutes.

I haven't timed it yet, but it does complete which is more than it used to do. The proof_generation test takes a long time, but it does eventually succeed.

@tbro
Copy link
Contributor Author

tbro commented Mar 14, 2024

@tbro roughly how long does it take for you when you run this locally? The test snark::tests::test_proof_generation has been running for multiple minutes.

real 22m7.089s
user 298m53.894s
sys 4m46.806s

@tbro tbro enabled auto-merge (squash) March 14, 2024 18:28
@tbro tbro requested a review from sveitser March 14, 2024 19:11
@tbro
Copy link
Contributor Author

tbro commented Mar 14, 2024 

[![Coverage Status](https://coveralls.io/repos/github/EspressoSystems/espresso-sequencer/badge.svg?branch=main)](https://coveralls.io/github/EspressoSystems/espresso-sequencer?branch=main)

Should do the trick

added the badge in 49cafc3

@tbro
Add code-coverage
e6eccc3 
Adds coverage.yaml and ads coverage shell to flake. also adds
code-coverage command to justfile.

NOTE: for rational of `-Cdebuginfo=2` see:
rust-lang/rust#72974

Closes #683
@sveitser
coverage: ignore contract-bindings and contracts
c1fa717 
The bindings files are auto generated and the contracts directory only
contains rust code to test the smart contracts.
@sveitser
Undo comment
3f5beae
@sveitser
Copy link
Collaborator

@tbro I added ignoring of files in contract-bindings and contracts.

sveitser
sveitser approved these changes Mar 15, 2024
View reviewed changes
Copy link
Collaborator

@sveitser sveitser left a comment

Choose a reason for hiding this comment

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

LGTM 🥳

@tbro tbro merged commit 7132d3f into main Mar 15, 2024
14 checks passed
@tbro tbro deleted the chore/add-llvm-cov branch March 15, 2024 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sveitser sveitser sveitser approved these changes

@nomaxg nomaxg Awaiting requested review from nomaxg

@philippecamacho philippecamacho Awaiting requested review from philippecamacho philippecamacho is a code owner

@ImJeremyHe ImJeremyHe Awaiting requested review from ImJeremyHe ImJeremyHe is a code owner

@jbearer jbearer Awaiting requested review from jbearer jbearer is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add code coverage and run it on CI
2 participants
@tbro @sveitser