paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.network/
1.62k stars 564 forks source link

Testing defensive paths. #203

Open ruseinov opened 1 year ago

ruseinov commented 1 year ago

Currently there seems to be no good way of testing defensive paths, due to the fact that they call debug_assert! under the hood, which panics in testing environment.

The only "solution" to that that I can currently see is using #[should_panic] macro, which is not the most eloquent way to deal with that and it also potentially catches other panics that we don't want to miss.

One decent option would be introducing #[defensive] macro for tests to catch that and reworking defensive checks to be catchable in test.

ggwpez commented 1 year ago

Do you mean to test that the defensive_assert! actually panics? That can be tested by checking for the correct message.

The problem is more that the tests have to be compiled twice currently to test panic and non-panic path. There was a discussion about this, but I cant find it right now.
It would be nicer to have something where we can assert posterior that there was a defensive failure without panicking the test. Otherwise we always have to write the test twice with debug_assertions, but even that is not ideal.

kianenigma commented 1 year ago

should_panic + always checking the error string is as good as it gets.

Also, there's a bit of a definition problem in

Currently there seems to be no good way of testing defensive paths

Because a defensive code path is one that by definition is almost never reached. Ofc I get that to be pedantic you want to test those as well, but the main value of the defensive_ ops is actually to observe that in other tests they are not triggered, not writing tests that specifically trigger them.

ruseinov commented 1 year ago

Because a defensive code path is one that by definition is almost never reached. Ofc I get that to be pedantic you want to test those as well, but the main value of the defensive_ ops is actually to observe that in other tests they are not triggered, not writing tests that specifically trigger them.

Do you mean to test that the defensive_assert! actually panics? That can be tested by checking for the correct message.

No, I'm talking about codepaths that are using various defensive_* mechanisms, mostly extensions to option/result and defensive! macro. Those use debug_assert! under the hood that does panic on cargo test builds.

but the main value of the defensive_ ops is actually to observe that in other tests they are not triggered, not writing tests that specifically trigger them.

Yep. I got that feeling as well. Will stick to not triggering those for now, but it would of course be much nicer if we had mechanisms in place to actually expect defensive checks and catch them.

defensive_catch! {
 something_that_emits_defensive_err()
}

or even

defensive_match!(something_that_emits_defensive_err(), err_string)
ggwpez commented 1 year ago

defensive_match!(something_that_emits_defensive_err(), err_string)

You can attempt that, but it will require some AssertUnwindSafe hacking, to get the objects across the panic unwind.
I dont think there is a straightforward way of doing this sadly…

xlc commented 1 year ago

I don't get it. If you want to test code should panic, use should_panic.

ruseinov commented 1 year ago

I don't get it. If you want to test code should panic, use should_panic.

Having a ‘defensive’ api that we control and no good way to test it seems like something that could be improved upon. Using ‘should_panic’ for now.