Remove unwrap() in linera-core tests when possible.

[MathieuDutSik]

Motivation

The test code of linera-core is a little complicated. Some of this complication can be reduced by replacing
.unwrap() by ?.

Proposal

This is done but only for the Result<_,_> return values. The Option are left untouched.
This adds to the clarity of the code as one sees more clearly what is Result and what is Option.

Test Plan

The CI.

Release Plan

• Nothing to do / These changes follow the usual release cycle.

Links

None.

[deuszx]

My issue with this change is that tests that fail at .? are more difficult to debug due to hiding the error source somewhere deep in the stacktrace. Compare the two outputs of a test that fails somewhere inside:

1. fail().unwrap():

---- chain::chain_tests::test_service_as_oracles::single_service_as_oracle_call stdout ----

thread 'chain::chain_tests::test_service_as_oracles::single_service_as_oracle_call' panicked at linera-chain/src/unit_tests/chain_tests.rs:410:12:
called `Result::unwrap()` on an `Err` value: This test is expected to fail, so we panic here to ensure it does not pass by accident.

2. fail()?:

---- chain::chain_tests::test_service_as_oracles::single_service_as_oracle_call stdout ----
Error: This test is expected to fail, so we panic here to ensure it does not pass by accident.

Stack backtrace:
   0: std::backtrace_rs::backtrace::libunwind::trace
             at /rustc/05f9846f893b09a1be1fc8560e33fc3c815cfecb/library/std/src/../../backtrace/src/backtrace/libunwind.rs:117:9
   1: std::backtrace_rs::backtrace::trace_unsynchronized
             at /rustc/05f9846f893b09a1be1fc8560e33fc3c815cfecb/library/std/src/../../backtrace/src/backtrace/mod.rs:66:14
   2: std::backtrace::Backtrace::create
             at /rustc/05f9846f893b09a1be1fc8560e33fc3c815cfecb/library/std/src/backtrace.rs:331:13
   3: anyhow::error::<impl anyhow::Error>::msg
             at /Users/gorskimateusz/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/anyhow-1.0.98/src/backtrace.rs:27:14
   4: anyhow::__private::format_err
             at /Users/gorskimateusz/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/anyhow-1.0.98/src/lib.rs:694:13
   5: linera_chain::chain::chain_tests::fail
             at ./src/unit_tests/chain_tests.rs:371:5
   6: linera_chain::chain::chain_tests::test_service_as_oracles::{{closure}}
             at ./src/unit_tests/chain_tests.rs:410:5
   7: linera_chain::chain::chain_tests::test_service_as_oracles::single_service_as_oracle_call::{{closure}}
             at ./src/unit_tests/chain_tests.rs:375:1
...
  52: std::sys::pal::unix::thread::Thread::new::thread_start
             at /rustc/05f9846f893b09a1be1fc8560e33fc3c815cfecb/library/std/src/sys/pal/unix/thread.rs:106:17
  53: __pthread_cond_wait

In the second case the actual source of the failure is buried at 6th position of the stacktrace.

[deuszx]

I don't see anything wrong with the PR apart from the comment I made earlier. (Not approving just yet to let other weigh in on the topic).

[ma2bd]

I don't see anything wrong with the PR apart from the comment I made earlier. (Not approving just yet to let other weigh in on the topic).

Yes, this is exactly the reason unwrap() was used in the first place. Perhaps the decision can be revisited if there is a way to get stack traces with ? now.

[MathieuDutSik]

I don't see anything wrong with the PR apart from the comment I made earlier. (Not approving just yet to let other weigh in on the topic).

Ok.
The answer is that if we use the "?" pattern, we cannot know the line that creates the problem. It is simply not possible since the stack is changed. The panic occurs after the anyhow::Result is returned.

Now, if the goal is to know the line where the problem occurs, then I think we are doing it all wrong.

(1) There are already some ? in the test code. Pointedly, it is for code that tends to fail the test:

• For example, at handle_confirmed_certificate. This is the case that was most problematic during debugging recently. I had the wrong hash, wrong users, and balances. So, it had to be corrected.
• Similar hard functions with ? are chain_state_view or handle_block_proposal.

(2) There are unwrap() around code that rarely fails in tests:

• The bcs::to_bytes. The serialization very rarely fails.
• The create_chain. The creation of chain is not the issue, what we do with it is more challenging.
• The make_first_block. Sure, the creation of the block can fail, but the most likely outcome is that we do not get what we expect.
• The local_balance. The access to the local balance is usually not a problem. What is more problematic is getting the balance that we expect.
• The synchronize_from_validators. It is also a function that rarely fails.

Of course, "rare error" and "frequent error" are vague terminology. This is why I proposed to introduce ? systematically.