test: locking -testdatadir when not specified and then deleting lock and dir at end of test #31328

1. 
   kevkevinpal commented at 2:05 am on November 20, 2024: contributor

   Description

   Motivated by this comment #31317 (review) I wanted to lock the temp directory like we do when we specify the -testdatadir param

   Changes

   Now when we run BasicTestingSetup::~BasicTestingSetup() we unlock the directory no matter what and we always delete unless -testdatadir is used

   Testing

   • I’ve validated by both running the fuzz test runner and individual fuzz tests like so
   • FUZZ=utxo_total_supply ./build_fuzz/src/test/fuzz/fuzz -jobs=2 -workers=2
   • functional tests are passing
2. 
   DrahtBot commented at 2:05 am on November 20, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31328.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   No conflicts as of last run.

3. DrahtBot added the label Tests on Nov 20, 2024
4. in src/test/util/setup_common.cpp:157 in d599673f22 outdated

   158-        }
   159+    m_path_root = m_path_lock / "datadir";
   160+    // Try to obtain the lock; if unsuccessful don't disturb the existing test.
   161+    TryCreateDirectories(m_path_lock);
   162+    if (util::LockDirectory(m_path_lock, ".lock", /*probe_only=*/false) != util::LockResult::Success) {
   163+        ExitFailure("Cannot obtain a lock on test data lock directory " + fs::PathToString(m_path_lock) + '\n' + "The test executable is probably already running.");
   

   ---

   ---

   

   maflcko commented at 7:10 am on November 20, 2024:

   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.".

   ---

   

   kevkevinpal commented at 1:47 pm on November 20, 2024:

   maybe we can update the error message to be

   0ExitFailure("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?

   ---

   

   kevkevinpal commented at 2:03 pm on November 20, 2024:

   updated in fe6da3744420bcbac9002ae324dfa04902aed490
5. DrahtBot added the label Needs rebase on Nov 20, 2024
6. kevkevinpal force-pushed on Nov 20, 2024
7. test: locking -testdatadir when not specified and then deleting lock and dir at end of test 8963bc860f
8. kevkevinpal force-pushed on Nov 20, 2024
9. DrahtBot removed the label Needs rebase on Nov 20, 2024

Contributors
kevkevinpal DrahtBot maflcko

Labels
Tests