The exception checking in unit tests is partly verbose, fragile, inconsistent and thus confusing.
Fix all those issues by using BOOST_CHECK_EXCEPTION consistently:
exceptionThrown = false; or BOOST_CHECK(0) can be removed.HasReason{...} or a similar predicate.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36018.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | l0rinc, janb84, jonatack |
| Concept ACK | hebasto |
| Stale ACK | rustaceanrob |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Concept ACK.
tACK fa279bdd58779954032a821bb989962406483f41
Clear test simplification and readability improvement
Now that we are touching the file, unify the HasReason usage () => {}
HasReason{"double lock detected"});
BOOST_CHECK_EXCEPTION(REVERSE_LOCK(lock1, mutex1), std::logic_error, HasReason{"mutex1 was not most recent critical section locked"});
Sure, but those are just strings. No narrowing can happen, so I don't think it matters. Leaving as-is for now.
ACK fa279bdd58779954032a821bb989962406483f41
Built and ran the test, LGTM. Checked each new expectation against the throw; the types and reasons match. Just an observation is that threadpool_test.cpp L 384 is now a bit stricter in some ways than the original.
few non-blocking nits to unify the HasReason usage now that we are touching the file.
1086 | ssPeers1 >> pchMsgTmp; 1087 | ssPeers1 >> addrman1; 1088 | - } catch (const std::exception&) { 1089 | - exceptionThrown = true; 1090 | - } 1091 | + }());
nit, can probably also be:
uint8_t pchMsgTmp[4];
BOOST_CHECK_NO_THROW(ssPeers1 >> pchMsgTmp >> addrman1);
nit2: "Fix all those issues by using BOOST_CHECK_EXCEPTION consistently" - might want to generalize the PR description
384 | - } catch (const std::exception& e) { 385 | - BOOST_CHECK(strstr(e.what(), 386 | - "Rewind limit must be less than buffer size") != nullptr); 387 | - } 388 | + // The buffer size must be greater than the rewind amount. 389 | + BOOST_CHECK_EXCEPTION((BufferedFile{file, /*nBufSize=*/25, /*nRewindIn=*/25}), std::runtime_error, HasReason{"Rewind limit must be less than buffer size"});
nit: https://github.com/bitcoin/bitcoin/blob/31ce729b28ec7b1013054518559c55c642ac778e/src/streams.h#L558
BOOST_CHECK_EXCEPTION((BufferedFile{file, /*nBufSize=*/25, /*nRewindIn=*/25}), std::ios_base::failure, HasReason{"Rewind limit must be less than buffer size"});
ACK fa279bdd58779954032a821bb989962406483f41
ACK fa0fe212f52ad261bfc59683dd8309a7a0cf3a51
ACK fa0fe212f52ad261bfc59683dd8309a7a0cf3a51
Light ACK fa0fe212f52ad261bfc59683dd8309a7a0cf3a51
(unsure if relevant here but it looks like there are a couple of similar remaining cases, e.g., in sockman_tests and transaction_tests)
(unsure if relevant here but it looks like there are a couple of similar remaining cases, e.g., in sockman_tests and transaction_tests)
The explanation why it can not be used is explained in the comment above the code:
src/test/sock_tests.cpp:168: // BOOST_CHECK_EXCEPTION() writes to some variables shared with the main thread which
src/test/sock_tests.cpp-169- // creates a data race. So mimic it manually.
This boost bug is tracked in #34666.
Re transaction_tests: I don't think this is possible, because BOOST_CHECK_NO_THROW(tx_valid = VerifyScript(...)); doesn't take an error message to print the "Bad test: " << strTest. So I think it is better to leave as-is for now.
Going further, one could also work on replacing BOOST_CHECK_THROW(code, ex_typ) -> BOOST_CHECK_EXCEPTION(code, ex_type, HasReason("the exact failure message")), where meaningful. (Happy to review such a pull, but it seems unrelated to this pull, whose goal is to remove hand-rolled impls)