Skip to content

Dev quality-of-life improvements (better test backtraces in CI and locally; faster test builds; RUST_BACKTRACE=1 by default)#1603

Merged
daira merged 5 commits into
zcash:mainfrom
daira:ci-logs-should-have-backtraces
Nov 2, 2024
Merged

Dev quality-of-life improvements (better test backtraces in CI and locally; faster test builds; RUST_BACKTRACE=1 by default)#1603
daira merged 5 commits into
zcash:mainfrom
daira:ci-logs-should-have-backtraces

Conversation

@daira
Copy link
Copy Markdown
Contributor

@daira daira commented Nov 2, 2024

  • Remove .gitlab-ci.yml which is not used (and probably bitrotted).
  • Changes the test profile to compile with optimizations by default, but keep debug info.
    • This improves both build+test times and backtraces relative to cargo test --release, and does not require you to remember to pass --release (you still can, and the meaning of that has not changed).
    • You can use --profile=dev to get more precise stack traces in case information is optimized out. Remember to only run the desired subset of tests in that case (as before).
    • CI testing now uses this test profile instead of --release.
  • cargo will now use RUST_BACKTRACE=1 by default in this workspace, so that there is no need to set it manually.

In other words, cargo test now just does the Right Thing 🎉 (you may still have to set features).

fixes #1602

@daira daira requested a review from str4d November 2, 2024 14:03
@daira daira added the A-CI label Nov 2, 2024
@daira daira requested a review from y4ssi November 2, 2024 14:03
@daira daira added the S-scratching-an-itch Status: Something we haven't planned for a sprint but are doing anyway label Nov 2, 2024
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.30%. Comparing base (4b7f973) to head (5672b30).
Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1603   +/-   ##
=======================================
  Coverage   56.30%   56.30%           
=======================================
  Files         148      148           
  Lines       19062    19062           
=======================================
+ Hits        10732    10733    +1     
+ Misses       8330     8329    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

daira added 5 commits November 2, 2024 18:30
Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
…test

profile to compile with optimizations by default, but keep full debug info.

This differs from the release profile in the following ways:
- it does not set `lto = true`, which increases compile times without
  substantially speeding up tests;
- it does not set `codegen-units = 1`, which increases compile times and
  is only useful to improve determinism of release builds;
- it does not set `panic = 'abort'`, which is in any case ignored for
  tests.

After this PR, to get results as close as possible to a release build, use
`cargo test --release`.

To speed up compilation and avoid optimizations potentially resulting in
lower-quality debug info, use `cargo test --profile=dev`.

Times on my machine starting from `cargo clean` for each run:
* `cargo test --all-targets --all-features`:
  * 484s (354s build, 130s tests)
* `cargo test --release --all-targets --all-features`:
  * 541s (415s build, 126s tests)
* `cargo test --profile=dev --all-targets --all-features`:
  * 1709s (146s build, 1563s tests)
  * this might still be faster when running individual tests.

Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
(instead of only in CI), so that there is no need to set it manually.

You can override this by setting `RUST_BACKTRACE=0`, or
`RUST_BACKTRACE=full` for a full backtrace.

Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
@daira daira force-pushed the ci-logs-should-have-backtraces branch from 63366c2 to 5672b30 Compare November 2, 2024 18:45
@daira daira requested a review from nuttycom November 2, 2024 19:01
@daira
Copy link
Copy Markdown
Contributor Author

daira commented Nov 2, 2024

See #1604 (comment) for an example backtrace from CI.

@daira daira changed the title CI test logs should have backtraces; also remove unused GitLab CI config Dev quality-of-life improvements (better test backtraces in CI and locally; faster test builds; RUST_BACKTRACE=1 by default; remove unused GitLab CI config) Nov 2, 2024
@daira daira changed the title Dev quality-of-life improvements (better test backtraces in CI and locally; faster test builds; RUST_BACKTRACE=1 by default; remove unused GitLab CI config) Dev quality-of-life improvements (better test backtraces in CI and locally; faster test builds; RUST_BACKTRACE=1 by default) Nov 2, 2024
@daira
Copy link
Copy Markdown
Contributor Author

daira commented Nov 2, 2024

This significantly improves CI latency for tests, by about a third (3 minutes). This is due to the faster build (no LTO and no codegen-units = 1) which more than compensates for a slight slowdown in running the tests.

Before (#1580):
image

After (this PR):
image

Copy link
Copy Markdown
Collaborator

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

utACK, looks great!

@daira daira added this pull request to the merge queue Nov 2, 2024
Merged via the queue into zcash:main with commit c6db09b Nov 2, 2024
@daira daira deleted the ci-logs-should-have-backtraces branch November 2, 2024 19:56
@str4d
Copy link
Copy Markdown
Contributor

str4d commented Nov 20, 2024

Post-hoc ACK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-CI S-scratching-an-itch Status: Something we haven't planned for a sprint but are doing anyway

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI tests should run with RUST_BACKTRACE=1

3 participants