tests: replace many .unwrap()s with ?s using a new TestResult helper#9107
tests: replace many .unwrap()s with ?s using a new TestResult helper#9107
.unwrap()s with ?s using a new TestResult helper#9107Conversation
The intention is so that tests can use `?` instead of `unwrap`s. This uses `#[track_caller]` and ensures that the backtrace is printed, (if `RUST_BACKTRACE` is set) similar to the output of a panic on `.unwrap()`. The actual conversion of tests is left to the following commits.
ilyagr
force-pushed
the
ig/test-result-magic
branch
3 times, most recently
from
fe6e5cc to
4e8715e
Compare
`Backtrace::capture()` always gives the `RUST_BACKTRACE=full` experience, not the `RUST_BACKTRACE=1` experience. This commit allows both using the `backtrace` crate. https://github.com/rust-lang/backtrace-rs `backtrace` crate is actually part of `std` codebase, but stable Rust does not expose its API. It can be used separately.
…Result from parent commit This commit should be comparable with #9050 As of this writing, this replaced all but out of 1053 instances of `.block().unwrap()`. The remaining ones (as counted by AI): ``` ┌──────────────────┬───────┬────────────────────────────────────────────────────┐ │ Category │ Count │ Could convert? │ ├──────────────────┼───────┼────────────────────────────────────────────────────┤ │ testutils │ 20 │ No — return concrete types, would cascade to │ │ helpers │ │ hundreds of callers across all test files │ ├──────────────────┼───────┼────────────────────────────────────────────────────┤ │ Closures │ 53 │ No — closures return () or concrete values │ ├──────────────────┼───────┼────────────────────────────────────────────────────┤ │ Test helpers │ 22 │ Technically yes, but adds ~70 ? at call sites to │ │ │ │ save 22 unwraps │ ├──────────────────┼───────┼────────────────────────────────────────────────────┤ │ Non-convertible │ 4 │ No — CommandError (2), Pin<Box<dyn Future>> (1), │ │ │ │ closure in test_matrix (1) │ └──────────────────┴───────┴────────────────────────────────────────────────────┘ ```
ilyagr
force-pushed
the
ig/test-result-magic
branch
from
4e8715e to
a98b159
Compare
|
I'm marking this as ready for review because I'm running a little out of steam. The backtrace mess is... a mess, though the AI-generated code seems to work. Opinions on how this compares to #9050 are welcome. I might look into whether the |
|
Or maybe not. That only affects whether hex addresses are shown Some possibilities/discussions: |
|
If anyone is interested here are two alternatives (still AI-generated) for the backtrace parsing code: A. Don't add any dependencies. Do everything manually ((instead of the second commit): diff --git a/lib/testutils/src/lib.rs b/lib/testutils/src/lib.rs
index c662eb06b9..0c9c99c09d 100644
--- a/lib/testutils/src/lib.rs
+++ b/lib/testutils/src/lib.rs
@@ -112,7 +112,42 @@
write!(f, "{}\n\nLocation: {}", self.inner, self.location)?;
match self.backtrace.status() {
std::backtrace::BacktraceStatus::Captured => {
- write!(f, "\n\n{}", self.backtrace)?;
+ let full = self.backtrace.to_string();
+ let is_full = env::var_os("RUST_BACKTRACE").is_some_and(|v| v == "full");
+ if is_full {
+ write!(f, "\n\n{full}")?;
+ } else {
+ write!(f, "\nstack backtrace:")?;
+ // Trim frames after `__rust_begin_short_backtrace`, like
+ // the panic handler does for `RUST_BACKTRACE=1`.
+ let mut count = 0;
+ for line in full.lines() {
+ if line.contains("__rust_begin_short_backtrace") {
+ break;
+ }
+ // Renumber frames starting from 0
+ let trimmed = line.trim_start();
+ if trimmed.starts_with("at ") {
+ write!(f, "\n{line}")?;
+ } else if trimmed.starts_with(char::is_numeric) {
+ let rest = trimmed.trim_start_matches(char::is_numeric);
+ write!(f, "\n{count:>4}{rest}")?;
+ count += 1;
+ } else {
+ write!(f, "\n{line}")?;
+ }
+ }
+ if count == 0 {
+ // Marker not found; fall back to full output
+ write!(f, "\n{full}")?;
+ } else {
+ write!(
+ f,
+ "\nnote: Some details are omitted, run with \
+ `RUST_BACKTRACE=full` for a verbose backtrace."
+ )?;
+ }
+ }
}
_ => {
write!(B. Add diff --git a/lib/testutils/src/lib.rs b/lib/testutils/src/lib.rs
index 062d13d3ca..d1d7da19aa 100644
--- a/lib/testutils/src/lib.rs
+++ b/lib/testutils/src/lib.rs
@@ -121,23 +121,17 @@
let is_full = env::var_os("RUST_BACKTRACE").is_some_and(|v| v == "full");
let mut bt = self.backtrace.clone();
bt.resolve();
- // Find the range of interesting frames by looking for the short
- // backtrace markers that the test harness inserts.
- let frames = bt.frames();
- let begin_marker = frames.iter().position(|frame| {
- frame.symbols().iter().any(|s| {
- s.name()
- .is_some_and(|n| format!("{n:#}").contains("__rust_begin_short_backtrace"))
- })
- });
- let end_frames = if let Some(idx) = begin_marker.filter(|_| !is_full) {
- &frames[..idx]
+ let frames: Vec<_> = if is_full {
+ bt.frames()
+ .iter()
+ .map(|frame| (frame, 0..frame.symbols().len()))
+ .collect()
} else {
- frames
+ backtrace_ext::short_frames_strict(&bt).collect()
};
write!(f, "\nstack backtrace:")?;
- for (i, frame) in end_frames.iter().enumerate() {
- let symbols = frame.symbols();
+ for (i, (frame, sub_range)) in frames.iter().enumerate() {
+ let symbols = &frame.symbols()[sub_range.clone()];
if symbols.is_empty() {
write!(f, "\n{i:>4}: <unknown>")?;
}
@@ -156,7 +150,7 @@
}
}
}
- if !is_full && begin_marker.is_some() {
+ if !is_full {
write!(
f,
"\nnote: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose \Not much shorter, but |
|
I'm not sure to understand what this custom approach brings compared to just using For reference, here is the result with just using /// Convenient return type for test functions.
pub type TestResult = eyre::Result<()>;
|
|
That |
The elephant I missed in the room: using
I don't think it can, but I might be wrong—I don’t consider myself an expert in error handling in Rust! |
|
See #9111 as an alternative to this PR using |
I'm not sure I understand the question, I thought that's what that location in the screenshot is. One additional thing that eyre gets us is the nice printing of the error's cause chain. We could do it without eyre too, but we'd essentially be talking its code. Unfortunately, eyre suffers from several problems if we care about backtraces. 😞 |
|
(Oops, wrong PR) |
3 participants
This is an alternative to #9050, also AI-generated. I have not reviewed it super-carefully at this point. I do follow the first commit :)
Here is a demo of stack traces:
There was an additional issue that, with Rust's stdlib, this always gave the
RUST_BACKTRACE=fullstack trace experience instead ofRUST_BACKTRACE=1. AI fixed it using thebacktracecrate, I have not carefully reviewed that commit yet.Help in reviewing is welcome :)
An alternative approach might be to use https://github.com/athre0z/color-backtrace (we can make color optional IIUC, but not its adjustments to make the bakctrace prettier; we'd probably then want to make all test panics use its backtrace), or one of the crates that uses (https://github.com/yaahc/btparse?), or fork it/copy its logic
To reproduce this, use this diff on top of this PR:
For comparison, if we replace that
?back to.unwrap(), we get this stack trace:Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)how it works, how it's organized), including any code drafted by an LLM.
an eye towards deleting anything that is irrelevant, clarifying anything
that is confusing, and adding details that are relevant. This includes,
for example, commit descriptions, PR descriptions, and code comments.