Skip to content

tests: replace many .unwrap()s with ?s using eyre #9111

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.

Sign up for GitHub

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

Open
glehmann wants to merge 7 commits into
base: main
Choose a base branch
from
+4,817 −4,936
Open

tests: replace many .unwrap()s with ?s using eyre #9111

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
465c748
testutils: use eyre::Result as TestResult
glehmann Mar 14, 2026
b202110
testutils: use color-eyre for backtrace display
glehmann Mar 14, 2026
dbdc038
testutils: add custom backtrace frame filter to color-eyre
glehmann Mar 15, 2026
c9b89b3
testutils: enable backtrace capture by default (RUST_BACKTRACE=1) in …
glehmann Mar 15, 2026
80ee629
tests: `.block_on().unwrap()` with `.block_on()?` in tests using Test…
ilyagr Mar 14, 2026
e758f76
tests: Replace another ~600 `unwrap()`s in tests with `?`
ilyagr Mar 14, 2026
3b5a9a7
tests: Replace Result.unwrap with ? operator in test functions
glehmann Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 132 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,17 @@ clap_complete_nushell = "4.6.0"
# will need regenerating.
clap-markdown = "=0.1.5"
clap_mangen = "0.2.25"
color-eyre = "0.6"
clru = "0.6.3"
criterion = "0.8.2"
ctor = "0.2"
crossterm = { version = "0.29", default-features = false, features = ["windows"] }
datatest-stable = "0.3.3"
digest = "0.10.7"
dunce = "1.0.5"
either = "1.15.0"
erased-serde = "0.4.10"
eyre = "0.6"
etcetera = "0.11.0"
futures = "0.3.32"
gix = { version = "0.80.0", default-features = false, features = [
Expand Down
3 changes: 3 additions & 0 deletions lib/testutils/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ readme = { workspace = true }
[dependencies]
async-trait = { workspace = true }
bstr = { workspace = true }
color-eyre = { workspace = true }
ctor = { workspace = true }
eyre = { workspace = true }
futures = { workspace = true }
gix = { workspace = true, features = [
"status",
Expand Down
27 changes: 27 additions & 0 deletions lib/testutils/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,33 @@ pub mod git;
pub mod proptest;
pub mod test_backend;

#[ctor::ctor]
fn init_color_eyre() {
// Enable backtrace capture by default in tests
if std::env::var("RUST_BACKTRACE").is_err() {
Copy link
Contributor

@ilyagr ilyagr Mar 18, 2026

Choose a reason for hiding this comment

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

(Probably no action needed) I have two worries about this:

  • Can ctor support this on all systems? AFAIU, it's only supposed to use libc. OTOH, if this passes CI, it probably works.
  • I'm not 100% sure we want this. But it's probably better? I'm up for trying it and seeing if it makes anyone unhappy.

unsafe {
std::env::set_var("RUST_BACKTRACE", "1");
}
}

drop(
color_eyre::config::HookBuilder::default()
.add_frame_filter(Box::new(|frames| {
frames.retain(|frame| {
frame
.name
.as_ref()
.map(|name| name.starts_with("jj_"))
Copy link
Contributor

@ilyagr ilyagr Mar 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

It's nice that color-eyre has filtering functionality! That could be very helpful.

I don't know if this filter is correct for all use-cases, though. Ideally, there's be a message letting you know that you can set RUST_BACKTRACE=full to get the full backtrace in the cases where this filters too much. (I haven't checked how easy that is to do).

As per my other message, I'm thinking of panic-s, not TestResult use-cases.

Aside: My PR has an example of a more complicated-looking filter that's similar to default panic handler frame filtering logic, for comparison. I'm not sure that's necessarily better; this stricter filter could be good in 90% of the cases if there was a way to disable it in the other 10%.

Copy link
Contributor Author

@glehmann glehmann Mar 18, 2026

Choose a reason for hiding this comment

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

There is an env var to set to disable the filtering, and it's displayed with the backtrace—see the screenshots in #9111 (comment)

image

The filtering is especially useful with #[tokio::test]: the backtraces are much longer in that case—29 frames instead of just 7 with the #[test] for the same test failure. #[pollster::test] is a bit more at 9 frames.

I like the way the error chains are displayed, but eyre alone already displays them well.

It may not be perfect every time, but it should work in most cases, produce much more compact and readable backtraces, also work with panics, and the filtering is easily deactivable when needed. I like it :)

Copy link
Contributor

@ilyagr ilyagr Mar 18, 2026

Choose a reason for hiding this comment

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

Have you tested that it works, that your code doesn't override it?

It's fine if the filtering is not completely perfect, as long as it can be disabled; we can always fine-tune or even change our mind about using color_eyre later. So, I think that as long as COLORBT_SHOW_HIDDEN works, that's sufficient.

.unwrap_or(false)
});
}))
.install(),
);
}

/// Convenient return type for test functions.
pub type TestResult<T = ()> = eyre::Result<T>;

pub const HERMETIC_GIT_CONFIGS: &[(&str, &str)] = &[
// gitoxide uses "main" as the default branch name, whereas git uses "master". This also
// prevents git CLI from issuing the initial branch name advice.
Expand Down