search

fedimint / fedimint

Federated E-Cash Mint
https://fedimint.org/
MIT License
536 stars 209 forks source link

coverage and test can be one step #383

Closed NicolaLS closed 1 year ago

NicolaLS commented 1 year ago

testing CI

dpc commented 1 year ago

Code coverage seems to generate a whole new ./target/ccov-llvm/ rebuilding the whole project. Please verify, just in case I got confused.

For this reason it is better to run it as a separate job, so it can do the whole different build in parallel since it can't really re-use anything.

NicolaLS commented 1 year ago

Code coverage seems to generate a whole new ./target/ccov-llvm/ rebuilding the whole project. Please verify, just in case I got confused.

For this reason it is better to run it as a separate job, so it can do the whole different build in parallel since it can't really re-use anything.

mhm yes it does, but in the dev meeting today we noticed that we don't have to use nightly (if we ditch --all-features) and also since the coverage tool basically just runs cargo test we could unify the test/coverage ?

codecov-commenter commented 1 year ago

Codecov Report

Merging #383 (9f27628) into master (66519d5) will decrease coverage by 0.05%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #383      +/-   ##
==========================================
- Coverage   75.29%   75.23%   -0.06%     
==========================================
  Files          83       83              
  Lines       10391    10407      +16     
==========================================
+ Hits         7824     7830       +6     
- Misses       2567     2577      +10
Impacted Files Coverage Δ
client/clientd/src/bin/clientd-cli.rs 3.84% <0.00%> (-0.92%) :arrow_down:
client/clientd/src/main.rs 1.42% <0.00%> (-0.22%) :arrow_down:
minimint/src/bin/minimint.rs 2.85% <0.00%> (-0.09%) :arrow_down:
client/clientd/src/lib.rs 2.22% <0.00%> (-0.06%) :arrow_down:
minimint/src/net/connect.rs 95.20% <0.00%> (+0.25%) :arrow_up:
minimint/src/net/peers.rs 93.42% <0.00%> (+0.52%) :arrow_up:
client/client-lib/src/api.rs 87.30% <0.00%> (+1.15%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

NicolaLS commented 1 year ago

Code coverage seems to generate a whole new ./target/ccov-llvm/ rebuilding the whole project. Please verify, just in case I got confused.

For this reason it is better to run it as a separate job, so it can do the whole different build in parallel since it can't really re-use anything.

so I guess in the CI now we build once "normal" and use it for a variety of tests, and also build the coverage separate. So now this would not change (build everything twice) but we would at least not run the whole Unit Tests twice which safes a lot of time..

dpc commented 1 year ago

and also since the coverage tool basically just runs cargo test we could unify the test/coverage

If that's the case the tests could just run only in the code-coverage job, and not in the normal build?

we don't have to use nightly (if we ditch --all-features)

Oh. Seems like cargo-udeps requires nightly too, so I thought it's simplest to just stick with nightly in the CI. But if you prefer stable, we can kick cargo-udeps completely (I don't think it is all that worthwhile) or move it back to a one-off non-flake pipeline.

NicolaLS commented 1 year ago

and also since the coverage tool basically just runs cargo test we could unify the test/coverage

If that's the case the tests could just run only in the code-coverage job, and not in the normal build?

we don't have to use nightly (if we ditch --all-features)

Oh. Seems like cargo-udeps requires nightly too, so I thought it's simplest to just stick with nightly in the CI. But if you prefer stable, we can kick cargo-udeps completely (I don't think it is all that worthwhile) or move it back to a one-off non-flake pipeline.

Yeah I tend to just dropping udeps its a nice thing to have but its not really worth it. maybe we can have a before-pr.sh shell script which runs stuff like that and encourage devs to run that before submitting a PR. What do you think @elsirion @justinmoon

dpc commented 1 year ago

maybe we can have a before-pr.sh shell script which runs stuff like that and encourage devs to run that before submitting a PR.

Since it invalidates the ./target I wouldn't even bother devs about it (BTW. there are already ./misc/git hooks where you can add checks like this).

I think it would be best run it a workflow that executes once a day, and that's about it. If something gets introduced it will just get fixed.

justinmoon commented 1 year ago
dpc commented 1 year ago

I don't think cargo udeps is worth maintaining a whole new toolchain. We can check for unused dependencies by hand, too. It's not a big deal.

Already moved to a separate once a day workflow.

Hopefully such a report could be generated in the background as we're running our normal CI jobs ...

This might be possible, I haven't reserached it . Though it shouldn't take much more (if at all) than the main build.

Also we an removed integration test from main build job, and save some time, by reusing CC job, so maybe it is actually saving us some real time.

justinmoon commented 1 year ago

Though it shouldn't take much more (if at all) than the main build.

Understood. We'd still be able to save ~50% on the buildjet build if we build half as much. But that's more of a nice-to-have I guess. Developer time more valuable ...

NicolaLS commented 1 year ago

Hey @dpc would you mind taking the lead on this ? I feel very clumsy doing the nix/yml/CI stuff. So right now we have two parallel jobs build/test and coverage but since the coverage basically runs cargo test we want to unify that. I guess this PR goes in the right direction ? (messed up the rebase thats why its failing now). If you want to do it I'll just close this PR

dpc commented 1 year ago

Sure. Will do .

NicolaLS commented 1 year ago

perfect thank you @dpc