Instead of running the sanity checks as part of a single test case, run them as part of the normal setup for each test case. Benefits are:
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
tACK fa8145af797a6dac2cff049399fc8d85fd30fe0b
Ran the tests several times and seems ok (MacOS 11.3)
@ryanofsky Any Concept NACK thoughts on this?
Code review ACK fa8145af797a6dac2cff049399fc8d85fd30fe0b, and this seems fine to me. I do have an irrational dislike of the sanity checks because they throw exceptions and cause GDB catch throw false positives. Also in the future, I think it would be good to strip down the base test class and make extra setup like this optional. But this seems like a good change to make for current tests to be more consistent with the app.
Reviewed https://github.com/bitcoin/bitcoin/pull/21763/commits/fa8145af797a6dac2cff049399fc8d85fd30fe0b. Unit and functional tests are run successfully.
Errors during test development (e.g. wrong datadir) are caught early in the setup
I can see LockDataDirectory is called inside AppInitSanityChecks. But how do I manually generate an error by providing the wrong datadir, for both before and after this change, to verify the error is caught earlier after the change? E.g. can I give a wrong datadir to the command test_bitcoin?
After this change, 3 sanity checks (stdlib, secp256k1, chrono) which were in sanity_tests.cpp are now called in AppInitSanityChecks in setup_common.cpp, for all test cases. As the same AppInitSanityChecks is called in bitcoind.cpp, I agree individual tests setup are closer to the bitcoind setup, which is generally desirable. But I feel I don’t have enough experience and knowledge to consider all pluses and minuses to give an ACK.
they throw exceptions and cause GDB catch throw false positives.
I think any code may throw exceptions, so I am wondering if this is something specific to init code?
Also in the future, I think it would be good to strip down the base test class and make extra setup like this optional.
Agree, though the ECCVerifyHandle is pretty basic. If you don’t need that, you likely don’t need any setup at all.
I can see LockDataDirectory is called inside AppInitSanityChecks. But how do I manually generate an error by providing the wrong datadir, for both before and after this change, to verify the error is caught earlier after the change? E.g. can I give a wrong datadir to the command test_bitcoin?
Without any warranty, this may be possible by removing g_insecure_rand_ctx_temp_path.rand256().ToString() or replacing it with a constant literal string, so that m_path_root will conflict with other’s when multiple tests are run in parallel (make check -j 2 or larger).
This is also run first in the main() function of bitcoind
Github ACK https://github.com/bitcoin/bitcoin/pull/21763/commits/fa607f8250edf9aaffd91ebc6d341f287b93e4cf
More closely resembles bitcoind startup and removes code.
Also remove sanity unit tests, as they are run before all tests.
5-#include <test/util/setup_common.h>
6-#include <util/time.h>
7-
8-#include <boost/test/unit_test.hpp>
9-
10-BOOST_FIXTURE_TEST_SUITE(sanity_tests, BasicTestingSetup)
In commit “test: Run AppInitSanityChecks before all tests” (facdfb61b8df2958e47ced2c5014383fcf5affef)
I wish this commit would just add the AppInitSanityChecks assert without deleting the other tests. I don’t see a reason to delete low level tests just because high level code calls some of the same functions. Low level tests can be useful because they can find identify errors that might be masked in high level tests that are changing other state. Also they can be provide more granular errors in case of failure, and they can serve as examples to show how low-level APIs can be called.