lib: reduce .block_on().unwrap() calls in tests with extension trait by martinvonz · Pull Request #9050 · jj-vcs/jj

martinvonz · 2026-03-08T01:27:19Z

This was all written by Gemini CLI. I'm happy to drop it if people don't like it.

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 AI
For any prose generated by AI, 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

Maybe we should wait a bit to see if there are other opinions before merging, but this seems good to me.

I thought a bit about other options:

We could use something like eyre or anyhow, change tests to return eyre::Result, and use .block_on()?. This should actually give OK stack traces since eyre is supposed to use #[track_caller] (like this PR), but is probably not worth the dependency/fuss. I imagine the stack traces will definitely not get any better than they are now from using eyre (though perhaps others know a way to do that).
https://github.com/nik-rev/evil is a fun new option, but it's nightly-only
#[pollster::test] exists, and would replace .block_on() with .await, but it wouldn't help with unwraps. Proc-macros also can make rust-analyzer unhappy.

lib/testutils/src/lib.rs

yuja · 2026-03-08T04:46:58Z

If we're going to switch to tokio, #[tokio::test] might be better?
https://rust-lang.github.io/async-book/part-guide/more-async-await.html

ilyagr · 2026-03-08T04:53:26Z

Can that do better than .await.unwrap()?

AFAIK tokio::test has all the same advantages and disadvantages as pollster::test syntactically. One difference is that it would run tests with a real runtime, if we want to test that.

martinvonz · 2026-03-08T06:17:31Z

I think some test runners let you return a Result from the test function. It doesn't look like tokio's test runner does, but if it did, we would have been able to write .await? instead.

martinvonz · 2026-03-08T06:32:23Z

Btw, if we keep this PR, we should also discuss what we call the method. Let me know if you prefer a different name, such as unwrap_future() or join_unwrap().

glehmann · 2026-03-08T15:38:26Z

I think some test runners let you return a Result from the test function. It doesn't look like tokio's test runner does, but if it did, we would have been able to write .await? instead.

Tokio's test runner supports that, but we don't get the error location in the error message in case of failure, unless we use eyre (or evil, which I didn't know) as suggested by @ilyagr.

IMO, tokio's runner (or pollster, which I didn't know either) combined with eyre would be a pleasant combination for writing tests and worth the extra dev dependency.

martinvonz · 2026-03-08T16:16:52Z

Tokio's test runner supports that

Did they just forget to mention it on https://docs.rs/tokio/latest/tokio/attr.test.html then? Do you have an example of what looks like?

ilyagr · 2026-03-08T19:54:29Z

I am not entirely sure, but I think all test runners are meant to support returning Result, so maybe the tokio docs didn't think it's worth mentioning (though it certainly would be helpful if they did).

Here's an example of the alternative approaches:

https://gist.github.com/ilyagr/ad182a098781d403a923612fc57690e5

Update: I just noticed AI put imports inside the test functions. Sorry, it seems too minor to fix up now that I posted stack traces that refer to line numbers.

To run it quickly, you can do something like:

cargo init --name demo /tmp/demo && \
         cargo add --manifest-path /tmp/demo/Cargo.toml eyre pollster --features pollster/macro && \
         cargo add --manifest-path /tmp/demo/Cargo.toml tokio --features full,macros,rt-multi-thread,test-util && \
         curl -sLf https://gist.githubusercontent.com/ilyagr/ad182a098781d403a923612fc57690e5/raw/7eaf1e077e6778599cf92c64d63f797cdf453575/block_unwrap_alternatives.rs -o /tmp/demo/src/main.rs

Then RUST_BACKTRACE=1 cargo test --manifest-path /tmp/demo/Cargo.toml.

Result for me:

$ RUST_BACKTRACE=1  cargo test --manifest-path /tmp/demo/Cargo.toml
running 3 tests
test test_eyre_style ... FAILED
test test_tokio_eyre_style ... FAILED
test test_unwrap_style ... FAILED

failures:

---- test_eyre_style stdout ----
Error: backend error: commit not found

Location:
    src/main.rs:76:18

---- test_tokio_eyre_style stdout ----
Error: backend error: commit not found

Location:
    src/main.rs:93:18

---- test_unwrap_style stdout ----

