The test is exercising the error, so it can capture it before the test framework displays it on the console as an unforeseen fatal error.
It is odd to observe a fatal error after executing the complete test suite and seeing it pass successfully.
Reproduction Steps: Run the unit test suite. A long AssumeUTXO fatal error will be printed even when all tests pass successfully.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | theStack, MarcoFalke, TheCharlatan |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
The test is exercising the error, so it can capture it before
the test framework displays it on the console as an unforeseen
fatal error.
665 | + { 666 | + bool error_printed = false; 667 | + DebugLogHelper log_helper("Bitcoin Core failed to validate the -assumeutxo snapshot state", [&](const std::string* s) { 668 | + if (s) error_printed = true; 669 | + return false; 670 | + });
ASSERT_DEBUG_LOG ("failed to validate the -assumeutxo snapshot state");
How is this different from just using the helper macro? Also, you can't use the hardcoded program name here. Finally, it may be good to add steps to reproduce/test for reviewers. Otherwise it is unclear what your goal is. Reading the description you are trying to avoid that some error string is output?
Didn't know that the macro existed. Thanks.
Finally, it may be good to add steps to reproduce/test for reviewers. Otherwise it is unclear what your goal is. Reading the description you are trying to avoid that some error string is output?
Yeah. The chainstatemanager_snapshot_completion_hash_mismatch unit test manually triggers a fatal error, and the test framework by default prints the error message to the console (this one).
So, just by running the unit test suite, you will see a long AssumeUTXO fatal error even when all tests passed successfully.
Updated per feedback, thanks @MarcoFalke.
DebugLogHelper usage to ASSERT_DEBUG_LOG.
Tested ACK 3e8bf2e10c26ef9c2f58307b523e4b674ac97a2c
Also it may be good to name the commit that introduced the bug. Otherwise it will be harder for reviewers to see if there was a bug in that commit or if the change was intentional. Also, reviewers will be missing context.
Also it may be good to name the commit that introduced the bug. Otherwise it will be harder for reviewers to see if there was a bug in that commit or if the change was intentional. Also, reviewers will be missing context.
Ok. It was introduced with the test 87a1108c8. Still, this isn't as serious as you imagine. It is just an extra print. 100% sure that wasn't intended.
The test is exercising a specific fatal error (checking that is being triggered), and doing so it makes the node call AbortNode. Which calls InitError which is connected to the noui signals interface, which is listened by the test framework base class, which prints the error to the console.
Are you sure, because locally I don't see it in 25.0:
$ ./bitcoin-25.0/bin/test_bitcoin -t validation_chainstatemanager_tests/chainstatemanager_snapshot_completion_hash_mismatch
Running 1 test case...
*** No errors detected
Are you sure, because locally I don't see it in 25.0:
$ ./bitcoin-25.0/bin/test_bitcoin -t validation_chainstatemanager_tests/chainstatemanager_snapshot_completion_hash_mismatch Running 1 test case... *** No errors detected
Ok good, found the commit. 6eb33bd0c21b3e075fbab596351cacafdc947472 introduced it. It removed the mock_shutdown function from the test (which was the one swalling the error).
cc @TheCharlatan then. Thanks @MarcoFalke.
lgtm ACK 3e8bf2e10c26ef9c2f58307b523e4b674ac97a2c
ACK 3e8bf2e10c26ef9c2f58307b523e4b674ac97a2c
Thank you for fixing this.