librustzcash icon indicating copy to clipboard operation
librustzcash copied to clipboard

CI: Run tests with cargo-nextest

Open str4d opened this issue 3 years ago • 4 comments

str4d avatar Feb 15 '22 03:02 str4d

Codecov Report

Base: 50.42% // Head: 50.80% // Increases project coverage by +0.38% :tada:

Coverage data is based on head (e778dc8) compared to base (67cb63a). Patch coverage: 49.24% of modified lines in pull request are covered.

Additional details and impacted files
@@                              Coverage Diff                               @@
##           non-consensus-changes-on-branchid-37519621     #512      +/-   ##
==============================================================================
+ Coverage                                       50.42%   50.80%   +0.38%     
==============================================================================
  Files                                              90       97       +7     
  Lines                                            8306     9522    +1216     
==============================================================================
+ Hits                                             4188     4838     +650     
- Misses                                           4118     4684     +566     
Impacted Files Coverage Δ
components/f4jumble/src/lib.rs 77.77% <ø> (ø)
components/zcash_address/src/convert.rs 4.25% <0.00%> (-2.00%) :arrow_down:
components/zcash_address/src/kind/unified.rs 38.80% <ø> (+0.82%) :arrow_up:
zcash_client_backend/src/data_api.rs 5.88% <ø> (ø)
zcash_client_backend/src/data_api/error.rs 20.51% <0.00%> (-2.35%) :arrow_down:
zcash_client_backend/src/data_api/wallet.rs 17.07% <0.00%> (-4.85%) :arrow_down:
zcash_client_backend/src/decrypt.rs 0.00% <0.00%> (ø)
zcash_client_sqlite/src/error.rs 23.52% <0.00%> (+2.10%) :arrow_up:
zcash_extensions/src/transparent/demo.rs 49.71% <ø> (+4.31%) :arrow_up:
zcash_history/src/lib.rs 0.00% <ø> (ø)
... and 126 more

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

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Feb 15 '22 04:02 codecov[bot]

Hmm, this actually ends up taking longer, because for some reason the sequence of compiles it triggers takes longer overall, plus we have several minutes compiling and installing cargo-nextest itself. So I'm putting this on the backburner until we can figure out build efficiencies.

str4d avatar Feb 17 '22 01:02 str4d

BTW nextest is now available as pre-built binaries, if you're interested in using them: https://nexte.st/book/pre-built-binaries

sunshowers avatar Feb 20 '22 20:02 sunshowers

(Below I ignore the 2m 50s for building the param-fetching binary, which is unchanged)

Without this PR:

  • Run tests and doc tests (7m 34s)
    • 6m 06s building, 1m 28s testing.
  • Run slow tests (1m 41s)
    • 0.15s building, so basically all testing.
  • Total build time: 6m 06s
  • Total test time: 3m 09s

With this PR:

  • Run tests and slow tests (8m 56s)
    • 6m 06s building, 2m 50s testing.
  • Run doc tests (3m 52s)
    • 3m 39s building, 13s testing.
  • Total build time: 9m 45s
  • Total test time: 3m 03s

Switching to nextest saves us just over 3% during testing (6 seconds), but increases build times by almost 60% due to the need to recompile with doctests enabled. I'm a little surprised by this, as I saw a 25% reduction overall (building + testing) locally, but I suspect this is due to both faster build times locally (I have a Ryzen 9 5950X), and overcontention on the more resource-constrained CI builders (since our proving tests are internally running multithreaded code). In any case, it's not worth us merging this PR unless nextest has a way to build the tests with doc tests enabled (even if it can't run them due to limitations in stable Rust).

str4d avatar Feb 28 '22 14:02 str4d

Speculation: is there any way we can do sophisticated / fine-grained build caching in CI, since that's the bottleneck?

I've experimented with a few cargo build caching github actions without much luck. I think I just don't understand gh CI well enough to understand how to do it well.

nathan-at-least avatar Sep 25 '23 16:09 nathan-at-least

Maintainer of nextest here -- it's a bit surprising that doctests cause a rebuild because I don't see that on my own projects, it's worth digging into why that is happening. Wondering if there's something related to proc macros going on (not sure if you have any).

In any case, it's not worth us merging this PR unless nextest has a way to build the tests with doc tests enabled

Sadly the two are related -- cargo test --doc is just a really special snowflake.

sunshowers avatar Oct 23 '23 07:10 sunshowers