test: test_bitcoin: allow -testdatadir= #26564

pull LarryRuane wants to merge 1 commits into bitcoin:master from LarryRuane:2022-11-testdir changing 8 files +121 −14
  1. LarryRuane commented at 2:13 am on November 24, 2022: contributor

    This backward-compatible change would help with code review, testing, and debugging. When test_bitcoin runs, it creates a working or data directory within /tmp/test_common_Bitcoin\ Core/, named as a long random (hex) string.

    This small patch does three things:

    • If the (new) argument -testdatadir=<datadir> is given, use <datadir>/test_temp/<test-name>/datadir as the working directory
    • When the test starts, remove <datadir>/test_temp/<test-name>/datadir if it exists from an earlier run (currently, it’s presumed not to exist due to the long random string)
    • Don’t delete the working directory at the end of the test if a custom data directory is being used

    Example usage, which will remove, create, use /somewhere/test_temp/getarg_tests/boolarg, and leave it afterward:

    0$ test_bitcoin --run_test=getarg_tests/boolarg -- -testdatadir=/somewhere
1Running 1 test case...
2Test directory (will not be deleted): "/somewhere/test_temp/getarg_tests/boolarg/datadir"
3
4*** No errors detected
5$ ls -l /somewhere/test_temp/getarg_tests/boolarg/datadir
6total 8
7drwxrwxr-x 2 larry larry 4096 Feb 22 10:28 blocks
8-rw-rw-r-- 1 larry larry 1273 Feb 22 10:28 debug.log

    (A relative pathname also works.)

    This change affects only test_bitcoin; it could also be applied to test_bitcoin-qt but that’s slightly more involved so I’m skipping that for now.

    The rationale for this change is that, when running the test using the debugger, it’s often useful to watch debug.log as the test runs and inspect some of the other files (I’ve looked at the generated blknnnn.dat files for example). Currently, that requires figuring out where the test’s working directory is since it changes on every test run. Tests can be run with -printtoconsole=1 to show debug logging to the terminal, but it’s nice to keep debug.log continuously open in an editor, for example.

    Even if not using a debugger, it’s sometimes helpful to see debug.log and other artifacts after the test completes.

    Similar functionality is already possible with the functional tests using the --tmpdir= and --nocleanup arguments.

  2. DrahtBot commented at 2:13 am on November 24, 2022: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK cbergqvist, marcofleon, davidgumberg, furszy, tdb3, achow101
    Concept ACK stickies-v, kouloumos, RandyMcMillan

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Tests on Nov 24, 2022
  4. in src/test/util/setup_common.cpp:102 in bc32016e58 outdated 
     97@@ -98,7 +98,9 @@ std::ostream& operator<<(std::ostream& os, const uint256& num)
 98 }
 99 
