tests: replace many .unwrap()s with ?s using eyre by glehmann · Pull Request #9111 · jj-vcs/jj

glehmann · 2026-03-14T18:48:49Z

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
I have updated the config schema (cli/src/config-schema.json)
I have added/updated tests to cover my changes
I fully understand the code that I am submitting (what it does,
how it works, how it's organized), including any code drafted by an LLM.
For any prose generated by an LLM, I have proof-read and copy-edited with
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.

ilyagr · 2026-03-15T02:47:12Z

Thanks for looking at this!

Have you tested what the backtraces look like? My understanding is that colored-eyre's backtraces will look like the version of panic backtraces you get with RUST_BACKTRACE=full, which are noticeably more verbose and confusing than the backtraces you get with RUST_BACKTRACE=1. There are examples in the pr message of my pr.

That's the issue that turned that pr into a bit of a headache.

ilyagr · 2026-03-15T03:04:39Z

Another reason I was doubtful about this approach is the need to either "install" colored-eyre in every test or use the ctor crate. ctor's docs make it seem a bit questionable.

Also I'm not sure whether this installing affects just the panic handler, or whether it also allows backtraces when you use ?. The annoying thing is that eyre absolutely collects the backtraces on ?, it just doesn't print them in impl Debug, which is what all the test handles use to print the error.

github-actions

The following commits do not follow our format for subject lines:

b3eabf0: more unwrap() converted to ?
445b3f9: error in simple_op_store.rs

Commits should have a subject line following the format <topic>: <description>. Please review the commit guidelines for more information.

glehmann · 2026-03-15T20:35:35Z

color-eyre has a mechanism to filter the backtrace. I've implemented it in this PR.

Here is how it looks by default with cargo t simple_op_store::tests::test_read_write_view:

with RUST_BACKTRACE=full cargo t simple_op_store::tests::test_read_write_view

with COLORBT_SHOW_HIDDEN=1 cargo t simple_op_store::tests::test_read_write_view:

Installing color-eyre in each test looked way too intrusive, but using ctor to avoid that seems acceptable to me.

color-eyre is also lacking the column number in the location—this is an easy fix in color-eyre though, and is probably acceptable to not have while it's being integrated in color-eyre.

All commits are now correctly formatted. Thank you for your contribution!

glehmann · 2026-03-15T21:01:59Z

The message also looks nice with a panic:

The backtrace shows more frames than before because I ran the test with #[tokio::test].

glehmann · 2026-03-16T08:37:44Z

color-eyre is also lacking the column number in the location—this is an easy fix in color-eyre though, and is probably acceptable to not have while it's being integrated in color-eyre.

eyre-rs/eyre#268

yuja · 2026-03-16T11:38:32Z

I agree that it's okay to use some linker magic like ctor in tests. I also don't care much about verbosity/prettiness of test failures so long as the traceback contains enough information to inspect the failure.

Filter backtrace frames to show only project code (jj_lib and jj_cli), removing noise from dependencies and standard library to make error reports easier to read during testing.

ilyagr · 2026-03-18T19:09:42Z

I had one new thought: perhaps the backtraces for the ?/TestResult case are not actually useful? The error pretty much has to happen in the main test function (those are the only ones returning TestResult). So, maybe the default eyre without the backtrace is just fine for this case.

If that's correct, and if TestResult is our only concern, I'd prefer using either eyre without color-eyre or the simple parts of my PR (not the backtrace processing stuff).

What color-eyre is good for is for formatting panics that occur in other locations. Whether that's worth using the ctor crate is a separate question. We can try it if people want and if we are happy with the backtraces color-eyre produces in that case (which might need some filtering).

ilyagr · 2026-03-18T19:13:49Z

lib/testutils/src/lib.rs

+                    frame
+                        .name
+                        .as_ref()
+                        .map(|name| name.starts_with("jj_"))


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%.

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

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 :)

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.

…tests So backtraces are shown without requiring the environment variable.

…Result from parent commit This commit should be comparable with jj-vcs#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) │ └──────────────────┴───────┴────────────────────────────────────────────────────┘ ```

