test: locking -testdatadir when not specified and then deleting lock and dir at end of test#31328
test: locking -testdatadir when not specified and then deleting lock and dir at end of test#31328kevkevinpal wants to merge 1 commit intobitcoin:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31328. ReviewsSee the guideline for information on the review process. ConflictsNo conflicts as of last run. |
src/test/util/setup_common.cpp
Outdated
There was a problem hiding this comment.
I think the error message was a bit incorrect. "The test executable is probably already running." may not be accurate, because a lockfile may be present when the test executable crashed, no?
Also, it may be confusing for the default case, which should never have a colliding test dir. The issue in that case would be the path collision, not that "The test executable is probably already running.".
There was a problem hiding this comment.
maybe we can update the error message to be
ExitFailure("Cannot obtain a lock on test data lock directory " + fs::PathToString(m_path_lock) + '\n' + "The directory and .lock file already exist.");
it is fine if the directory exists, we are just don't want .lock file to be present?
There was a problem hiding this comment.
updated in fe6da3744420bcbac9002ae324dfa04902aed490
kevkevinpal
force-pushed
the
lockTestDataDir
branch
from
d599673 to
fe6da37
Compare
…and dir at end of test
kevkevinpal
force-pushed
the
lockTestDataDir
branch
from
fe6da37 to
8963bc8
Compare
|
Code review 8963bc8 Think this PR might be overly paranoid. Regardless of whether const auto rand{HexStr(g_rng_temp_path.randbytes(10))}Should provide this many possible patterns: Surely concurrent collisions should be practically non-existent on a given machine? Locking is more necessary when |
That is a fair point, if others agree I can close this PR. But I do think adding a lock to the directory does add a bit more robustness and protection to the test suite. |
|
I think its fine to leave this as-is for now. So going to close. |
5 participants
Description
Motivated by this comment #31317 (comment)
I wanted to lock the temp directory like we do when we specify the
-testdatadirparamChanges
Now when we run
BasicTestingSetup::~BasicTestingSetup()we unlock the directory no matter what and we always delete unless-testdatadiris usedTesting
FUZZ=utxo_total_supply ./build_fuzz/src/test/fuzz/fuzz -jobs=2 -workers=2