thread 'test_unwrap_style' (67067479) panicked at src/main.rs:59:44:
called `Result::unwrap()` on an `Err` value: BackendError("commit not found")
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/47611e16044c68ef27bac31c35fda2ba1dc20b73/library/std/src/panicking.rs:689:5
   1: core::panicking::panic_fmt
             at /rustc/47611e16044c68ef27bac31c35fda2ba1dc20b73/library/core/src/panicking.rs:80:14
   2: core::result::unwrap_failed
             at /rustc/47611e16044c68ef27bac31c35fda2ba1dc20b73/library/core/src/result.rs:1867:5
   3: <core::result::Result<demo::Commit, demo::BackendError>>::unwrap
             at /Users/ilyagr/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/result.rs:1233:23
   4: demo::test_unwrap_style
             at ./src/main.rs:59:44
   5: demo::test_unwrap_style::{closure#0}
             at ./src/main.rs:56:23
   6: <demo::test_unwrap_style::{closure#0} as core::ops::function::FnOnce<()>>::call_once
             at /Users/ilyagr/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
   7: <fn() -> core::result::Result<(), alloc::string::String> as core::ops::function::FnOnce<()>>::call_once
             at /rustc/47611e16044c68ef27bac31c35fda2ba1dc20b73/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.


failures:
    test_eyre_style
    test_tokio_eyre_style
    test_unwrap_style

test result: FAILED. 0 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s

This also illustrates how each test needs to change.

IMO, the advantage of #[tokio::test] (or #[pollster::test]) over #[test] is minimal (unless we want to run tests with a real runtime), but the downsides of using that are also minimal (assuming rust-analyzer has as much support for #[tokio::test] as they do for #[test], which is likely but not certain). Eyre does seem to eat useful parts of the backtrace, though it leaves the most important part.

Apparently, using color-eyre and putting color_eyre::install() in the beginning of each test might help with the backtrace, I might get to testing it if people are really interested, or you the reader can try.

ilyagr · 2026-03-08T20:12:03Z

And here's a comparison between .block().unwrap() and this PR's approach:

---- test_pr9050_style stdout ----

thread 'test_pr9050_style' (67120915) panicked at src/main.rs:133:33:
called `Result::unwrap()` on an `Err` value: BackendError("commit not found")
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/47611e16044c68ef27bac31c35fda2ba1dc20b73/library/std/src/panicking.rs:689:5
   1: core::panicking::panic_fmt
             at /rustc/47611e16044c68ef27bac31c35fda2ba1dc20b73/library/core/src/panicking.rs:80:14
   2: core::result::unwrap_failed
             at /rustc/47611e16044c68ef27bac31c35fda2ba1dc20b73/library/core/src/result.rs:1867:5
   3: <core::result::Result<demo::Commit, demo::BackendError>>::unwrap
             at /Users/ilyagr/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/result.rs:1233:23
   4: <core::result::Result<demo::Commit, demo::BackendError> as demo::TestUnwrap>::test_unwrap
             at ./src/main.rs:112:38
   5: <demo::read_commit::{closure#0} as demo::FutureTestExt>::block_unwrap
             at ./src/main.rs:126:25
   6: demo::test_pr9050_style
             at ./src/main.rs:133:33
   7: demo::test_pr9050_style::{closure#0}
             at ./src/main.rs:132:23
   8: <demo::test_pr9050_style::{closure#0} as core::ops::function::FnOnce<()>>::call_once
             at /Users/ilyagr/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
   9: <fn() -> core::result::Result<(), alloc::string::String> as core::ops::function::FnOnce<()>>::call_once
             at /rustc/47611e16044c68ef27bac31c35fda2ba1dc20b73/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

---- test_unwrap_style stdout ----

thread 'test_unwrap_style' (67120917) panicked at src/main.rs:59:44:
called `Result::unwrap()` on an `Err` value: BackendError("commit not found")
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/47611e16044c68ef27bac31c35fda2ba1dc20b73/library/std/src/panicking.rs:689:5
   1: core::panicking::panic_fmt
             at /rustc/47611e16044c68ef27bac31c35fda2ba1dc20b73/library/core/src/panicking.rs:80:14
   2: core::result::unwrap_failed
             at /rustc/47611e16044c68ef27bac31c35fda2ba1dc20b73/library/core/src/result.rs:1867:5
   3: <core::result::Result<demo::Commit, demo::BackendError>>::unwrap
             at /Users/ilyagr/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/result.rs:1233:23
   4: demo::test_unwrap_style
             at ./src/main.rs:59:44
   5: demo::test_unwrap_style::{closure#0}
             at ./src/main.rs:56:23
   6: <demo::test_unwrap_style::{closure#0} as core::ops::function::FnOnce<()>>::call_once
             at /Users/ilyagr/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
   7: <fn() -> core::result::Result<(), alloc::string::String> as core::ops::function::FnOnce<()>>::call_once
             at /rustc/47611e16044c68ef27bac31c35fda2ba1dc20b73/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

To see this, add the following to the demo from the gist in my last comment:

trait TestUnwrap {
    type Output;
    #[track_caller]
    fn test_unwrap(self) -> Self::Output;
}
impl<T, E: std::fmt::Debug> TestUnwrap for Result<T, E> {
    type Output = T;
    #[track_caller]
    fn test_unwrap(self) -> T { self.unwrap() }
}
impl<T> TestUnwrap for Option<T> {
    type Output = T;
    #[track_caller]
    fn test_unwrap(self) -> T { self.unwrap() }
}

trait FutureTestExt: std::future::Future + Sized {
    #[track_caller]
    fn block_unwrap(self) -> <Self::Output as TestUnwrap>::Output
    where Self::Output: TestUnwrap,
    {
        use pollster::FutureExt as _;
        self.block_on().test_unwrap()
    }
}
impl<F: std::future::Future + Sized> FutureTestExt for F {}

#[test]
fn test_pr9050_style() {
    let commit = read_commit(0).block_unwrap();
    assert_eq!(commit.id, 2);
    let parent = read_parent(&commit).block_unwrap().unwrap();
    assert_eq!(parent.id, 1);
}

ilyagr · 2026-03-08T20:55:50Z

Oh, another option (if people are interested) might be to write our own version of eyre::Result that causes a stack trace to be printed.

I believe we'd just need to make the Display trait on our MagicResult to print what we need, and we'd need some functions to use #[track_caller]. It'd be up to us whether that logic is affected by the RUST_BACKTRACE env var or not. This is in addition to reimplementing the functionality of eyre::Result that makes MagicResult to wrap any Error, which might not be hard; I'm not sure if there are other things that'd need implementing that I haven't thought of.

It'd still have the minor limitation that ? would work only on Result<_, impl Error>; it would not work on Option. Making it work on Option like evil::Result does requires nightly, apparently.

My mild preference is to use this PR as-is, but implementing the above or any of the options (including doing nothing) are fine.

ilyagr · 2026-03-08T21:27:03Z

Here's an AI-generated 30-line implementation of MagicResult from my previous comment. It'd probably need a better name (TestResult?).

https://gist.github.com/ilyagr/9c51d69e209d2aed640d0e3298ad07f0

We'd need to add Ok(() to the end of every test for that to work (and make it return MagicResult). Apparently, there's no good way around that.

Tests using this (to be concatenated with both gists):

use pollster::FutureExt as _;

#[test]
fn test_magic_result() -> MagicResult {
    let commit = read_commit(0).block_on()?;
    assert_eq!(commit.id, 2);
    let parent = read_parent(&commit).block_on()?.unwrap();
    assert_eq!(parent.id, 1);
    Ok(())
}

#[pollster::test]  // Or #[tokio::test]
async fn test_magic_result() -> MagicResult {
    let commit = read_commit(0).await?;
    assert_eq!(commit.id, 2);
    let parent = read_parent(&commit).await?.unwrap();
    assert_eq!(parent.id, 1);
    Ok(())
}

martinvonz · 2026-03-09T04:01:29Z

That looks good to me. I don't feel strongly about which solution we go with. Does anyone else?

martinvonz · 2026-03-13T17:36:43Z

Do I understand correctly that the consensus is to drop this PR?

ilyagr · 2026-03-14T00:29:20Z

I'm still at

My mild preference is to use this PR as-is, but implementing the above or any of the options (including doing nothing) are fine.

I'm having an AI try to implement the TestResult. It has some limitations, but I'm not sure whether they are worth worrying about. The limitations I'm aware of so far:

Ugly to use TestResult in async closures.
Can't use it inside non-test code (this PR fixes up a couple of non-test .block_on().unwrap()-s)

Update: Though TestResult also has an upside: there are .unwrap()-s that have nothing to do with .block_on() can be replaced with ?s.

…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 · 2026-03-14T02:42:34Z

OK, I have a competing #9107. I'll see if I can make it output short backtraces, that will be ugly and might not work at all. If not for that, I think I now prefer it to this approach.

…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) │ └──────────────────┴───────┴────────────────────────────────────────────────────┘ ```

yuja · 2026-03-14T10:02:52Z

Do I understand correctly that the consensus is to drop this PR?

I don't follow the detailed comparison, but if anyhow/eyre produces readable output, that seems better than introducing our own test helpers. At some point, our internal might depend on tokio runtime.

…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) │ └──────────────────┴───────┴────────────────────────────────────────────────────┘ ```

martinvonz requested a review from a team as a code owner March 8, 2026 01:27

ilyagr approved these changes Mar 8, 2026

View reviewed changes

lib/testutils/src/lib.rs Show resolved Hide resolved

lib: reduce .block_on().unwrap() calls in tests with extension trait

bffcccd

martinvonz force-pushed the mz/xztxlnlylpsn branch from 1b337d6 to bffcccd Compare March 8, 2026 06:30

ilyagr mentioned this pull request Mar 14, 2026

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

Open

6 tasks

Conversation

martinvonz commented Mar 8, 2026

Checklist

Uh oh!

ilyagr left a comment • 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!

Uh oh!

yuja commented Mar 8, 2026

Uh oh!

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

Uh oh!

Uh oh!

martinvonz commented Mar 8, 2026

Uh oh!

martinvonz commented Mar 8, 2026

Uh oh!

glehmann commented Mar 8, 2026

Uh oh!

martinvonz commented Mar 8, 2026

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

martinvonz commented Mar 9, 2026

Uh oh!

martinvonz commented Mar 13, 2026

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

yuja commented Mar 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ilyagr left a comment •

edited

Loading

ilyagr commented Mar 8, 2026 •

edited

Loading

ilyagr commented Mar 8, 2026 •

edited

Loading

ilyagr commented Mar 8, 2026 •

edited

Loading

ilyagr commented Mar 8, 2026 •

edited

Loading

ilyagr commented Mar 8, 2026 •

edited

Loading

ilyagr commented Mar 14, 2026 •

edited

Loading

ilyagr commented Mar 14, 2026 •

edited

Loading