[nextest-runner] skip loading output for replays unless necessary#3090
[nextest-runner] skip loading output for replays unless necessary#3090sunshowers merged 1 commit intomainfrom
Conversation
c6a634a to
b21564e
Compare
There was a problem hiding this comment.
Pull request overview
This PR optimizes cargo nextest replay by skipping decompression/loading of recorded test output when reporter configuration guarantees the output will never be displayed, while keeping the decision logic conservative and validated via property tests.
Changes:
- Introduces
OutputLoadDeciderto conservatively decide whether a replayed output event needs its output bytes loaded. - Adds a
NotLoadedoutput variant during replay conversion and wires a per-eventLoadOutputdecision through replay conversion. - Refactors output description types:
OutputSpecnow usesChildOutputDesc, and recordings use the newZipStoreOutputDescription(separate from live output).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| nextest-runner/src/reporter/structured/libtest.rs | Handles the new NotLoaded output variant defensively (unreachable in live libtest reporting). |
| nextest-runner/src/reporter/mod.rs | Re-exports OutputLoadDecider for replay callers. |
| nextest-runner/src/reporter/events.rs | Refactors output description generics (ChildOutputDesc) and adds ChildOutputDescription::NotLoaded. |
| nextest-runner/src/reporter/error_description.rs | Adds NotLoaded match arm with invariant-based unreachable!. |
| nextest-runner/src/reporter/displayer/unit_output.rs | Factors output display overrides and adds NotLoaded match arm with invariant-based unreachable!. |
| nextest-runner/src/reporter/displayer/status_level.rs | Adds proptests establishing safety invariants for skipping output loads during replay. |
| nextest-runner/src/reporter/displayer/mod.rs | Re-exports OutputLoadDecider. |
| nextest-runner/src/reporter/displayer/imp.rs | Implements OutputLoadDecider and wires overrides through displayer logic. |
| nextest-runner/src/reporter/aggregator/junit.rs | Handles NotLoaded output variant defensively (unreachable in live JUnit reporting). |
| nextest-runner/src/record/summary.rs | Introduces ZipStoreOutputDescription for recorded output metadata and updates generic bounds to ChildOutputDesc. |
| nextest-runner/src/record/rerun.rs | Updates tests to use ZipStoreOutputDescription. |
| nextest-runner/src/record/replay.rs | Adds LoadOutput, threads it through conversion, and returns NotLoaded when skipping output loads. |
| nextest-runner/src/record/recorder.rs | Converts live ChildOutputDescription into ZipStoreOutputDescription and asserts NotLoaded is impossible during recording. |
| nextest-runner/src/record/mod.rs | Re-exports LoadOutput and ZipStoreOutputDescription. |
| nextest-runner/src/output_spec.rs | Renames associated type to ChildOutputDesc and maps Live/Recording specs to the new description types. |
| cargo-nextest/src/dispatch/core/replay.rs | Computes LoadOutput per event via OutputLoadDecider and passes it into replay conversion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if baseline.store_final == OutputStoreFinal::No { | ||
| assert_eq!( | ||
| with_cancel.store_final, | ||
| OutputStoreFinal::No, | ||
| "cancel_status={cancel_status:?} caused final output storage that \ |
There was a problem hiding this comment.
In cancellation_only_hides_output, the final-output monotonicity check only covers the baseline.store_final == OutputStoreFinal::No case. Consider also asserting that if baseline.store_final is Yes { display_output: false } then with_cancel.store_final never becomes Yes { display_output: true } (i.e. cancellation never causes output bytes to be displayed at the end). This would better enforce the stated invariant and protect against future changes to compute_output_on_test_finished that might accidentally flip display_output from false to true.
Measured this against a 727 test run locally -- `time cargo nextest replay > /dev/null` dropped from 0.15s to 0.135s. We use a pure function with a conservative approximation to avoid having to track the cancel reason and other less relevant facts. The property-based tests ensure the invariants.
b21564e to
929829e
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3090 +/- ##
==========================================
- Coverage 84.27% 84.27% -0.01%
==========================================
Files 156 156
Lines 42446 42536 +90
==========================================
+ Hits 35772 35847 +75
- Misses 6674 6689 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Measured this against a 727 test run locally --
time cargo nextest replay > /dev/nulldropped from 0.15s to 0.135s.We use a pure function with a conservative approximation to avoid having to track the cancel reason and other less relevant facts.
The property-based tests ensure the invariants.