Mostly done with OpenCode and Claude Sonnet 4.6 with the following prompt, and reviewed manually: * Replace the Result.unwrap with the ? operator in test functions * Add TestResult as return type in the test functions where you replace unwrap by the ? operator, and Ok(()) at the end. * Do the replacement only in method with the `#[test]` decorator. You can't exclude files that are not in tests dirs, because some files have inline tests. * Don't replace unwraps in closures. * Don't replace Option.unwrap. * Be careful to not change the insta snapshot references. * You can identify all the Result.unwrap locations with this command: cargo clippy --tests --message-format=short -- -W clippy::unwrap_used 2>&1 | grep / | grep Result | grep <filename> * Run the tests each time you're done with a file. * Begin with the file with the most Result.unwrap in them as shown by this command: cargo clippy --tests --message-format=short -- -W clippy::unwrap_used 2>&1 | grep Result | cut -d: -f1 | sort | uniq -c | sort -nr Before this PR: ❯ cargo clippy --tests --message-format=short -- -W clippy::unwrap_used 2>&1 | grep Result | wc -l 3973 After this PR: ❯ cargo clippy --tests --message-format=short -- -W clippy::unwrap_used 2>&1 | grep Result | wc -l 1174

ilyagr · 2026-03-18T23:08:14Z

lib/testutils/src/lib.rs

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


(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.

ilyagr · 2026-03-18T23:11:24Z

I'm pretty happy with this. I'm not approving yet because

I haven't looked over the latest version of the code very carefully, nor have I tested it out myself yet
I wanted to wait at least a bit if others have opinions

One minor concern I have is #9111 (comment), but unless somebody has thoughts, I don't think changes are needed. If either of my concerns are realized, I think it will be neither subtle nor confusing.

ilyagr · 2026-03-18T23:16:33Z

I agree that it's okay to use some linker magic like ctor in tests.

I think you have more experience with linkers than I, @yuja, so thanks for looking. Perhaps you're also the person who can tell whether #9111 (comment) is worrying or fine.

glehmann mentioned this pull request Mar 14, 2026

tests: replace many .unwrap()s with ?s using a new TestResult helper #9107

Closed

6 tasks

glehmann changed the title ~~Gln/refactor unwrap to question mark in tests lqyw~~ tests: replace many .unwrap()s with ?s using eyre Mar 14, 2026

github-actions bot previously requested changes Mar 15, 2026

View reviewed changes

glehmann force-pushed the gln/refactor-unwrap-to-question-mark-in-tests-lqyw branch from 3771490 to 0e3b5ad Compare March 15, 2026 20:40

glehmann added 3 commits March 18, 2026 17:42

testutils: use eyre::Result as TestResult

465c748

testutils: use color-eyre for backtrace display

b202110

testutils: add custom backtrace frame filter to color-eyre

dbdc038

Filter backtrace frames to show only project code (jj_lib and jj_cli), removing noise from dependencies and standard library to make error reports easier to read during testing.

glehmann force-pushed the gln/refactor-unwrap-to-question-mark-in-tests-lqyw branch from 0e3b5ad to 395ea53 Compare March 18, 2026 17:05

glehmann marked this pull request as ready for review March 18, 2026 17:09

glehmann requested a review from a team as a code owner March 18, 2026 17:09

ilyagr reviewed Mar 18, 2026

View reviewed changes

glehmann and others added 4 commits March 18, 2026 22:27

testutils: enable backtrace capture by default (RUST_BACKTRACE=1) in …

c9b89b3

…tests So backtraces are shown without requiring the environment variable.

tests: Replace another ~600 unwrap()s in tests with ?

e758f76

glehmann force-pushed the gln/refactor-unwrap-to-question-mark-in-tests-lqyw branch from 395ea53 to 3b5a9a7 Compare March 18, 2026 21:28

ilyagr reviewed Mar 18, 2026

View reviewed changes

Conversation

glehmann commented Mar 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ilyagr commented Mar 15, 2026

Uh oh!

ilyagr commented Mar 15, 2026

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

glehmann commented Mar 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glehmann commented Mar 15, 2026

Uh oh!

glehmann commented Mar 16, 2026

Uh oh!

yuja commented Mar 16, 2026

Uh oh!

ilyagr commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilyagr Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glehmann Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

ilyagr Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

ilyagr Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

ilyagr commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilyagr commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

glehmann commented Mar 14, 2026 •

edited

Loading

glehmann commented Mar 15, 2026 •

edited

Loading

ilyagr commented Mar 18, 2026 •

edited

Loading

ilyagr Mar 18, 2026 •

edited

Loading

ilyagr commented Mar 18, 2026 •

edited

Loading

ilyagr commented Mar 18, 2026 •

edited

Loading