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

CI: Run tests with cargo-nextest #512

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

CI: Run tests with cargo-nextest #512

wants to merge 1 commit into from

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Feb 15, 2022

No description provided.

@str4d str4d marked this pull request as draft February 15, 2022 03:57
@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

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

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%) ⬇️
components/zcash_address/src/kind/unified.rs 38.80% <ø> (+0.82%) ⬆️
zcash_client_backend/src/data_api.rs 5.88% <ø> (ø)
zcash_client_backend/src/data_api/error.rs 20.51% <0.00%> (-2.35%) ⬇️
zcash_client_backend/src/data_api/wallet.rs 17.07% <0.00%> (-4.85%) ⬇️
zcash_client_backend/src/decrypt.rs 0.00% <0.00%> (ø)
zcash_client_sqlite/src/error.rs 23.52% <0.00%> (+2.10%) ⬆️
zcash_extensions/src/transparent/demo.rs 49.71% <ø> (+4.31%) ⬆️
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.

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

@str4d str4d changed the base branch from non-consensus-changes-on-branchid-37519621 to master February 17, 2022 01:14
@str4d str4d marked this pull request as ready for review February 17, 2022 01:15
@str4d
Copy link
Contributor Author

str4d commented Feb 17, 2022

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 str4d marked this pull request as draft February 17, 2022 01:39
@sunshowers
Copy link

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

@str4d str4d changed the base branch from master to non-consensus-changes-on-branchid-37519621 February 24, 2022 20:56
@str4d
Copy link
Contributor Author

str4d commented Feb 28, 2022

(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 str4d changed the base branch from non-consensus-changes-on-branchid-37519621 to main September 16, 2022 15:21
@nathan-at-least
Copy link
Contributor

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.

@sunshowers
Copy link

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.

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.

3 participants