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

pull kevkevinpal wants to merge 1 commits into bitcoin:master from kevkevinpal:lockTestDataDir changing 1 files +20 −17
  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.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me