TestAssertionFailure should implement Error

[bjackman]

I'm trying to incrementally adopt Googletest. I have a test helper that returns anyhow::Result<()> and I want to start replacing its inner logic with verify_that!(...)?. verify_that! doesn't return an anyhow::Result of course, and usually in Anyhow I solve that by slapping a .context() call onto the end. But this doesn't work:

 2  error[E0599]: the method `context` exists for enum `Result<(), TestAssertionFailure>`, but its trait bounds were not satisfied
     --> src/test.rs:1297:57
      |
 1297 |             verify_that!(notif.status, eq(want_status)).context("assertion failed")?;
      |                                                         ^^^^^^^ method cannot be called on `Result<(), TestAssertionFailure>` due to unsatisfied trait bounds

So, basically you cannot mix TestAssertionFailure with other types of error (be it via Anyhow or just like Box<dyn Error> or whatever).

Maybe truly orthopraxic (just added that word to my vocabulary, thanks ChatGPT) test helpers wouldn't wanna do this and the fact that I have this problem is a sign that I haven't properly absorbed the googletest-rs doctrine? Like, when something that isn't exactly a test failure goes wrong, instead of returning errors in the failure case I should be failing an assert_*?

But, it seems unnecessary to enforce that doctrine? And maybe there are more important reasons that I haven't thought of why TestAssertionFailure should implement error?

[bjackman]

Hmm, or maybe the solution here is to go the other way around: instead of getting TestAssertionFailure into a std::Result<_, E: std::Error> you should just squash your non-googletest result into a googltest result using IntoTestResult?

[bjacotg]

I tried to add impl std::error::Error for TestAssertionFailure {} but it clash with the impl<T: std::error::Error> From<T> for TestAssertionFailure (in the case of T == TestAssertionFailure since there is a impl<T> From<T> for T in core).
I prefer keeping the impl From since it allow to ? any Result<_, impl Error> like anyhow::Error does. This is the same reason why anyhow::Error does not implement Error (see dtolnay/anyhow#153).

I still need to iron out the googletest-rs error handling doctrine, but I like to think of googletest::Result<_> as the anyhow::Result<_> for tests and testonly code. So, I would recommend to replace testonly functions return type from anyhow::Result to googletest::Result. Any Result<_, impl Error> can still be ?, but you would need to change .context() to .into_test_result() (that method name can be changed to something nicer...).

I have an AI to dig further into anyhow and check that googletest covers all of its features. If you have any issues with the approach above, please let me know, I will be very interested in your feedback!

[bjackman]

This is the same reason why anyhow::Error does not implement Error (see dtolnay/anyhow#153).

Oh yeah I actually discovered that after posting this issue, I should have updated it!

I like to think of googletest::Result<> as the anyhow::Result<> for tests and testonly code.

That sounds sensible to me. I think the most important thing Anyhow does is the Context trait which allows you to tack on extra info as you propagate errors, so you end up with error messages like the ones Go code tends to produce ("couldn't do the thing: couldn't read the config file from /path/to/config: no such file or directory") instead of the ones C++ code tends to produce ("no such file or directory" or "couldn't do the thing"). Not sure if googletest::Result already has something like that?