Hi @lukas good points (POC in particular, I do that)
I debate this in house actually. To me a test should not blindly abort unless explicitly via an assert. Now tests can have Result return types I feel it’s best that an error is returned so we can see the logic that caused that. I wrote up a wee in house thing, I will post here. Good to get thoughts on it.
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=Redacted] Some good points. One worry I have is that using
Resultmight 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 Redacted, try it in some tests. I am thinking we don’t useResultas 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=Redacted] 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 prevalent 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=Redacted] 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.
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=Redacted] I see
log_or_panicas similar todebug_assertexcept 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 normalpanic. 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=Redacted] Yes, not needed since 1.42 which stabilised the
track_callerfeature.
[name=dirvine]
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=Redacted] Still not entirely clear on why
assertis OK butunwrapis not, as bothpanicunder 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, whereaspanicdue 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 purposeexit/abortthing. 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 sense. So we panic onX::new().unwrap()for instance and we are saying I don’t care why you fail, just abort, whereas in tests we do/should care.
) though with significant caveats.