Simplify test code by using Results instead of unwraps

[wiktor-k]

As it says on the tin. I thought it would be a little bit more readable if we had ?s instead of unwraps in test code too. :)

[ionut-arm]

Oh, I'm not sure it's a good idea - maybe if they've changed something in how Rust is implemented, but if you use Result<...> and you get an error, it doesn't tell you where the error came from, just points to the line where the test function is declared. It's why we've been going the opposite way in some of our repos....

[wiktor-k]

Indeed that seems to be the case.

Running it like that gives the following info:

thread 'encrypt_decrypt' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /rustc/a178d0322ce20e33eac124758e837cbd80a6f633/library/test/src/lib.rs:193:5
test encrypt_decrypt ... FAILED

But when passing RUST_BACKTRACE=1 the trace looks like that:

thread 'encrypt_decrypt' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /rustc/a178d0322ce20e33eac124758e837cbd80a6f633/library/test/src/lib.rs:193:5
stack backtrace:
   0: rust_begin_unwind
             at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633/library/std/src/panicking.rs:515:5
   1: core::panicking::panic_fmt
             at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633/library/core/src/panicking.rs:92:14
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
             at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633/library/core/src/panicking.rs:117:5
   4: test::assert_test_result
             at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633/library/test/src/lib.rs:193:5
   5: basic::encrypt_decrypt::{{closure}}
             at ./tests/basic.rs:69:1
   6: core::ops::function::FnOnce::call_once
             at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633/library/core/src/ops/function.rs:227:5
   7: core::ops::function::FnOnce::call_once
             at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
test encrypt_decrypt ... FAILED

Filtering our stackframes (that is frames with sources in src) shows the error was triggered in:

   5: basic::encrypt_decrypt::{{closure}}
             at ./tests/basic.rs:69:1

This points to this line and just for tests I've inserted the following code into here:

if true {
    return Err(Error::NotSupported);
}

Unfortunately the line it points to is just the name of the test not the exact line when the error was returned - not really useful.

So, thanks for your time! I'm closing this PR.

[ionut-arm]

It would be so much nicer if errors somehow got some diagnostic information attached so you could print the backtrace for where it actually originated, that would be useful for debugging "production" code as well. 😞

[wiktor-k]

It would be so much nicer if errors somehow got some diagnostic information attached so you could print the backtrace for where it actually originated, that would be useful for debugging "production" code as well. 😞

Err... having 15 years of professional Java experience I can say that this is definitely a good point! :) Maybe this is just toothing problems for Rust as in nightly there are some interesting APIs like https://doc.rust-lang.org/std/backtrace/index.html

I just hacked together a small proof-of-concept and it seems it's possible to "have a cake and eat it too" with a bit of boilerplate code:

#[derive(Debug)]
struct WError; // this error struct is used only in function signatures as a result type

impl From<cryptoki::Error> for WError {
    fn from(p: cryptoki::Error) -> Self {
	panic!("WError: {:#?}", p);
    }
}

#[test]
#[serial]
fn encrypt_decrypt() -> std::result::Result<(), WError> { // <-- WError used here, the rest of the code remains unchanged

And since each ? is desugared into into (pun not intended) that calls from and panics this prints the correct stacktrace when the error was encountered even though ? is used:

   4: basic::encrypt_decrypt::{{closure}}
             at ./tests/basic.rs:114:9

Well, that's it, an interesting behavior 🤷 See you later! 👋

[ionut-arm]

Nice! Could do something even more generic and make that type in From<...> templated to any type that implements std::error::Error. There might be some crate out there that already does this, but it's small enough to just copy-paste around where we need it.

[wiktor-k]

Indeed. This looks simple enough. Naming suggestions welcome as I'm really bad at that ;)

[ionut-arm]

🚀

[ionut-arm]

Naming suggestions welcome as I'm really bad at that ;)

I'm not particularly good at it either, nor too fussy, so we can just leave as is, we don't even need to use the type anywhere.

[hug-dev]

Nice, good idea!