Standardise usage of #[should_panic] vs panic::catch_unwind in tests
#715
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `#[should_panic]` annotation allows marking tests that are expected to panic. It accepts an `expected` field which allows for substring matching against the panic message. `panic::catch_unwind` allows doing something similar, but requires a lot more boilerplate. However, it can be used in cases where the panic message contains dynamic values and substring matching isn't adequate for the purpose of what the tests intends to test. In general I believe we should prefer `#[should_panic]` and only use `panic::catch_unwind` when it benefits the test. I've added this recommendation to the top of the integration tests file, and updated several of the tests to follow it. Notably: - For the tests `address_for_port_when_container_crashed` and `app_dir_invalid_path`, the tests now uses `panic::catch_unwind` allowing the assertions to match against the whole dynamic message, improving coverage. - For `expected_pack_failure_still_panics_for_non_pack_failure`, the test has been switched back to using `#[should_panic]` (as it was before #666), since the purpose of the test is not to check the full error message of that specific failure mode, but instead to confirm that a non-pack failure isn't marked as a success when using `.expected_pack_result()`. See: https://doc.rust-lang.org/book/ch11-01-writing-tests.html#checking-for-panics-with-should_panic https://doc.rust-lang.org/std/panic/fn.catch_unwind.html https://doc.rust-lang.org/std/panic/struct.AssertUnwindSafe.html
Malax
approved these changes
Nov 2, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
#[should_panic]annotation allows marking tests that are expected to panic. It accepts anexpectedfield which allows for substring matching against the panic message.panic::catch_unwindallows doing something similar, but requires a lot more boilerplate. However, it can be used in cases where the panic message contains dynamic values and substring matching isn't adequate for the purpose of what the tests intends to test.In general I believe we should prefer
#[should_panic]and only usepanic::catch_unwindwhen it benefits the test.I've added this recommendation to the top of the integration tests file, and updated several of the tests to follow it.
Notably:
address_for_port_when_container_crashedandapp_dir_invalid_path, the tests now usespanic::catch_unwindallowing the assertions to match against the whole dynamic message, improving coverage.expected_pack_failure_still_panics_for_non_pack_failure, the test has been switched back to using#[should_panic](as it was before Support local buildpacks and meta-buildpacks in libcnb-test #666), since the purpose of the test is not to check the full error message of that specific failure mode, but instead to confirm that a non-pack failure isn't marked as a success when using.expected_pack_result().See:
https://doc.rust-lang.org/book/ch11-01-writing-tests.html#checking-for-panics-with-should_panic
https://doc.rust-lang.org/std/panic/fn.catch_unwind.html
https://doc.rust-lang.org/std/panic/struct.AssertUnwindSafe.html
GUS-W-14445053.