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
10. 
    hodlinator commented at 11:07 pm on November 21, 2024: contributor

    Code review 8963bc860f6f55462851fbda28627b4483ba8240

    Think this PR might be overly paranoid.

    Regardless of whether G_TEST_GET_FULL_NAME returns a value for the given test, is locking really necessary if directories are sufficiently unique (random) thanks to faaaf59f71ede057b2c1d369ef8db973c2f2dbc2 being included in the already merged #31317?

    0const auto rand{HexStr(g_rng_temp_path.randbytes(10))}
    

    Should provide this many possible patterns:

    02^{8*10} = 256^{10} \approx 1.21*10^{24}
    

    Surely concurrent collisions should be practically non-existent on a given machine?

    Locking is more necessary when -testdatadir is set and randomness is not included in the path.

11. 
    kevkevinpal commented at 5:42 pm on November 22, 2024: contributor

    Code review 8963bc8

    Think this PR might be overly paranoid.

    Regardless of whether G_TEST_GET_FULL_NAME returns a value for the given test, is locking really necessary if directories are sufficiently unique (random) thanks to faaaf59 being included in the already merged #31317?

    0const auto rand{HexStr(g_rng_temp_path.randbytes(10))}
    

    Should provide this many possible patterns: 2 8 ∗ 10 = 256 10 ≈ 1.21 ∗ 10 24

    Surely concurrent collisions should be practically non-existent on a given machine?

    Locking is more necessary when -testdatadir is set and randomness is not included in the path.

    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.

12. 
    fanquake commented at 8:43 pm on February 20, 2025: member

    I think its fine to leave this as-is for now. So going to close.
13. fanquake closed this on Feb 20, 2025

    ---

Contributors
kevkevinpal DrahtBot maflcko hodlinator fanquake

Labels
Tests