###### tags: `features` `code-quality`

# Testing rust code

## Test return types

Now rust allows tests to return a result. In my opinion this is very good. We can convert

fn test() {
  let res = SomeType::new().unwrap(); // or expect("some comment")
  assert!(some_condition(res), "logic");
}

into

fn test() -> Result<(), Error> {
    let res = SomeType::new()?;
    assert!(some_condition(res), "logic");
}

So both tests fail to execute when the assert is invalid. However the first test with unwrap may unexpectedly fail to execute with a line number and optionally some text. The second allows the test to fail to execute with an **unexpected** error using the error system to shw us **why** it errored/failed. What I like about this is that it forces us to correctly handle unexpected errors and not mix them up with test asserts. If we wish to test a bit of code does error then we can get the error and compare via an assert if it did error as we thought. that would and get away from using `#[should_panic]` which is a bit messy/backwards.

In both cases we still use asserts to test invariants.

**Note:** *These unexpected failures are **not** test failures. They mean the test failed to test/run to completion or assertion failure. So the test was not carried out, it never failed, it just did not run!*

*A test failling means some assert failed, however we should not conflate failing test with a test that failed to run. They are not the same. When a test fails to run to completion it's just bad code, not a failed test.*

> [name=Adam Cigánek] Some good points. One worry I have is that using `Result` might make it harder to find the source of the failure. That might get fixed when backtrace gets stabilized though.
> Very Good point about the expected errors - this would be much better than the `#[should_panic]` mechanism.
> Might be worth trying this pattern out on a couple of tests before deciding to apply it everywhere.
> [name=divine] Yes that is what I think as well Adam, try it in some tests. I am thinking we don't use `Result` as a pass fail, but only to show us unexpected errors, so still rely on assert to do the actual tests. If we see failures due to an error it should be printed out AFAIK. To me that shows we need another test case and possibly a code fix.

## Test code, quality

Or, should tests be allowed to have otherwise dirty code in them, i.e. lots of asserts and panics, or should that be restricted? My feeling is using `Result` return types even in unit tests will allow us to clean up tests and hopefully have the code quality as high as production code.

> [name=Adam Cigánek] Don't necessarily agree that asserts/panics/unwraps are dirty (especially in tests), otherwise see my previous comment.
> [name=dirvine] It's the panics and unwraps really. I do not think they should be any more prevelent in tests than they are in production, if that makes sense? Asserts are fine & required in tests though I agree. My point is too many in lots of tests may be test code smell as the test is doing too much?

## Test code documentation

Are our tests well documented as to their purpose? This is rhetorical really. I suspect we need to have a quick 1 or 2 line comment above each unit test to tell us why it exists and for integration tests as synopsis of what the test is checking and how.

> [name=Adam Cigánek] We should also aim to write simple and readable tests, so it's easy to figure out what's going on in them. One way to do this is to use well named test helpers. But comments are great too.
> [name=dirvine] Yes, 100% simple easy to understand tests are critical. We shoudl add some text about that.

## Unit tests

Testing methods, functions and private logic.

## Integration tests

Testing public interfaces.

## Functional / e2e tests

Testing at crate level via creating use cases for the crate or testing in it's actual use case, such as testing routing with a vault network or quic-p2p with scl/api.

## Asserts

Asserts are good in tests as they are very powerful. They halt the test when an invariant is broken. This invariant check should be the purpose of a test and it is probably a rare thing to have many invariant checks in a single test (not impossible, but rare). As asserts are powerful things we should use them very wisely.

> [name=Adam Cigánek] :+1:

## log_or_panic

This seems like it should be `error_or_panic` and even then it would be strange in code. If we get an error we must handle it or maybe abort. However handling errors is what the code should do, not print out text (log). In our case almost every call to log_or_panic has a loglevel `Error` which I think cements the case being made here. There is confusion at times between `critical` and `error`, log levels. We seem to use `Error` when we mean `critical` well, if it's critical enough for us to fail our code after aborting a test!

I suggest all use of this macro are removed and replaced with error handling.

> [name=Adam Cigánek] I see `log_or_panic` as similar to `debug_assert` except it's enabled by a feature flag instead of the profile (debug vs release). Perhaps this should be made more clear by renaming it to something else (`test_panic`?) and also using a separate feature to enable it. Also maybe mixing logging and panicking into one expression is confusing and two separate expressions would be better? But then the message would probably be repeated in both...
> That said, I'm not opposed to stopping using it. If the situation in which it is used is really invalid, then use normal `panic`. Otherwise handle it like any other error.
> [name=dirvine] Yes, it's the latter part I feel is the case. It kinda allows us to ignore stuff I feel we need to fix. I mean if we fail a test becaue of the error then just logging in production feels wrong.

## Errors

A big area, people tend to either have 1 or 2 errors defined and use them for all errors, or they have an error type for every function. I claim, both approaches are incorrect and errors should be very difficult to just add to a system, but the error should explain why a function failed.

We moved recently to `err-derive`, not the best crate or even the latest and greatest in rust error handling, but the `Error` type is based on `std::Error` which gives us error composition (source()) and more. This allows us to analyse errors, even in tests to see the error path through our code and more easily confirm the problem. This further allows us to look at where we can fix (recover from) that error in our code.

## unwrap macro

Probably not required as assert and panic now have line numbers that are printed out. Reducing our dependencies is a good goal and this crate should now be removed.

> [name=Adam Cigánek] Yes, not needed since 1.42 which stabilised the `track_caller` feature.
> [name=dirvine] :+1:

## Unwrap in tests

I would favour limiting it to the places we allow in production. As explained above, I think our test code needs to be production quality. I will repeat this point so it's not lost, we are not saying ban `unwrap` or `expect` in tests, what we are saying is do not use them in places where we would not also use in production code, which is **very limited**.

## assert in tests

Don't think any of us have an issue here ,but asserting what specifically can be a point of debate. It relates to "what is this test for" type questions.

> [name=Adam Cigánek] Still not entirely clear on why `assert` is OK but `unwrap` is not, as both `panic` under the hood.
> [name=dirvine] I see unwrap as a quick thing and ok for POC work, but I feel our test code should be of the same standard as our production code. Assert is a similar thing, but decorated differently and is expected in tests. It should be quite specific, whereas `panic` due to unwrap can hide why the panic happened. The panic in our cases most of the time is from an eror, but we hide the error by aborting. So the code broke in a manner we don't really know why, but we guess the cause, where we could specifically know the cause if we checked what the error was.
> Assert does abort, but prints the assertion failure reason out, which is what it's designed for, panic is a more general purpose `exit/abort` thing. You can add text etc. to a panic and so on, but it kinda is more a sledge hammer. If we don't unwrap and use error handling in the test suite then we can see the reason for the failure, which is just more info really.
> tl;dr assert is testing a specific assertion, however panic is more like saying "abort on any error" if that makes sence. So we panic on `X::new().unwrap()` for instance and we are saying I don't care why you fail, just abort, whereas in tests we do/should care.