100 BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::vector<const char*>& extra_args)
101-    : m_path_root{fs::temp_directory_path() / "test_common_" PACKAGE_NAME / g_insecure_rand_ctx_temp_path.rand256().ToString()},
102+    : m_path_root{std::getenv("TEST_BITCOIN_PATH") ?
103+                      std::getenv("TEST_BITCOIN_PATH") :
    maflcko commented at 7:31 am on November 24, 2022:

    pretty sure this will break when running in parallel.

    What about a –nocleanup option, like the one in the functional test framework, if the goal is to not delete the dir?

    stickies-v commented at 11:26 am on November 24, 2022:
    Not sure if this would work, but what about checking the datadir lock before removing it, e.g. with util/system.cpp::LockDirectory?
    maflcko commented at 1:48 pm on November 24, 2022:
    To explain, tests running in parallel can not share a data directory. With rand256, each test gets a unique one. However, with TEST_BITCOIN_PATH all tests get the same one.
    stickies-v commented at 2:02 pm on November 24, 2022:
    I know, my suggestion was rather: since this is an advanced setting, we could let the user handle making sure that there are no tests running in parallel, but just add a simple check in case they aren’t aware/mistaken (by checking the datadir lock)?
    LarryRuane commented at 5:22 pm on November 24, 2022:

    What about a –nocleanup option … if the goal is to not delete the dir?

    Yes, but also to have a fixed data directory path, so I can open its debug.log in an editor, and re-read the file as the test runs and after the test completes. (Or better yet, set the editor to automatically re-read if possible, which I do with vim.) Then if I run the test again, I can re-read the file again, without having to open a different file.

    But I think specifying this environment variable can also indicate not to delete the directory when the test completes; you wouldn’t want to do this normally because the disk usage would continuously increase as you run the test repeatedly. If instead you specify a fixed directory, then disk usage doesn’t grow as you repeatedly run tests, so it’s okay for the test not to delete it at the end.

    tests running in parallel can not share a data directory

    Yes, you definitely wouldn’t specify this to multiple tests running in parallel. I would only use this when I’m running a single test that I’m trying to understand or debug. That is, I definitely would not set this environment variable in my .bashrc! (I could improve the doc changes to make that clear.) The default is to use separate directories (as today).

    … checking the datadir lock …

    The unit tests don’t create a lock file; only bitcoind does that. The unit tests don’t actually create a full datadir, only some parts of it, and can also create extra files that aren’t normally in a datadir. (That’s why I called it a working dir in the title and description, instead of a datadir, although the test README does call it a datadir.)

    I don’t think it’s worth adding a lock file; I think users of this will be advanced enough to understand that multiple tests can’t simultaneously share the same work directory.

    maflcko commented at 5:30 pm on November 24, 2022:
    ok, sgtm
  5. in src/test/README.md:58 in bc32016e58 outdated 
    52@@ -53,6 +53,11 @@ The `-printtoconsole=1` after the two dashes redirects the debug log, which
53 would normally go to a file in the test datadir
54 (`BasicTestingSetup::m_path_root`), to the standard terminal output.
55 
56+The `TEST_BITCOIN_PATH` environment variable, if set, determines the
57+location of the test datadir, which is recreated for each individual
    stickies-v commented at 11:17 am on November 24, 2022:
    Useful to specify which tests this affects?
    LarryRuane commented at 5:24 pm on November 24, 2022:
    Do you mean which executables (as this PR’s description does, bench, fuzz)? That’s a good idea, I can add this text (or similar) to the READMEs for those too.
    stickies-v commented at 2:37 pm on November 25, 2022:
    Yep pretty much what you did in the PR description
  6. in src/test/README.md:56 in bc32016e58 outdated 
    52@@ -53,6 +53,11 @@ The `-printtoconsole=1` after the two dashes redirects the debug log, which
53 would normally go to a file in the test datadir
54 (`BasicTestingSetup::m_path_root`), to the standard terminal output.
55 
56+The `TEST_BITCOIN_PATH` environment variable, if set, determines the
    stickies-v commented at 11:17 am on November 24, 2022:

    I think it’s helpful to have all env vars start with BITCOIN_

    0The `BITCOIN_TEST_PATH` environment variable, if set, determines the
    LarryRuane commented at 5:26 pm on November 24, 2022:
    Good idea, I named it that way because the executable is called test_bitcoin but that’s not a strong reason. I’ll make that change.
  7. stickies-v commented at 11:26 am on November 24, 2022: contributor
    Concept ACK - seems helpful to improve productivity in certain workflows.
  8. Onyeali approved
  9. LarryRuane force-pushed on Nov 25, 2022
  10. LarryRuane renamed this:
    test: allow TEST_BITCOIN_PATH to specify working dir
    test: allow BITCOIN_TEST_PATH to specify working dir
    on Nov 25, 2022
  11. LarryRuane commented at 8:43 pm on November 25, 2022: contributor

    Force-pushed to incorporate two review suggestions (thanks!):

    • add description to the other README files (benchmark, fuzz, qt)
    • rename TEST_BITCOIN_PATH to BITCOIN_TEST_PATH
  12. luke-jr commented at 8:41 am on November 26, 2022: member

    If the environment variable BITCOIN_TEST_PATH is set, use its value as the test’s working directory, rather than a randomly-named path in /tmp/test_common_Bitcoin\ Core/ When the test starts, remove the working directory if it exists

    This combination seems dangerous… :/

  13. LarryRuane force-pushed on Nov 26, 2022
  14. LarryRuane commented at 6:12 pm on November 26, 2022: contributor

    @luke-jr

    This combination seems dangerous… :/

    If the concern is that someone may specify a real data directory by mistake, that’s a good point! A real datadir has a .lock file, whether bitcoind is running or not. This file will never exist in one of these unit test work directories (I verified that by running all the tests, and also by source code inspection starting with ".lock").

    I just force-pushed code to exit without removing the work directory if contains .lock, for example:

    0$ BITCOIN_TEST_PATH=~/.bitcoin src/test/test_bitcoin --run_test=getarg_tests/logargs
1Running 1 test case...
2unknown location(0): fatal error: in "getarg_tests/logargs": std::runtime_error: .lock exists, not using possible real data directory
3test/getarg_tests.cpp(423): last checkpoint: "logargs" fixture ctor
4
5*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
6$

    Does this address your concern? Is there a better way to report the error?

  15. luke-jr commented at 7:54 am on November 27, 2022: member
    Not quite, I’m more concerned about a situation where BITCOIN_TEST_PATH might be /home/user or something else weird. While it’s a crazy user error, nobody expects setting an env variable would nuke a path.
  16. LarryRuane force-pushed on Nov 28, 2022
  17. LarryRuane commented at 6:03 am on November 28, 2022: contributor

    Not quite, I’m more concerned about a situation where BITCOIN_TEST_PATH might be /home/user or something else weird. While it’s a crazy user error, nobody expects setting an env variable would nuke a path.

    Another great point! Force-pushed a change (c48a1b4c6949af4e610035d037c6195bc22904ca) so that the environment variable only replaces the random path component within /tmp/test_common_Bitcoin\ Core/mydatadir/. The specified path must be relative. All I care about is that the datadir has a fixed name (across multiple test runs). This should now be very safe. (Also removed the check for .lock, not needed.) I’ll update the description now too.

    0$ BITCOIN_TEST_PATH=mydatadir src/test/test_bitcoin --run_test=getarg_tests/doubledash
1Running 1 test case...
2
3*** No errors detected
4$ ls -l /tmp/test_common_Bitcoin\ Core/mydatadir/
5total 8
6drwxrwxr-x 2 larry larry 4096 Nov 27 22:45 blocks
7-rw-rw-r-- 1 larry larry 1003 Nov 27 22:45 debug.log
8$
  18. in src/test/util/setup_common.cpp:109 in c48a1b4c69 outdated 
    106+    std::string test_path;
107+    if (test_path_env == nullptr) {
108+        test_path = g_insecure_rand_ctx_temp_path.rand256().ToString();
109+    } else {
110+        test_path = test_path_env;
111+        if (fs::PathFromString(test_path).is_absolute()) {
    maflcko commented at 8:07 am on November 28, 2022:
    Maybe just use a hardcoded path item like "fixed" and turn the env into a flag? This should be fine, because anyone can still modify the root temp dir.
    LarryRuane commented at 4:39 pm on November 28, 2022:

    I know this sounds a little far-fetched, but I have actually done the following: I have two test binaries, one from a PR branch and the other from master, and the test fails on the PR branch but passes on master. I’ve single-stepped through the tests simultaneously (in sync) comparing them to see what happens leading up to the failure. In this case, it would be convenient to have two different fixed data directories (if I needed to restart the test many times) because I also want to watch the two debug.log files.

    Also, as long as we’re going to the trouble of implementing and documenting an environment variable, it’s no extra work to make it a value instead of a flag.

    maflcko commented at 4:48 pm on November 28, 2022:

    Yes, that is still possible. You can set TMPDIR=/tmp/1, see https://en.cppreference.com/w/cpp/filesystem/temp_directory_path

    Not sure if relevant, but BITCOIN_TEST_PATH=../../home/user will still delete your home dir.

    LarryRuane commented at 6:31 pm on November 28, 2022:

    Good catch! I just force-pushed an update to require that BITCOIN_TEST_PATH doesn’t contain the separator character. That should take care of both problems. I would say that if the user has set TMPDIR, it should be safe to delete the given directory, even if it’s within the user’s home directory, since paths containing ../ are no longer allowed.

    0$ BITCOIN_TEST_PATH=../mydatadir src/test/test_bitcoin --run_test=getarg_tests/doubledash
1Running 1 test case...
2unknown location(0): fatal error: in "getarg_tests/doubledash": std::runtime_error: BITCOIN_TEST_PATH cannot contain path separators
3test/getarg_tests.cpp(380): last checkpoint: "doubledash" fixture ctor
4
5*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
6$
  19. LarryRuane force-pushed on Nov 28, 2022
  20. in src/test/README.md:58 in 8aee8b42c3 outdated 
    54+
55+By default the directory is named as a random string (different for each
56+run), but this can be overridden with the `BITCOIN_TEST_PATH` environment
57+variable. Be careful not to run multiple simultaneous instances of
58+`test_bitcoin` that share the same test data directory; they'll conflict
59+with each other.
    kouloumos commented at 9:22 am on February 2, 2023:

    Just from this note, I did not realize that running

    0BITCOIN_TEST_PATH=mydatadir test_bitcoin --run_test=getarg_tests

    is also not optimal because each BOOST_AUTO_TEST_CASE overrides the previous directory. And the same goes for running benchmarks with a --filter=. Which I think does not fall under “multiple simultaneous instances of” test_bitcoin or bench_bitcoin.

    I don’t think that this is bad, just that it should be more explicitly stated that using this env variable is mostly suitable for running single benchmarks/test cases.

    I am not familiar with fuzzing, therefore I haven’t verified if the same applies there.

  21. in src/test/util/setup_common.cpp:123 in 8aee8b42c3 outdated 
    111+        if (test_path.find(fs::path::preferred_separator) != std::string::npos) {
112+            throw std::runtime_error("BITCOIN_TEST_PATH cannot contain path separators");
113+        }
114+    }
115+    m_path_root = fs::temp_directory_path() / "test_common_" PACKAGE_NAME / test_path;
116+
    kouloumos commented at 9:51 am on February 2, 2023:

    Maybe give a hint to the user on where to look for the test directory, the same way that the functional test framework is doing it. It could be done with g_print_dir = true and then here

    0    if (std::getenv("BITCOIN_TEST_PATH") && g_print_dir) {
1        std::cout << "Test directory: " << m_path_root << "\n"
2        g_print_dir = false
3    }

    I was initially thinking that this could be part of the destructor but, per my other comment, that can lead to multiple redundant prints.

  22. kouloumos commented at 10:39 am on February 2, 2023: contributor

    Concept ACK I think this is useful; I recently wanted to read some logs from a benchmark and I wasn’t sure how to do it.

    This functionality is already possible with the functional tests using the –tmpdir= and –nocleanup arguments.

    But this (fixed data directory path by overriding) is something that is not currently possible in the functional tests framework. Which initially made me a bit skeptical, but the explanation of your flow makes sense, plus the fact that now the possible ~paths~ directories are confined inside /tmp/test_common_Bitcoin\ Core/.

    And since the BITCOIN_TEST_PATH environmental variable does not define a path anymore but a directory, maybe BITCOIN_TEST_DIR could be a better name?

  23. achow101 commented at 4:02 pm on April 25, 2023: member
    Are you still working on this?
  24. LarryRuane force-pushed on Apr 27, 2023
  25. LarryRuane commented at 5:00 pm on April 27, 2023: contributor
    Yes, still hoping to get this merged. Force-pushed 5df0ed5ab789e005cb9aa0fc3ab61b7ce0b80ec8 to rebase onto the latest master (since it’s been a long time)
  26. LarryRuane force-pushed on Apr 27, 2023
  27. LarryRuane commented at 5:12 pm on April 27, 2023: contributor

    Another force push to improve the doc slightly and to address review comments, thanks @kouloumos!

    Ready for review, also ping @theStack.

  28. DrahtBot added the label Needs rebase on May 9, 2023
  29. LarryRuane force-pushed on Jun 11, 2023
  30. DrahtBot removed the label Needs rebase on Jun 11, 2023
  31. LarryRuane commented at 10:38 pm on June 11, 2023: contributor
    Rebased for merge conflict.
  32. DrahtBot added the label CI failed on Aug 30, 2023
  33. DrahtBot removed the label CI failed on Sep 4, 2023
  34. achow101 requested review from stickies-v on Sep 20, 2023
  35. achow101 requested review from josibake on Sep 20, 2023
  36. DrahtBot added the label CI failed on Oct 17, 2023
  37. DrahtBot removed the label CI failed on Oct 21, 2023
  38. DrahtBot added the label CI failed on Nov 26, 2023
  39. maflcko commented at 1:34 pm on November 27, 2023: member
    Could rebase for fresh CI?
  40. LarryRuane force-pushed on Nov 28, 2023
  41. in doc/benchmarking.md:45 in 4be0734ff8 outdated 
    40+`test_common_Bitcoin Core/`, which is within the system's temporary directory
41+(for example, `/tmp`). By default the directory is
42+named as a random string, but this can be
43+overridden with the `BITCOIN_TEST_PATH` environment variable.
44+Be careful not to run multiple simultaneous instances of `bench_bitcoin`
45+that share the same test data directory; they'll conflict with each other.
    furszy commented at 10:21 pm on November 28, 2023:
    What about creating a pid file during init and removing it during shutdown? Failing early if the file already exist.
    LarryRuane commented at 10:38 pm on November 28, 2023:
    Yes, perhaps even better, maybe the test startup code can call LockDirectory() like bitcoind does. I’ll see if I can get that to work, thanks for the suggestion!
  42. DrahtBot removed the label CI failed on Nov 29, 2023
  43. LarryRuane commented at 5:02 pm on November 29, 2023: contributor
    Added commit ed37d296ba0a17daaf3334a011398ce05b30d69c to implement review suggestion #26564 (review), thank you, @furszy!
  44. DrahtBot added the label Needs rebase on Nov 29, 2023
  45. DrahtBot added the label CI failed on Nov 29, 2023
  46. LarryRuane force-pushed on Nov 29, 2023
  47. LarryRuane force-pushed on Nov 29, 2023
  48. LarryRuane commented at 9:07 pm on November 29, 2023: contributor
    Pushed a4bd4c490c140b7f59dc98664f1a0996087cf43d to fix merge conflict, and 567ce8bfecfc674b8f68613bfdfe6ddd55310769 to fix Windows CI failure (I think the problem was that we were trying to remove the data directory while still holding the .lock file open, which isn’t allowed in Windows).
  49. RandyMcMillan commented at 9:49 pm on November 29, 2023: contributor
    Concept ACK
  50. DrahtBot removed the label CI failed on Nov 29, 2023
  51. DrahtBot removed the label Needs rebase on Nov 29, 2023
  52. furszy commented at 3:02 pm on December 3, 2023: member

    Instead of introducing the new environment variable, could enable the existing -datadir=<path> arg capability in this way https://github.com/furszy/bitcoin-core/commit/135147aaa69f40aa59109f1b6f1760a125ed36e0 for the unit test framework.

    Example of its usage:

    0/test_bitcoin --run_test=txindex_tests -- -datadir="/path/to/dir"

    I think it is simpler and easier to remember as it does not change the way people/we use any core binary. Moreover, expanding any other binary with this feature isn’t hard. Just need to expand the expected args (let me know if you want it, and could do it as well).

    Feel free to take the commit if you like it.

    Update: Just realized that the commit needs a bit more work. Will update it soon.

  53. LarryRuane force-pushed on Dec 7, 2023
  54. LarryRuane renamed this:
    test: allow BITCOIN_TEST_PATH to specify working dir
    test: test_bitcoin: allow -testdatadir=<datadir>
    on Dec 8, 2023
  55. LarryRuane commented at 0:20 am on December 8, 2023: contributor
    Thanks, @furszy, I reworked your commit a bit, that was a big help! It did mean dropping support for fuzz, bench_bitcoin, and test_bitcoin-qt but this change really only benefits test_bitcoin (in my view). This PR is ready to review. Note the updated PR title and description.
  56. furszy commented at 0:38 am on December 8, 2023: member
    Cool, great you liked it. Will review it soon. Ended up connecting this work to #28955#pullrequestreview-1766044359.
  57. in src/test/util/setup_common.cpp:159 in 5dd89adbcb outdated 
    142+            // Don't allow anything but a simple string (a single path component) because we're
143+            // going to remove this path, and it would be bad if the string was "../../home".
144+            if (testdatadir.find(fs::path::preferred_separator) != std::string::npos) {
145+                std::cout << "testdatadir cannot contain path separators: " << testdatadir << std::endl;
146+                exit(1);
147+            }
    furszy commented at 1:13 am on December 8, 2023:

    In 5dd89adbc: Can drop this now. Custom test data directories are not deleted anymore.

    This is good for running test on a different storage location. Not only on the OS temporary files directory.

    LarryRuane commented at 5:14 pm on December 8, 2023:
    We do remove the custom directory at the beginning of the test: https://github.com/bitcoin/bitcoin/pull/26564/files#diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6R164 so I think this restriction is still needed. I just verified that /tmp/test_common_Bitcoin\ Core can be a symlink, so that would allow you to use any location, right? (I could add that to the README but not sure it’s worth it.)
    furszy commented at 8:00 pm on December 8, 2023:

    We do remove the custom directory at the beginning of the test: https://github.com/bitcoin/bitcoin/pull/26564/files#diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6R164 so I think this restriction is still needed. I just verified that /tmp/test_common_Bitcoin\ Core can be a symlink, so that would allow you to use any location, right? (I could add that to the README but not sure it’s worth it.)

    hmm, that wasn’t there before. Why we should introduce it?.

    We have two cases, default and custom dir path. The default test behavior creates a random directory path within the OS temp directory; therefore, we don’t need to remove anything on this execution path. Regarding the custom path functionality, I’m not entirely certain we should assign core the responsibility of cleaning up the custom directory before executing the test/s. I think we should just try to lock the folder and run the test, similar to what we do for bitcoind. Running tests on different paths, not on the temp directory, would be helpful. Doing it has no risk if we don’t remove the directory after running the test there. And I think that users who make use of the custom path functionality should be aware of what they are doing

    LarryRuane commented at 9:42 pm on December 10, 2023:

    The default test behavior creates a random directory path within the OS temp directory; therefore, we don’t need to remove anything on this execution path.

    No, in current master, the test framework does delete the directory after the test: https://github.com/bitcoin/bitcoin/blob/3e691258d8789a4a89cce42e7e71b130491594d7/src/test/util/setup_common.cpp#L162 and I think that’s a good thing or else running many tests could fill up your temp directory. If the test stops unexpectedly, I think the directory isn’t removed, which is probably a good thing so that the cause of the failure can be investigated.

    The directory doesn’t need to be removed before the test; we can presume the directory doesn’t exist because of the random name. (This PR does attempt to remove the random directory, which does no harm but it’s not necessary.)

    Regarding the custom path functionality, I’m not entirely certain we should assign core the responsibility of cleaning up the custom directory before executing the test/s.

    No, it does need to, because if a (populated) directory exists when the test begins, the test will likely fail because of this unexpected leftover state.

    furszy commented at 1:50 pm on December 12, 2023:

    I was talking about what we do before the test. The test cleanup after execution is fine for the default case.

    I think we shouldn’t block the test creation on different paths. Be forced to place the test only inside the temp directory doesn’t seems so useful to me. What if we make the -testdatadir arg the base path for all test cases, and create a subdirectory for each run test inside it?. (with this, you would not need to delete the datadir at the beginning, because it will not exist). E.g.

     0fs::path base_dir;
 1if (!m_node.args->IsArgSet("-testdatadir")) {
 2    base_dir = fs::temp_directory_path();
 3} else {
 4    std::string testdatadir = m_node.args->GetArg("-testdatadir").value();
 5    // Allow only a simple string (a single path component) because we're
 6    // going to remove this path, and it would be bad if it was "../../home".
 7    if (testdatadir.find(fs::path::preferred_separator) != std::string::npos) {
 8        std::cerr << "-testdatadir cannot contain path separators: " << testdatadir << std::endl;
 9        exit(1);
10    }
11
12    // Allow only regular directory paths
13    if (fs::is_regular_file(testdatadir) || fs::is_symlink(testdatadir)) {
14        std::cerr << "-testdatadir needs to be a directory path: " << testdatadir << std::endl;
15        exit(1);
16    }
17
18    // Create base dir if needed
19    fs::create_directories(testdatadir);
20    base_dir = fs::path{testdatadir.c_str()};
21    m_has_custom_datadir = true;
22}
23
24// Create test dir within the base directory
25m_path_root = base_dir / "test_common_" PACKAGE_NAME / g_insecure_rand_ctx_temp_path.rand256().ToString().c_str();
    LarryRuane commented at 10:54 pm on December 12, 2023:

    This defeats the purpose of this pull because it generates a randomly named data directory path; the point of this PR is to have a fixed datadir pathname so that I can have various files open in an editor across multiple test runs (not have to determine files’ locations and re-open them on each test run).

    Be forced to place the test only inside the temp directory doesn’t seems so useful to me.

    Well, if we want the datadir pathnames to be static (which is what I’m trying to accomplish with this PR), then we must delete the datadir before the test begins. The first version of this PR did indeed allow the datadir to be anywhere, but this comment: #26564 (comment) points out that it’s dangerous to recursively remove a directory (because of the possibility of user error) – but within /tmp, and especially within /tmp/test_common_Bitcoin\ Core/, recursive directory remove is okay, not dangerous at all.

    And, again, the user can set up a symlink from /tmp/test_common_Bitcoin\ Core/ to anywhere, if there’s a desire not to use /tmp for some reason (but that should be rare). That reintroduces the danger, of course, but that’s why we won’t even document setting up a symlink.

    furszy commented at 2:12 am on December 15, 2023:

    And, again, the user can set up a symlink from /tmp/test_common_Bitcoin\ Core/ to anywhere, if there’s a desire not to use /tmp for some reason (but that should be rare).

    Have you seen #26684#pullrequestreview-1566578747 and #28955 (comment)? It seems that we need it.

    Well, if we want the datadir pathnames to be static (which is what I’m trying to accomplish with this PR), then we must delete the datadir before the test begins. The first version of this PR did indeed allow the datadir to be anywhere, but this comment: #26564 (comment) points out that it’s dangerous to recursively remove a directory (because of the possibility of user error) – but within /tmp, and especially within /tmp/test_common_Bitcoin\ Core/, recursive directory remove is okay, not dangerous at all.

    Yes. I understood why you added it and the history. What I proposed was a middle ground between the “only run tests in a /temp/ directory” option and the “run test on any custom path” option. It was an option to allow users to provide the base directory path where the test will be created and run without being deleted after it execution.

    Other than that, I’m just not sure why we need to delete the directory path recursively at the beginning of the test. Why we cannot delete only what the test will use? or.. don’t delete anything and move such responsibility to the user. — Also, I have to admit that I’m not a fan of the symlink approach but.. thats a different topic —

    LarryRuane commented at 7:17 pm on December 15, 2023:

    I hadn’t seen those PRs, thanks. I tried running test_bitcoin with the TMPDIR environment variable set (on Linux), and it works, so that means you don’t have to set up a symlink.

    or.. don’t delete anything and move such responsibility to the user.

    Yes, I like that suggestion. I had forgotten that vscode (which is where I run the debugger from) allows one to set up an arbitrary “task” to run, preLaunchTask, before starting the executable, and this can remove the (fixed-named) data directory, so that’s perfect. Much less dangerous than recursive remove.

    So what do you think about this:

    • change the argument name to -datadir= instead of -testdatadir= (since the argument will have the same meaning as for other executables like bitcoind)
    • if -datadir is not specified, behave as today
    • have the datadir argument be the actual full pathname to use for the data directory (can have path separators, and be a relative name or absolute path, we don’t care)
    • don’t delete this directory before starting the test (could possibly warn if it’s non-empty? it really should be empty when we start)
    • if -datadir= is specified, don’t delete the directory after the test completes.
    furszy commented at 8:23 pm on December 15, 2023:
    Sounds good to me. Probably the only scenario we may want to abort running a test on a specific datadir is when the directory contain mainnet data. But it shouldn’t be a problem as we should be having something to detect this scenario inside init.cpp already.
    LarryRuane commented at 11:25 pm on December 15, 2023:

    That’s an excellent point, but I doubt there is anything already present that will prevent this.

    Here are two ideas, what do you think?

    • Whatever argument is given to -datadir=, always make (if necessary) and use a subdirectory (within the specified directory) called test_bitcoin. That way, even if the user specifies their real data directory, all the test stuff will be within this subdirectory. (It’s similar to how there already are subdirectories for testnet, signet, and regtest.)
    • make the argument keyword -testdatadir= (like it is now) instead of -datadir= (which might lead someone to think of specifying the real data directory path).

    The first option is probably safer. Actually, I might do both, because this -datadir argument would behave a little differently (by making that subdirectory) than what people may be expecting. Also, there’s a technical reason why -datadir= is a little more difficult to do (I won’t bother to explain it right here).

    If I have a chance, I’ll see if you’re correct that it’s already prevented.

    furszy commented at 3:43 pm on December 16, 2023:

    Whatever argument is given to -datadir=, always make (if necessary) and use a subdirectory (within the specified directory) called test_bitcoin. That way, even if the user specifies their real data directory, all the test stuff will be within this subdirectory. (It’s similar to how there already are subdirectories for testnet, signet, and regtest.)

    Sounds good. s/test_bitcoin/tests (we don’t use “signet_bitcoin”, “testnet_bitcoin”, “regtest_bitcoin”).

    Also, probably something for another PR but.. the following feature came to my mind: https://github.com/furszy/bitcoin-core/commits/2023_test_nocleanup/. Check the two commits there.

    LarryRuane commented at 8:25 pm on December 20, 2023:
    Thanks for the suggestion; I decided to name the extra path component test_temp instead of tests, because if the user thought it was okay to specify their home directory, they may have a directory there called tests with some source code. This name isn’t likely to conflict, and the temp part of the name indicates that it’s a temporary directory (unlike signet, regtest, etc., which make sense to persist).
  58. in doc/benchmarking.md:16 in 0324942a74 outdated 
    13 warns if you configure with `--enable-debug`, but consider if building without
14 it will impact the benchmark(s) you are interested in by unlatching log printers
15 and lock analysis.
16 
17-    make -C src bitcoin_bench
18+    make -C src bench/bench_bitcoin
    maflcko commented at 9:35 am on December 8, 2023:
    Maybe submit this (bugfix? or change?) in a separate pull?
    LarryRuane commented at 4:49 pm on December 8, 2023:
    Good idea, will do, probably best not to sneak in something unrelated even if it is small. I’ll remove this from this PR. Thanks!
  59. LarryRuane force-pushed on Dec 8, 2023
  60. LarryRuane commented at 6:36 pm on December 8, 2023: contributor
    Force pushed to remove the fix to doc/benchmarking.md as suggested by #26564 (review)
  61. in src/test/util/setup_common.cpp:142 in 56bcaf03f5 outdated 
    140+        } else {
141+            testdatadir = m_node.args->GetArg("-testdatadir").value();
142+            // Don't allow anything but a simple string (a single path component) because we're
143+            // going to remove this path, and it would be bad if the string was "../../home".
144+            if (testdatadir.find(fs::path::preferred_separator) != std::string::npos) {
145+                std::cout << "testdatadir cannot contain path separators: " << testdatadir << std::endl;
    furszy commented at 1:51 pm on December 12, 2023:
    this should use std::cerr
  62. LarryRuane force-pushed on Dec 13, 2023
  63. LarryRuane commented at 2:37 am on December 13, 2023: contributor
    Force pushed suggestion by @furszy (thanks!) #26564 (review)
  64. DrahtBot added the label CI failed on Dec 14, 2023
  65. LarryRuane marked this as a draft on Dec 17, 2023
  66. LarryRuane marked this as ready for review on Dec 20, 2023
  67. LarryRuane force-pushed on Dec 20, 2023
  68. LarryRuane commented at 8:50 pm on December 20, 2023: contributor
    Force pushed the changes described above (https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1428581458( but I don’t understand why CI is failing. All the failures are that LockDirectory() is not found, but it’s declared in util/fs_helpers.h and that file is included. It builds for me. @furszy, can you try building this branch? Thanks.
  69. furszy commented at 10:48 pm on December 20, 2023: member

    Force pushed the changes described above (#26564 (comment) but I don’t understand why CI is failing. All the failures are that LockDirectory() is not found, but it’s declared in util/fs_helpers.h and that file is included. It builds for me. @furszy, can you try building this branch? Thanks.

    Without compiling, would say that you are missing the namespace. util::LockDirectory() instead of LockDirectory().

  70. LarryRuane commented at 6:07 am on December 21, 2023: contributor
    Yes, that’s what the compiler error says, but that function doesn’t seem to be in that namespace. Its other callers don’t qualify it, and if I do qualify it, the build fails for me.
  71. furszy commented at 12:24 pm on December 21, 2023: member

    Yes, that’s what the compiler error says, but that function doesn’t seem to be in that namespace. Its other callers don’t qualify it, and if I do qualify it, the build fails for me.

    Seems that the function was moved to the util namespace on #28075. Will need to do some modifications here. LockDirectory() does not return a boolean anymore. Try cleaning up your project and rebasing it on master. (git clean -fdx && git pull –rebase origin master && make). Probably thats what the CI does.

  72. LarryRuane force-pushed on Dec 21, 2023
  73. DrahtBot removed the label CI failed on Dec 21, 2023
  74. LarryRuane force-pushed on Dec 21, 2023
  75. LarryRuane commented at 9:11 pm on December 21, 2023: contributor
    Force pushed twice to rebase and fix the CI problem, ready for review, thanks, @furszy!
  76. in src/test/util/setup_common.cpp:125 in 9d8f12146b outdated 
    123-    m_args.ForceSetArg("-datadir", fs::PathToString(m_path_root));
124-    gArgs.ForceSetArg("-datadir", fs::PathToString(m_path_root));
125     gArgs.ClearPathCache();
126     {
127         SetupServerArgs(*m_node.args);
128+        m_node.args->AddArg("-testdatadir", "Custom data directory (default: <random_string>)", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
    furszy commented at 2:42 am on December 29, 2023:

    Could mention that the datadir will not be removed automatically.

    Also, the default value isn’t just a random string. It should be:

    0        m_node.args->AddArg("-testdatadir", strprintf("Custom data directory (default: %s<random_string>)", fs::PathToString(fs::temp_directory_path() / "test_common_" PACKAGE_NAME / "")), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
  77. in src/test/util/setup_common.cpp:135 in 9d8f12146b outdated 
    133         }
134     }
135+
136+    if (!m_node.args->IsArgSet("-testdatadir")) {
137+        // By default, the data directory has a random name
138+        const std::string testdatadir{g_insecure_rand_ctx_temp_path.rand256().ToString().c_str()};
    furszy commented at 2:57 am on December 29, 2023:

    Let’s avoid using the string pointer (c_str()), could instead use:

    0testdatadir = fs::PathFromString(g_insecure_rand_ctx_temp_path.rand256().ToString())
  78. in src/test/util/setup_common.cpp:140 in 9d8f12146b outdated 
    138+        const std::string testdatadir{g_insecure_rand_ctx_temp_path.rand256().ToString().c_str()};
139+        m_path_root = fs::temp_directory_path() / "test_common_" PACKAGE_NAME / testdatadir;
140+    } else {
141+        // Custom data directory
142+        const std::string testdatadir{m_node.args->GetArg("-testdatadir").value()};
143+        m_path_root = fs::path(testdatadir.c_str()) / "test_temp";
    furszy commented at 3:02 am on December 29, 2023:
    Same here; let’s avoid using the string pointer c_str(), and instead use fs::PathFromString(path)
    LarryRuane commented at 6:42 am on December 29, 2023:
    I can’t get this to work.
    furszy commented at 1:32 pm on December 29, 2023: 
    0fs::path dir = m_node.args->GetPathArg("-testdatadir");
1// todo: check if path is valid
2m_path_root = dir / "test_temp";
  79. in src/test/util/setup_common.cpp:148 in 9d8f12146b outdated 
    146+        // Print the test directory name if custom, but only once per test run
147+        static bool g_printed_dir{false};
148+        if (!g_printed_dir) {
149+            g_printed_dir = true;
150+            std::cout << "Test directory (will not be deleted): " << m_path_root << std::endl;
151+        }
    furszy commented at 3:10 am on December 29, 2023:
    Is this because you are running all tests within a boost fixture test suite or is it happening when you run a single test case? E.g. fixture test suite: ./test_bitcoin --run_test=db_tests vs single test case ./test_bitcoin --run_test= db_tests/getwalletenv_file.
    LarryRuane commented at 6:42 am on December 29, 2023:
    The g_printed_dir guard is only needed because the user may be running more than one test case (like your first example). I don’t love this global variable but it seems weird to print this out on every individual test case, and I think it’s okay for a test program. I convinced myself that no race condition is possible, but we could use std::call_once() here but I didn’t think it was worth it.
    furszy commented at 9:26 pm on December 29, 2023:

    The g_printed_dir guard is only needed because the user may be running more than one test case

    This would break the PR goal in the current implementation. Only the last executed test datadir would be kept, the rest will be deleted. I think we should include the test name in the path and create a datadir directory for each executed tests. Could pull most of what I did before: 53eb4980d561491798d0c8b0ee383c83ba193432.

  80. in src/test/util/setup_common.cpp:139 in 9d8f12146b outdated 
    137+        // By default, the data directory has a random name
138+        const std::string testdatadir{g_insecure_rand_ctx_temp_path.rand256().ToString().c_str()};
139+        m_path_root = fs::temp_directory_path() / "test_common_" PACKAGE_NAME / testdatadir;
140+    } else {
141+        // Custom data directory
142+        const std::string testdatadir{m_node.args->GetArg("-testdatadir").value()};
    furszy commented at 3:15 am on December 29, 2023:

    Can use GetPathArg("-testdatadir") directly.

    Also, I would check the path correctness here and error out early when it is invalid.

    LarryRuane commented at 6:44 am on December 29, 2023:
    Good idea, but what do you mean by path correctness?
    furszy commented at 1:35 pm on December 29, 2023:

    Good idea, but what do you mean by path correctness?

    Probably we should fail when -testdatadir is an empty string.

  81. in src/test/util/setup_common.cpp:166 in e00c75df8b outdated 
    161+        lock_datadir();
162+
163+        // Always start with a fresh data directory (must release the lock before remove on some platforms).
164+        ReleaseDirectoryLocks();
165         fs::remove_all(m_path_root);
166+        lock_datadir();
    furszy commented at 3:27 am on December 29, 2023:
    If a concurrent process locks the datadir right after the lock release, this process would remove the root path even when the other process is using it?
    LarryRuane commented at 6:50 am on December 29, 2023:
    Good catch, I was actually aware of this but concluded that it is such a small timing window, that it’s not a problem in practice. The problem is that on Windows, you can’t remove an open file (and having a file lock is a form of having the file open). I could make this conditional on WIN32 (I think it is), but I’m not sure that’s even worth it.
  82. furszy commented at 3:27 am on December 29, 2023: member
    Left few comments
  83. in src/test/util/setup_common.cpp:153 in e00c75df8b outdated 
    151+                fs::create_directories(m_path_root);
152+            } catch (const std::exception&) {
153+            }
154+            if (!fs::exists(m_path_root)) {
155+                std::cerr << "Cannot create test data directory " + fs::PathToString(m_path_root) + '\n';
156+                exit(1);
    furszy commented at 1:38 pm on December 29, 2023:

    This should use the EXIT_FAILURE constant, not exit(1).

    Also, I would move the print + exit to a standalone function to dedup code. Like Shutdown(const std::string& err_msg);.

  84. LarryRuane marked this as a draft on Dec 29, 2023
  85. DrahtBot added the label CI failed on Jan 13, 2024
  86. LarryRuane marked this as ready for review on Jan 22, 2024
  87. LarryRuane force-pushed on Jan 22, 2024
  88. LarryRuane force-pushed on Jan 22, 2024
  89. LarryRuane force-pushed on Jan 22, 2024
  90. LarryRuane force-pushed on Jan 23, 2024
  91. DrahtBot removed the label CI failed on Jan 24, 2024
  92. in src/test/util/setup_common.cpp:162 in 3a298b2a86 outdated 
    148+            std::cerr << "-testdatadir argument is empty, please specify a path\n";
149+            exit(EXIT_FAILURE);
150+        }
151+        const std::string test_path{G_TEST_GET_FULL_NAME ? G_TEST_GET_FULL_NAME() : ""};
152+        const fs::path lockdir{root_dir / "test_temp" / fs::PathFromString(test_path)};
153+        m_path_root = lockdir / "datadir";
    furszy commented at 0:35 am on February 7, 2024:
    What is the purpose of this extra levels? This can just be root_dir / G_TEST_GET_FULL_NAME().
    LarryRuane commented at 7:56 pm on February 8, 2024:

    It’s because after successfully locking the lockdir directory, I don’t want to release the lock (you had caught that problem earlier), which means we can’t delete .lock, but I do want to start with a clean data directory. This extra level makes it easy to delete the old data directory (if it exists) without disturbing the .lock file so it can remain locked the entire time.

    The alternative would be to iterate over the contents of the data directory delete (call fs::remove_all()) on everything except .lock. I think the added code complexity isn’t worth it, but that’s a judgment call.

    furszy commented at 1:02 pm on February 21, 2024:

    It’s because after successfully locking the lockdir directory, I don’t want to release the lock (you had caught that problem earlier), which means we can’t delete .lock, but I do want to start with a clean data directory. This extra level makes it easy to delete the old data directory (if it exists) without disturbing the .lock file so it can remain locked the entire time.

    What if we use the test group directory level instead?. See 6697933c. It locks the entire test group. Also, 6697933c changes the “/test_temp/” to “/test_common_” PACKAGE_NAME /" to keep the same structure.

    Update: I’m not really happy with this suggestion anymore. It will block concurrent sub-tests runs.. (but, I’m still thinking that “test_temp” should be replaced for “/test_common_” PACKAGE_NAME /")

    cbergqvist commented at 8:12 pm on February 21, 2024:

    What is the purpose of this extra levels? This can just be root_dir / G_TEST_GET_FULL_NAME().

    Also reacted to this, but another bonus of having the intermediate test_temp dir is that one doesn’t unintentionally flood an already existing directory with directories based on test-names.

  93. in src/test/util/setup_common.cpp:156 in 3a298b2a86 outdated 
    154+
155+        // Try to obtain the lock; if unsuccessful don't disturb the existing test.
156+        try {
157+            fs::create_directories(lockdir);
158+        } catch (const std::exception&) {
159+        }
    furszy commented at 0:43 am on February 7, 2024:

    Guess that you are duplicating TryCreateDirectories()?

    You could just try to create the directory and lock it right away. E.g.

    0TryCreateDirectories(dir);
1if (util::LockDirectory(dir, ".lock", /*probe_only=*/false) != util::LockResult::Success) {
2   // error out
3}
4etc..

    Shouldn’t need to execute the fs::exist call.

  94. in src/test/util/setup_common.cpp:169 in 3a298b2a86 outdated 
    167+            exit(EXIT_FAILURE);
168+        }
169+
170+        // Always start with a fresh data directory; this doesn't delete the .lock file.
171+        fs::remove_all(m_path_root);
172+        fs::create_directories(m_path_root);
    furszy commented at 0:46 am on February 7, 2024:
    If this doesn’t delete the .lock file, then the fs::create_directories call isn’t needed?
    LarryRuane commented at 8:07 pm on February 8, 2024:
    It’s needed because fs::remove_all(m_path_root) removes m_path_root (and everything below), so we must re-create m_path_root (only the last component of the pathname will need to actually be created). I’ll change the fs::create_directories() to TryCreateDirectories() (even though it shouldn’t matter here).
  95. LarryRuane force-pushed on Feb 8, 2024
  96. LarryRuane commented at 8:11 pm on February 8, 2024: contributor
    Thanks for the comments, furszy, force-pushed 7b81dea7eb6b14a939230477835159dad6b4b75b
  97. tdb3 commented at 0:34 am on February 19, 2024: contributor

    Nice feature. Concept ACK.

    Ran a few practical tests on 7b81dea7eb6b14a939230477835159dad6b4b75b. Unless I’m overlooking something, seems like there may be an issue with some tests when testdatadir is provided a relative path (i.e. non-absolute path).

    1. Ran all unit tests, without -testdatadir. All passed. Verified /tmp/test_common_Bitcoin Core was empty afterward.
    2. Ran all unit tests, with -testdatadir=/absolute/path/to/mytestdir/newlycreateddir/, where mytestdir was created ahead of time and newlycreateddir was not. All passed. Confirmed that /tmp/test_common_Bitcoin Core was not created and directories within mytestdir/newlycreateddir/test_temp/<test> were present after test execution.
    3. Ran all unit tests, with -testdatadir=./mytestdir/newlycreateddir/. Received an error on walletdb_tests/walletdb_read_write_deadlock, both when running all tests, and also when running just walletdb_tests/walletdb_read_write_deadlock. The test passed without the testdatadir argument present, and also passed when an absolute path was specified with testdatadir . Maybe I’m missing something simple?
    0$test_bitcoin --run_test=walletdb_tests/walletdb_read_write_deadlock -- -testdatadir="./mytestdir/newlycreateddir/"
1Running 1 test case...
2Test directory (will not be deleted): "mytestdir/newlycreateddir/test_temp/walletdb_tests/walletdb_read_write_deadlock/datadir"
3wallet/test/walletdb_tests.cpp(42): error: in "walletdb_tests/walletdb_read_write_deadlock": check status == DatabaseStatus::SUCCESS has failed [8 != 0]
4test_bitcoin: ./wallet/wallet.h:443: virtual wallet::WalletDatabase& wallet::CWallet::GetDatabase() const: Assertion `static_cast<bool>(m_database)' failed.
5unknown location(0): fatal error: in "walletdb_tests/walletdb_read_write_deadlock": signal: SIGABRT (application abort requested)
6wallet/test/walletdb_tests.cpp(50): last checkpoint
7
8*** 2 failures are detected in the test module "Bitcoin Core Test Suite"

    and from the debug.log (feature already proving useful!):

    02024-02-19T00:29:48.224636Z [test] [wallet/bdb.cpp:315] [Verify] Using wallet mytestdir/newlycreateddir/test_temp/walletdb_tests/walletdb_read_write_deadlock/datadir/wallet_0_.dat/wallet.dat
12024-02-19T00:29:48.224786Z [test] [wallet/bdb.cpp:161] [Open] BerkeleyEnvironment::Open: LogDir=mytestdir/newlycreateddir/test_temp/walletdb_tests/walletdb_read_write_deadlock/datadir/wallet_0_.dat/database ErrorFile=mytestdir/newlycreateddir/test_temp/walletdb_tests/walletdb_read_write_deadlock/datadir/wallet_0_.dat/db.log
22024-02-19T00:29:48.227776Z [test] [wallet/bdb.cpp:189] [Open] BerkeleyEnvironment::Open: Error -30974 opening database environment: DB_RUNRECOVERY: Fatal error, run database recovery
  98. in src/test/util/setup_common.cpp:171 in 7b81dea7eb outdated 
    159+                             "The test executable is probably already running.\n";
160+            exit(EXIT_FAILURE);
161+        }
162+
163+        // Always start with a fresh data directory; this doesn't delete the .lock file.
164+        fs::remove_all(m_path_root);
    cbergqvist commented at 2:01 pm on February 21, 2024:

    Why not pass lock_dir here into fs::remove_all() instead of the datadir-subdirectory m_path_root? Seems like current version does NOT remove old test directories, just the datadir.

    This contradicts the PR summary:

    When the test starts, remove /test_temp/ if it exists from an earlier run (currently, it’s presumed not to exist due to the long random string)

    Current (7b81dea) behavior:

     0$ ./src/test/test_bitcoin --run_test=getarg_tests/doubledash -- --testdatadir=/tmp/
 1...
 2
 3$ touch /tmp/test_temp/getarg_tests/doubledash/foo
 4$ touch /tmp/test_temp/getarg_tests/doubledash/datadir/bar
 5
 6$ ./src/test/test_bitcoin --run_test=getarg_tests/doubledash -- --testdatadir=/tmp/
 7...
 8
 9$ ls /tmp/test_temp/getarg_tests/doubledash/foo
10/tmp/test_temp/getarg_tests/doubledash/foo
11
12$ ls /tmp/test_temp/getarg_tests/doubledash/datadir/bar
13ls: cannot access '/tmp/test_temp/getarg_tests/doubledash/datadir/bar': No such file or directory
    davidgumberg commented at 2:26 am on February 29, 2024:

    @cbergqvist As I understand, the reasoning is hinted at by the line 161 comment, namely the reason for the extra parent (lockdir) of our m_path_root is solely for us to grab a lock on the lockdir, and once we’ve obtained it, safely delete and write to the nested datadir. test_bitcoin will never write to anywhere other than the datadir.

    As discussed at: #26564 (review) and #26564 (review) :

    “The problem is that on Windows, you can’t remove an open file (and having a file lock is a form of having the file open).”

    cbergqvist commented at 7:33 am on February 29, 2024:
    You are probably correct, I meant it more as a change in that direction, but probably would have required a slightly different solution. @LarryRuane addressed my comment here: #26564 (comment)
  99. cbergqvist commented at 7:06 pm on February 21, 2024: none
    Generally great debugging UX improvement in my eyes!
  100. LarryRuane commented at 5:22 pm on February 22, 2024: contributor

    may be an issue with some tests when testdatadir is provided a relative path

    I can’t reproduce this problem. I tried both running the one test alone and running the entire test suite (not specifying --run_test=).

  101. LarryRuane commented at 5:43 pm on February 22, 2024: contributor

    This contradicts the PR summary:

    Thanks for the review! You’re right, good catch. I updated the PR description.

    The behavior is correct, we don’t want to remove the .lock file at the start of the run, because we’ve just acquired the lock, and want to keep it while we delete the test’s data directory that might be left over from a previous run. That’s why there’s a datadir at the same level as .lock (so we can delete datadir without also deleting .lock). This behavior was suggested in this comment earlier in this PR: #26564 (review) (but I forgot to update the PR description).

  102. in src/test/util/setup_common.cpp:175 in 7b81dea7eb outdated 
    163+        // Always start with a fresh data directory; this doesn't delete the .lock file.
164+        fs::remove_all(m_path_root);
165+        TryCreateDirectories(m_path_root);
166+
167+        // Print the test directory name if custom.
168+        std::cout << "Test directory (will not be deleted): " << m_path_root << std::endl;
    cbergqvist commented at 5:53 pm on February 22, 2024:
    Suggestion: change to something like "Test directory (not deleted until next run): "
    LarryRuane commented at 6:28 pm on February 22, 2024:
    Thanks, I think it’s clear enough as is. If it says “until next run”, it might not be clear if that’s before the next run of any test or this particular test. Clarifying that would add more text and be a bit too chatty. I think the current text is clear enough to convey “not deleted after the current run”; I think it’s understood that if you specify the same data directory and the same test, it will overwrite that same directory.
    cbergqvist commented at 8:25 pm on February 22, 2024:

    Last suggestion: "Test directory (remains after run)".

    If it hadn’t been so chatty compared to running without -testdatadir I wouldn’t mind. Would have been nicer to explain the part within the parenthesis one single time, before all the tests are run.

    LarryRuane commented at 6:38 pm on February 23, 2024:

    Last suggestion: "Test directory (remains after run)".

    I’m not sure if I see that as an improvement; I’ll hold off to see if anyone else has a preference.

    Would have been nicer to explain the part within the parenthesis one single time, before all the tests are run.

    A previous iteration of this PR did this (before each test case had its own data directory), the code worked but was messy (required a static (global) variable), so I was glad to remove it which needed to be done anyway since now each test has its own data directory. I don’t really want to add that complexity back in. I think I’ll leave it as is unless someone has a strong opinion to the contrary.

  103. cbergqvist approved
  104. cbergqvist commented at 5:55 pm on February 22, 2024: none
    ACK 7b81dea. One follow-up suggestion.
  105. DrahtBot requested review from tdb3 on Feb 22, 2024
  106. DrahtBot requested review from kouloumos on Feb 22, 2024
  107. DrahtBot removed review request from tdb3 on Feb 22, 2024
  108. DrahtBot requested review from tdb3 on Feb 22, 2024
  109. DrahtBot removed review request from tdb3 on Feb 22, 2024
  110. DrahtBot requested review from tdb3 on Feb 22, 2024
  111. DrahtBot removed review request from tdb3 on Feb 22, 2024
  112. DrahtBot requested review from tdb3 on Feb 22, 2024
  113. DrahtBot removed review request from tdb3 on Feb 23, 2024
  114. DrahtBot requested review from tdb3 on Feb 23, 2024
  115. tdb3 commented at 6:49 pm on February 23, 2024: contributor

    may be an issue with some tests when testdatadir is provided a relative path

    I can’t reproduce this problem. I tried both running the one test alone and running the entire test suite (not specifying --run_test=).

    The specific command I used that caused the error was as follows. Only seems to happen when using a relative path for testdatadir.

    0src/test/test_bitcoin --run_test=walletdb_tests/walletdb_read_write_deadlock -- -testdatadir="./mytestdir/newlycreateddir/"

    I’m getting the same failure on two different machines (both running Ubuntu 22.04, one x86_64 and one aarch64). Here’s how I built Core (maybe something wrong here?):

    0make -C depends && ./autogen.sh && ./configure BDB_LIBS="-L${BDB_PREFIX}/lib -ldb_cxx-4.8" BDB_CFLAGS="-I${BDB_PREFIX}/include" --disable-bench --disable-fuzz-binary --enable-debug --without-gui --enable-suppress-external-warnings && make -j4 && make check
  116. DrahtBot removed review request from tdb3 on Feb 23, 2024
  117. DrahtBot requested review from tdb3 on Feb 23, 2024
  118. LarryRuane commented at 11:03 pm on February 23, 2024: contributor

    @tdb3 - Ah, I bet the problem relates to BDB. I don’t build that way. I don’t know what’s wrong, but just to verify one thing, the build document says:

    Note: Make sure that BDB_PREFIX is an absolute path.

    What is that environment variable set to in your environment? But if not, you may want to try that. (Having said this, if this is the problem, I don’t understand how this causes the problem.)

  119. DrahtBot removed review request from tdb3 on Feb 23, 2024
  120. DrahtBot requested review from tdb3 on Feb 23, 2024
  121. tdb3 commented at 11:06 pm on February 23, 2024: contributor

    @tdb3 - Ah, I bet the problem relates to BDB. I don’t build that way. I don’t know what’s wrong, but just to verify one thing, the build document says:

    Note: Make sure that BDB_PREFIX is an absolute path.

    What is that environment variable set to in your environment? But if not, you may want to try that. (Having said this, if this is the problem, I don’t understand how this causes the problem.)

    Hmm. BDB_PREFIX was an absolute path, and it was sanity checked with ls -l $BDB_PREFIX

  122. DrahtBot removed review request from tdb3 on Feb 23, 2024
  123. DrahtBot requested review from tdb3 on Feb 23, 2024
  124. LarryRuane commented at 11:24 pm on February 23, 2024: contributor

    @tdb3

    Hmm. BDB_PREFIX was an absolute path

    Draht, I’ll set up BDB on my system here and see if it reproduces. If not too much trouble and if you have time, maybe you can try building without BDB and see if that works.

  125. DrahtBot removed review request from tdb3 on Feb 23, 2024
  126. DrahtBot requested review from tdb3 on Feb 23, 2024
  127. LarryRuane force-pushed on Feb 24, 2024
  128. LarryRuane commented at 2:05 am on February 24, 2024: contributor
    @tdb3 I force-pushed a fix, it turns out that Berkeley DB (BDB) doesn’t work with a relative path, only absolute. So it’s a one-line fix. Thanks for finding this bug!
  129. tdb3 commented at 10:33 am on February 24, 2024: contributor

    @tdb3 I force-pushed a fix, it turns out that Berkeley DB (BDB) doesn’t work with a relative path, only absolute. So it’s a one-line fix. Thanks for finding this bug!

    Happy to help. Nice job with the fix. ACK 11aeabde29820ab218757a6c5b959f2ba2cd8f13.

    Retested on my machine as well (the same steps outlined above). All passed!

  130. DrahtBot removed review request from tdb3 on Feb 24, 2024
  131. DrahtBot requested review from cbergqvist on Feb 24, 2024
  132. cbergqvist commented at 1:31 pm on February 24, 2024: none

    Great find @tdb3! re-ACK 11aeabd.

    (Diffed against previous commit and re-ran ./src/test/test_bitcoin -- --testdatadir= with absolute and relative paths. Not explicitly opting in to BDB though).

  133. DrahtBot removed review request from cbergqvist on Feb 24, 2024
  134. davidgumberg commented at 3:05 am on February 29, 2024: contributor

    tACK https://github.com/bitcoin/bitcoin/pull/26564/commits/11aeabde29820ab218757a6c5b959f2ba2cd8f13

    Tested with the following script that runs every test suite with an absolute and a relative dir:

     0bitcoin_dir="$HOME/dev/bitcoin"
 1
 2test_absolute_dir="$HOME/dev/test_data"
 3test_relative_dir="test_data"
 4
 5for file in "$bitcoin_dir"/src/test/*_tests.cpp; do
 6    test_name=$(basename $file | sed 's/\.[^.]*$//')
 7
 8    $bitcoin_dir/src/test/test_bitcoin -t $test_name -- -testdatadir="$test_absolute_dir"
 9    if [ $? -ne 0 ]; then
10        echo $test_name failed with absolute dir $test_absolute_dir!
11        exit 1
12    fi
13
14    $bitcoin_dir/src/test/test_bitcoin -t $test_name -- -testdatadir="$test_relative_dir"
15    if [ $? -ne 0 ]; then
16        echo $test_name failed with relative dir $test_relative_dir!
17        exit 1
18    fi
  135. tdb3 commented at 5:04 am on March 1, 2024: contributor

    @tdb3 I force-pushed a fix, it turns out that Berkeley DB (BDB) doesn’t work with a relative path, only absolute. So it’s a one-line fix. Thanks for finding this bug!

    Not sure how to check if CI was passing before the force push, but if it was, maybe this is a gap in CI? Maybe an Issue should be written?

  136. LarryRuane commented at 1:30 am on March 3, 2024: contributor

    @tdb3

    Not sure how to check if CI was passing before the force push, but if it was, maybe this is a gap in CI? Maybe an Issue should be written?

    CI was passing before this fix, but in my opinion, CI (regression testing) doesn’t need to test test-only code. (Otherwise, we could end up with an infinite recursion of tests! :sweat_smile:). I think for a change like this, manual testing (exactly as you did) is sufficient.

  137. in src/test/util/setup_common.cpp:150 in 11aeabde29 outdated 
    148+            std::cerr << "-testdatadir argument is empty, please specify a path\n";
149+            exit(EXIT_FAILURE);
150+        }
151+        root_dir = fs::absolute(root_dir);
152+        const std::string test_path{G_TEST_GET_FULL_NAME ? G_TEST_GET_FULL_NAME() : ""};
153+        const fs::path lockdir{root_dir / "test_temp" / fs::PathFromString(test_path)};
    furszy commented at 1:22 am on March 4, 2024:
    Can we keep the same parent file? s/“test_temp”/“test_common_” PACKAGE_NAME
    LarryRuane commented at 6:39 pm on March 5, 2024:
    Yes, this makes sense, but it’s unfortunate that PACKAGE_NAME (Bitcoin Core) contains a space character. Still a good idea for consistency.
  138. furszy commented at 1:43 am on March 4, 2024: member

    utACK 11aeabde29. Left a comment.

    And also, have made few tiny changes. Take them if you like them:

     0diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
 1--- a/src/test/util/setup_common.cpp	(revision 11aeabde29820ab218757a6c5b959f2ba2cd8f13)
 2+++ b/src/test/util/setup_common.cpp	(date 1709516409185)
 3@@ -98,6 +98,20 @@
 4 };
 5 static NetworkSetup g_networksetup_instance;
 6 
 7+/** Register test-only arguments */
 8+static void SetupUnitTestArgs(ArgsManager& argsman)
 9+{
10+    argsman.AddArg("-testdatadir", strprintf("Custom data directory (default: %s<random_string>)", fs::PathToString(fs::temp_directory_path() / "test_common_" PACKAGE_NAME / "")),
11+                   ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
12+}
13+
14+/** Test setup failure */
15+static void ExitFailure(std::string_view str_err)
16+{
17+    std::cerr << str_err << "\n";
18+    exit(EXIT_FAILURE);
19+}
20+
21 BasicTestingSetup::BasicTestingSetup(const ChainType chainType, const std::vector<const char*>& extra_args)
22     : m_args{}
23 {
24@@ -123,8 +137,7 @@
25     gArgs.ClearPathCache();
26     {
27         SetupServerArgs(*m_node.args);
28-        m_node.args->AddArg("-testdatadir", strprintf("Custom data directory (default: %s<random_string>)", fs::PathToString(fs::temp_directory_path() / "test_common_" PACKAGE_NAME / "")),
29-                            ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
30+        SetupUnitTestArgs(*m_node.args);
31         std::string error;
32         if (!m_node.args->ParseParameters(arguments.size(), arguments.data(), error)) {
33             m_node.args->ClearArgs();
34@@ -141,10 +154,8 @@
35         // Custom data directory
36         m_has_custom_datadir = true;
37         fs::path root_dir{m_node.args->GetPathArg("-testdatadir")};
38-        if (root_dir.empty()) {
39-            std::cerr << "-testdatadir argument is empty, please specify a path\n";
40-            exit(EXIT_FAILURE);
41-        }
42+        if (root_dir.empty()) ExitFailure("-testdatadir argument is empty, please specify a path");
43+
44         root_dir = fs::absolute(root_dir);
45         const std::string test_path{G_TEST_GET_FULL_NAME ? G_TEST_GET_FULL_NAME() : ""};
46         const fs::path lockdir{root_dir / "test_temp" / fs::PathFromString(test_path)};
47@@ -153,14 +164,12 @@
48         // Try to obtain the lock; if unsuccessful don't disturb the existing test.
49         TryCreateDirectories(lockdir);
50         if (util::LockDirectory(lockdir, ".lock", /*probe_only=*/false) != util::LockResult::Success) {
51-            std::cerr << "Cannot obtain a lock on test data lock directory " + fs::PathToString(lockdir) + '\n' +
52-                             "The test executable is probably already running.\n";
53-            exit(EXIT_FAILURE);
54+            ExitFailure("Cannot obtain a lock on test data lock directory " + fs::PathToString(lockdir) + '\n' + "The test executable is probably already running.");
55         }
56 
57-        // Always start with a fresh data directory; this doesn't delete the .lock file.
58+        // Always start with a fresh data directory; this doesn't delete the .lock file located a level above.
59         fs::remove_all(m_path_root);
60-        TryCreateDirectories(m_path_root);
61+        if (!TryCreateDirectories(m_path_root)) ExitFailure("Cannot create test data directory");
62 
63         // Print the test directory name if custom.
64         std::cout << "Test directory (will not be deleted): " << m_path_root << std::endl;
  139. LarryRuane force-pushed on Mar 5, 2024
  140. LarryRuane commented at 9:39 pm on March 5, 2024: contributor
    Force pushed to accept review comments, thanks furszy
  141. furszy commented at 10:40 pm on March 5, 2024: member
    utACK 0e477344fe4094ada5262a1e7e465d4cb93cacdd
  142. DrahtBot requested review from davidgumberg on Mar 5, 2024
  143. DrahtBot requested review from tdb3 on Mar 5, 2024
  144. DrahtBot requested review from cbergqvist on Mar 5, 2024
  145. DrahtBot removed review request from davidgumberg on Mar 5, 2024
  146. DrahtBot removed review request from tdb3 on Mar 5, 2024
  147. DrahtBot removed review request from cbergqvist on Mar 5, 2024
  148. DrahtBot requested review from tdb3 on Mar 5, 2024
  149. DrahtBot requested review from davidgumberg on Mar 5, 2024
  150. DrahtBot requested review from cbergqvist on Mar 5, 2024
  151. marcofleon commented at 0:07 am on March 6, 2024: contributor

    Concept ACK. I’ve observed that running test_bitcoin without specifying a testdatadir passes and leaves no trace of a test directory. I’ve reviewed that running individual tests with a specific testdatadir results in the directory format that is described in src/test/README.md. I also tested the two exit failure messages.

    However, I’m encountering an error and I can’t figure out why. I might be overlooking something simple.

    I run ./test_bitcoin (all the tests) without testdatadir and everything works. But when I run

    0./test_bitcoin -- -testdatadir=./testingdir/newdir

    it always fails on rpc_tests/rpc_parse_monetary_values with the console error:

    0Test directory (will not be deleted): "/Users/mc/contrib/bitcoin/src/test/testingdir/test_common_Bitcoin Core/rpc_tests/rpc_parse_monetary_values/datadir"
1Error: A fatal internal error occurred, see debug.log for details
2Assertion failed: (status == node::ChainstateLoadStatus::SUCCESS), function LoadVerifyActivateChainstate, file setup_common.cpp, line 281.
3unknown location:0: fatal error: in "rpc_tests/rpc_parse_monetary_values": signal: SIGABRT (application abort requested)
4test/rpc_tests.cpp:277: last checkpoint: "rpc_parse_monetary_values" fixture ctor
5unknown location:0: fatal error: in "rpc_tests/rpc_ban": std::__1::ios_base::failure: Could not create TokenPipe: unspecified iostream_category error

    The tests prior to this one all succeed and create the proper directories. The rest of the tests after that fail with the unknown location error.

    I’ve tried it with an absolute path as well and the same thing happens. The thing is, when I run the rpc_tests suite or the single test individually, everything passes.

     02024-03-05T22:45:17.249171Z [test] [test/util/random.cpp:31] [Seed] Seed: Setting random seed for current tests to RANDOM_CTX_SEED=52e12d6667c3a8adebe9dbb0ada39c7d07c09ace1970adc18c604aae29be2049
 12024-03-05T22:45:17.249183Z [test] [init/common.cpp:155] [LogPackageVersion] Bitcoin Core version v26.99.0-5b73af0dcf3b (release build)
 22024-03-05T22:45:17.249241Z [test] [node/chainstatemanager_args.cpp:54] [ApplyArgsManOptions] Script verification uses 7 additional threads
 32024-03-05T22:45:17.249286Z [test] [kernel/context.cpp:20] [Context] Using the 'arm_shani(1way,2way)' SHA256 implementation
 42024-03-05T22:45:17.249437Z [test] [script/sigcache.cpp:104] [InitSignatureCache] Using 16 MiB out of 16 MiB requested for signature cache, able to store 524288 elements
 52024-03-05T22:45:17.249465Z [test] [validation.cpp:1885] [InitScriptExecutionCache] Using 16 MiB out of 16 MiB requested for script execution cache, able to store 524288 elements
 62024-03-05T22:45:17.249493Z [scheduler] [util/thread.cpp:20] [TraceThread] scheduler thread start
 72024-03-05T22:45:17.249535Z [test] [policy/fees.cpp:560] [CBlockPolicyEstimator] /Users/mc/contrib/bitcoin/src/test/testingdir/test_common_Bitcoin Core/rpc_tests/rpc_parse_monetary_values/datadir/fee_estimates.dat is not found. Continue anyway.
 82024-03-05T22:45:17.249586Z [test] [dbwrapper.cpp:249] [CDBWrapper] Opened LevelDB successfully
 92024-03-05T22:45:17.249594Z [test] [dbwrapper.cpp:274] [CDBWrapper] Using obfuscation key for /Users/mc/contrib/bitcoin/src/test/testingdir/test_common_Bitcoin Core/rpc_tests/rpc_parse_monetary_values/datadir/blocks/index: 0000000000000000
102024-03-05T22:45:17.249612Z [test] [node/chainstate.cpp:166] [LoadChainstate] Assuming ancestors of block 000000000000000000026811d149d4d261995ec5b3f64f439a0a10e1a464af9a have valid signatures.
112024-03-05T22:45:17.249617Z [test] [node/chainstate.cpp:170] [LoadChainstate] Setting nMinimumChainWork=000000000000000000000000000000000000000063c4ebd298db40af57541800
122024-03-05T22:45:17.249632Z [test] [dbwrapper.cpp:249] [CDBWrapper] Opened LevelDB successfully
132024-03-05T22:45:17.249637Z [test] [dbwrapper.cpp:274] [CDBWrapper] Using obfuscation key for /Users/mc/contrib/bitcoin/src/test/testingdir/test_common_Bitcoin Core/rpc_tests/rpc_parse_monetary_values/datadir/blocks/index: 0000000000000000
142024-03-05T22:45:17.249642Z [test] [node/blockstorage.cpp:505] [LoadBlockIndexDB] LoadBlockIndexDB: last block file = 0
152024-03-05T22:45:17.249648Z [test] [node/blockstorage.cpp:509] [LoadBlockIndexDB] LoadBlockIndexDB: last block file info: CBlockFileInfo(blocks=0, size=0, heights=0...0, time=1970-01-01...1970-01-01)
162024-03-05T22:45:17.249652Z [test] [node/blockstorage.cpp:520] [LoadBlockIndexDB] Checking all blk files are present...
172024-03-05T22:45:17.249656Z [test] [validation.cpp:4710] [LoadBlockIndex] Initializing databases...
182024-03-05T22:45:17.249669Z [test] [flatfile.cpp:44] [Open] Unable to open file /Users/mc/contrib/bitcoin/src/test/testingdir/test_common_Bitcoin Core/rpc_tests/rpc_parse_monetary_values/datadir/blocks/blk00000.dat
192024-03-05T22:45:17.249679Z [test] [flatfile.cpp:44] [Open] Unable to open file /Users/mc/contrib/bitcoin/src/test/testingdir/test_common_Bitcoin Core/rpc_tests/rpc_parse_monetary_values/datadir/blocks/blk00000.dat
202024-03-05T22:45:17.249684Z [test] [logging.h:269] [error] ERROR: WriteBlockToDisk: OpenBlockFile failed
212024-03-05T22:45:17.249688Z [test] [node/abort.cpp:22] [AbortNode] *** Failed to write block
222024-03-05T22:45:17.249694Z [test] [noui.cpp:43] [noui_ThreadSafeMessageBox] Error: A fatal internal error occurred, see debug.log for details
232024-03-05T22:45:17.249705Z [test] [logging.h:269] [error] ERROR: LoadGenesisBlock: writing genesis block to disk failed

    The error occurs when trying to create blk00000.dat. My next idea is to try running subsets of the test suites along with rpc_tests and see if there’s a certain suite that causes the issue. But for now, wanted to get feedback and see if there’s something more obvious that I’m missing.

  152. DrahtBot removed review request from tdb3 on Mar 6, 2024
  153. DrahtBot removed review request from davidgumberg on Mar 6, 2024
  154. DrahtBot removed review request from cbergqvist on Mar 6, 2024
  155. DrahtBot requested review from tdb3 on Mar 6, 2024
  156. DrahtBot requested review from cbergqvist on Mar 6, 2024
  157. DrahtBot requested review from davidgumberg on Mar 6, 2024
  158. DrahtBot removed review request from tdb3 on Mar 6, 2024
  159. DrahtBot removed review request from cbergqvist on Mar 6, 2024
  160. DrahtBot removed review request from davidgumberg on Mar 6, 2024
  161. DrahtBot requested review from cbergqvist on Mar 6, 2024
  162. DrahtBot requested review from davidgumberg on Mar 6, 2024
  163. DrahtBot requested review from tdb3 on Mar 6, 2024
  164. LarryRuane commented at 1:06 am on March 6, 2024: contributor
    @marcofleon - by any chance is your filesystem running out of space? I can’t reproduce this problem. Running all the tests creates a directory of size 2.6GB (according to du -sh).
  165. DrahtBot removed review request from cbergqvist on Mar 6, 2024
  166. DrahtBot removed review request from davidgumberg on Mar 6, 2024
  167. DrahtBot removed review request from tdb3 on Mar 6, 2024
  168. DrahtBot requested review from tdb3 on Mar 6, 2024
  169. DrahtBot requested review from davidgumberg on Mar 6, 2024
  170. DrahtBot requested review from cbergqvist on Mar 6, 2024
  171. marcofleon approved
  172. marcofleon commented at 6:25 pm on March 6, 2024: contributor

    @LarryRuane - problem was my open file limit, it was set low. I appreciate your help.

    Tested ACK 0e477344fe4094ada5262a1e7e465d4cb93cacdd. I think this is a useful feature to add.

    1. Successfully ran test_bitcoin without using -testdatadir to ensure backwards compatibility. Confirmed that no test directories existed in /tmp after all tests completed.
    2. Ran test_bitcoin using the new feature, specifying a directory using both relative and absolute paths. No issues were observed (once I raised ulimit 😅). Checked that all the directories persisted after completion of the tests and that they were constructed in the format laid out in README.md. Checked inside multiple directories for debug.log files and other relevant files. Successfully ran several individual tests and test suites as well with -testdatadir specified.
    3. Confirmed that the exit failure messages rightfully appeared when leaving testdatadir empty and when attempting to run tests concurrently using the same directory.
  173. DrahtBot removed review request from tdb3 on Mar 6, 2024
  174. DrahtBot removed review request from davidgumberg on Mar 6, 2024
  175. DrahtBot removed review request from cbergqvist on Mar 6, 2024
  176. DrahtBot requested review from tdb3 on Mar 6, 2024
  177. DrahtBot requested review from davidgumberg on Mar 6, 2024
  178. DrahtBot requested review from cbergqvist on Mar 6, 2024
  179. LarryRuane force-pushed on Mar 6, 2024
  180. LarryRuane commented at 10:48 pm on March 6, 2024: contributor

    @marcofleon

    problem was my open file limit, it was set low.

    Thanks, you revealed a bug! The .lock file was not being closed after each test. This file descriptor leak is why you needed to increase that limit.

    Force pushed 2209a7f565ac8569ee777ee44c508683670009d6 to fix this problem. If you have time and interest, perhaps you can revert back to your lower open file limit, it should work now. I verified both the leak and the fix by running this command in another window as the test ran:

    0$ ls -l /proc/$(pgrep -f src/test/test_bitcoin)/fd

    Before this latest force-push, you would see many .lock files, one for each separate test. Now you see just one.

  181. in src/test/util/setup_common.cpp:210 in 2209a7f565 outdated 
    203@@ -159,7 +204,13 @@ BasicTestingSetup::~BasicTestingSetup()
204     m_node.kernel.reset();
205     SetMockTime(0s); // Reset mocktime for following tests
206     LogInstance().DisconnectTestLogger();
207-    fs::remove_all(m_path_root);
208+    if (m_has_custom_datadir) {
209+        // Only remove the lock file, preserve the data directory.
210+        UnlockDirectory(m_path_lock, ".lock");
211+        fs::remove_all(m_path_lock / ".lock");
    cbergqvist commented at 11:24 pm on March 6, 2024:
    Should probably be fs::remove instead of fs::remove_all?
    LarryRuane commented at 5:12 pm on March 7, 2024:
    Good catch, force-pushed d27e2d87b95b7982c05b4c88e463cc9626ab9f0a to fix.
  182. cbergqvist approved
  183. cbergqvist commented at 11:50 pm on March 6, 2024: none

    ACK 2209a7f565ac8569ee777ee44c508683670009d6

    Ran single test using the fixture + all tests, with no, relative and absolute dirs.

    0$ src/test/test_bitcoin -t argsman_tests
1$ src/test/test_bitcoin -t argsman_tests -- -testdatadir=foo
2$ src/test/test_bitcoin -t argsman_tests -- -testdatadir=/mnt/tmp
3$ src/test/test_bitcoin
4$ src/test/test_bitcoin -- -testdatadir=foo
5$ src/test/test_bitcoin -- -testdatadir=/mnt/tmp

    Verified that dirs remained for -testdatadir versions and where removed without.

  184. DrahtBot removed review request from tdb3 on Mar 6, 2024
  185. DrahtBot removed review request from davidgumberg on Mar 6, 2024
  186. DrahtBot requested review from furszy on Mar 6, 2024
  187. DrahtBot requested review from tdb3 on Mar 6, 2024
  188. DrahtBot requested review from davidgumberg on Mar 6, 2024
  189. DrahtBot requested review from marcofleon on Mar 6, 2024
  190. DrahtBot removed review request from tdb3 on Mar 6, 2024
  191. DrahtBot removed review request from davidgumberg on Mar 6, 2024
  192. DrahtBot removed review request from marcofleon on Mar 6, 2024
  193. DrahtBot requested review from marcofleon on Mar 6, 2024
  194. DrahtBot requested review from tdb3 on Mar 6, 2024
  195. DrahtBot requested review from davidgumberg on Mar 6, 2024
  196. test: test_bitcoin: allow -testdatadir=<datadir> 
    Specifying this argument overrides the path location for test_bitcoin;
it becomes <datadir>/test_common_Bitcoin Core/<testname>/datadir. Also,
this directory isn't removed after the test completes. This can make it
easier for developers to study the results of a test (see the state of
the data directory after the test runs), and also (for example) have an
editor open on debug.log to monitor it across multiple test runs instead
of having to re-open a different pathname each time.

Example usage (note the "--" is needed):

test_bitcoin --run_test=getarg_tests/boolarg -- \
-testdatadir=/somewhere/mydatadir

This will create (if necessary) and use the data directory:

/somewhere/mydatadir/test_common_Bitcoin Core/getarg_tests/boolarg/datadir

Co-authored-by: furszy <mfurszy@protonmail.com>
    d27e2d87b9
  197. LarryRuane force-pushed on Mar 7, 2024
  198. cbergqvist approved
  199. cbergqvist commented at 9:00 pm on March 7, 2024: none
    ACK d27e2d87b95b7982c05b4c88e463cc9626ab9f0a! (Already did some testing with fs::remove() to make sure it was compatible with the util::Lock/UnlockDirectory implementation).
  200. DrahtBot removed review request from marcofleon on Mar 7, 2024
  201. DrahtBot removed review request from tdb3 on Mar 7, 2024
  202. DrahtBot removed review request from davidgumberg on Mar 7, 2024
  203. DrahtBot requested review from tdb3 on Mar 7, 2024
  204. DrahtBot requested review from davidgumberg on Mar 7, 2024
  205. DrahtBot requested review from marcofleon on Mar 7, 2024
  206. marcofleon approved
  207. marcofleon commented at 9:43 pm on March 7, 2024: contributor

    ACK d27e2d87b95b7982c05b4c88e463cc9626ab9f0a. I ran all the tests with my previous open file limit and no errors were detected. Also ran some individual tests with no, relative, and absolute paths and everything looks good.

    I used watch -n 3 lsof -p PID while running the tests in both this commit and the previous one to compare the outputs and observe the leak. I noticed the presence of only one .lock file as compared to a buildup of .lock files when running the old code.

    Thanks for the guidance here @LarryRuane! Everything lgtm

  208. DrahtBot removed review request from tdb3 on Mar 7, 2024
  209. DrahtBot removed review request from davidgumberg on Mar 7, 2024
  210. DrahtBot requested review from tdb3 on Mar 7, 2024
  211. DrahtBot requested review from davidgumberg on Mar 7, 2024
  212. davidgumberg commented at 10:03 pm on March 7, 2024: contributor

    Diffed and re-tested running with -testdatadir with an absolute path and with a relative path.

    ACK https://github.com/bitcoin/bitcoin/pull/26564/commits/d27e2d87b95b7982c05b4c88e463cc9626ab9f0a

  213. DrahtBot removed review request from tdb3 on Mar 7, 2024
  214. DrahtBot removed review request from davidgumberg on Mar 7, 2024
  215. DrahtBot requested review from tdb3 on Mar 7, 2024
  216. furszy commented at 4:35 pm on March 8, 2024: member
    ACK d27e2d8
  217. DrahtBot removed review request from tdb3 on Mar 8, 2024
  218. DrahtBot requested review from tdb3 on Mar 8, 2024
  219. tdb3 commented at 1:42 am on March 9, 2024: contributor

    re-ACK for d27e2d87b95b7982c05b4c88e463cc9626ab9f0a

    Re-executed the test actions in #26564#pullrequestreview-1887381174. All unit tests passed.

  220. DrahtBot removed review request from tdb3 on Mar 9, 2024
  221. achow101 commented at 10:52 am on March 11, 2024: member
    ACK d27e2d87b95b7982c05b4c88e463cc9626ab9f0a
  222. achow101 merged this on Mar 11, 2024
  223. achow101 closed this on Mar 11, 2024


LarryRuane DrahtBot maflcko stickies-v Onyeali luke-jr kouloumos achow101 furszy RandyMcMillan cbergqvist tdb3 davidgumberg marcofleon


josibake kouloumos stickies-v

Labels
Tests

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-07-03 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me