test: test_bitcoin: allow -testdatadir=<datadir> #26564

LarryRuane · 2022-11-24T02:13:25Z

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:

$ test_bitcoin --run_test=getarg_tests/boolarg -- -testdatadir=/somewhere
Running 1 test case...
Test directory (will not be deleted): "/somewhere/test_temp/getarg_tests/boolarg/datadir"

*** No errors detected
$ ls -l /somewhere/test_temp/getarg_tests/boolarg/datadir
total 8
drwxrwxr-x 2 larry larry 4096 Feb 22 10:28 blocks
-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.

DrahtBot · 2022-11-24T02:13:27Z

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.

maflcko · 2022-11-24T07:31:02Z

src/test/util/setup_common.cpp

@@ -98,7 +98,9 @@ std::ostream& operator<<(std::ostream& os, const uint256& num)
 }

 BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::vector<const char*>& extra_args)
-    : m_path_root{fs::temp_directory_path() / "test_common_" PACKAGE_NAME / g_insecure_rand_ctx_temp_path.rand256().ToString()},
+    : m_path_root{std::getenv("TEST_BITCOIN_PATH") ?
+                      std::getenv("TEST_BITCOIN_PATH") :


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?

Not sure if this would work, but what about checking the datadir lock before removing it, e.g. with util/system.cpp::LockDirectory?

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.

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)?

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.

stickies-v

Concept ACK - seems helpful to improve productivity in certain workflows.

src/test/README.md

stickies-v · 2022-11-24T11:26:02Z

src/test/util/setup_common.cpp

@@ -98,7 +98,9 @@ std::ostream& operator<<(std::ostream& os, const uint256& num)
 }

 BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::vector<const char*>& extra_args)
-    : m_path_root{fs::temp_directory_path() / "test_common_" PACKAGE_NAME / g_insecure_rand_ctx_temp_path.rand256().ToString()},
+    : m_path_root{std::getenv("TEST_BITCOIN_PATH") ?
+                      std::getenv("TEST_BITCOIN_PATH") :


Not sure if this would work, but what about checking the datadir lock before removing it, e.g. with util/system.cpp::LockDirectory?

LarryRuane · 2022-11-25T20:43:18Z

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

luke-jr · 2022-11-26T08:41:26Z

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... :/

LarryRuane · 2022-11-26T18:12:51Z

@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:

$ BITCOIN_TEST_PATH=~/.bitcoin src/test/test_bitcoin --run_test=getarg_tests/logargs
Running 1 test case...
unknown location(0): fatal error: in "getarg_tests/logargs": std::runtime_error: .lock exists, not using possible real data directory
test/getarg_tests.cpp(423): last checkpoint: "logargs" fixture ctor

*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
$

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

luke-jr · 2022-11-27T07:54:29Z

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.

LarryRuane · 2022-11-28T06:03:52Z

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 (c48a1b4) 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.

$ BITCOIN_TEST_PATH=mydatadir src/test/test_bitcoin --run_test=getarg_tests/doubledash
Running 1 test case...

*** No errors detected
$ ls -l /tmp/test_common_Bitcoin\ Core/mydatadir/
total 8
drwxrwxr-x 2 larry larry 4096 Nov 27 22:45 blocks
-rw-rw-r-- 1 larry larry 1003 Nov 27 22:45 debug.log
$

maflcko · 2022-11-28T08:07:54Z

src/test/util/setup_common.cpp

+        test_path = g_insecure_rand_ctx_temp_path.rand256().ToString();
+    } else {
+        test_path = test_path_env;
+        if (fs::PathFromString(test_path).is_absolute()) {


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.

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.

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.

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.

$ BITCOIN_TEST_PATH=../mydatadir src/test/test_bitcoin --run_test=getarg_tests/doubledash Running 1 test case... unknown location(0): fatal error: in "getarg_tests/doubledash": std::runtime_error: BITCOIN_TEST_PATH cannot contain path separators test/getarg_tests.cpp(380): last checkpoint: "doubledash" fixture ctor *** 1 failure is detected in the test module "Bitcoin Core Test Suite" $

kouloumos

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?

src/test/README.md

src/test/util/setup_common.cpp

achow101 · 2023-04-25T16:02:30Z

Are you still working on this?

LarryRuane · 2023-04-27T17:00:07Z

Yes, still hoping to get this merged. Force-pushed 5df0ed5 to rebase onto the latest master (since it's been a long time)

LarryRuane · 2023-04-27T17:12:06Z

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

Ready for review, also ping @theStack.

LarryRuane · 2023-06-11T22:38:46Z

Rebased for merge conflict.

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>

cbergqvist

ACK d27e2d8! (Already did some testing with fs::remove() to make sure it was compatible with the util::Lock/UnlockDirectory implementation).

marcofleon

ACK d27e2d8. 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

davidgumberg · 2024-03-07T22:03:40Z

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

ACK d27e2d8

furszy

ACK d27e2d8

tdb3 · 2024-03-09T01:42:05Z

re-ACK for d27e2d8

Re-executed the test actions in #26564 (review). All unit tests passed.

achow101 · 2024-03-11T10:52:47Z

ACK d27e2d8

DrahtBot added the Tests label Nov 24, 2022

maflcko reviewed Nov 24, 2022

View reviewed changes

stickies-v reviewed Nov 24, 2022

View reviewed changes

Onyeali approved these changes Nov 24, 2022

View reviewed changes

LarryRuane force-pushed the 2022-11-testdir branch from bc32016 to 68b1a67 Compare November 25, 2022 20:35

LarryRuane changed the title ~~test: allow TEST_BITCOIN_PATH to specify working dir~~ test: allow BITCOIN_TEST_PATH to specify working dir Nov 25, 2022

LarryRuane force-pushed the 2022-11-testdir branch from 68b1a67 to be0b1e5 Compare November 26, 2022 17:53

LarryRuane force-pushed the 2022-11-testdir branch from be0b1e5 to c48a1b4 Compare November 28, 2022 05:55

maflcko reviewed Nov 28, 2022

View reviewed changes

LarryRuane force-pushed the 2022-11-testdir branch from c48a1b4 to 8aee8b4 Compare November 28, 2022 18:22

DrahtBot mentioned this pull request Jan 21, 2023

test: Remove boost test msg #26936

Closed

kouloumos reviewed Feb 2, 2023

View reviewed changes

src/test/README.md Outdated Show resolved Hide resolved

src/test/util/setup_common.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Apr 19, 2023

refactor: Move chain constants to the util library #27491

Merged

LarryRuane force-pushed the 2022-11-testdir branch from 8aee8b4 to 5df0ed5 Compare April 27, 2023 16:58

LarryRuane force-pushed the 2022-11-testdir branch from 5df0ed5 to 538e9a9 Compare April 27, 2023 17:09

DrahtBot mentioned this pull request May 5, 2023

kernel: Remove args, settings, chainparams, chainparamsbase from kernel library #27576

Merged

DrahtBot added the Needs rebase label May 9, 2023

LarryRuane force-pushed the 2022-11-testdir branch from 538e9a9 to 4916434 Compare June 11, 2023 21:30

DrahtBot removed the Needs rebase label Jun 11, 2023

maflcko mentioned this pull request Aug 8, 2023

bench: add readblock benchmark #26684

Merged

DrahtBot requested review from tdb3 and davidgumberg March 6, 2024 23:50

LarryRuane force-pushed the 2022-11-testdir branch from 2209a7f to d27e2d8 Compare March 7, 2024 17:11

cbergqvist approved these changes Mar 7, 2024

View reviewed changes

DrahtBot requested review from tdb3, davidgumberg and marcofleon and removed request for marcofleon, tdb3 and davidgumberg March 7, 2024 21:00

marcofleon approved these changes Mar 7, 2024

View reviewed changes

DrahtBot requested review from tdb3 and davidgumberg and removed request for tdb3 and davidgumberg March 7, 2024 21:43

DrahtBot requested review from tdb3 and removed request for tdb3 and davidgumberg March 7, 2024 22:03

furszy reviewed Mar 8, 2024

View reviewed changes

DrahtBot requested review from tdb3 and removed request for tdb3 March 8, 2024 16:35

DrahtBot removed the request for review from tdb3 March 9, 2024 01:42

achow101 merged commit 5ebb406 into bitcoin:master Mar 11, 2024

furszy mentioned this pull request Sep 30, 2024

bench: add support for custom data directory #31000

Merged

xyephy mentioned this pull request Feb 26, 2025

Upgrade to bitcoin v28 whiveio/whive#12

Open

bitcoin locked and limited conversation to collaborators Mar 11, 2025

test: test_bitcoin: allow -testdatadir=<datadir> #26564

test: test_bitcoin: allow -testdatadir=<datadir> #26564

Uh oh!

Conversation

LarryRuane commented Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickies-v Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LarryRuane commented Nov 25, 2022

Uh oh!

luke-jr commented Nov 26, 2022

Uh oh!

LarryRuane commented Nov 26, 2022

Uh oh!

luke-jr commented Nov 27, 2022

Uh oh!

LarryRuane commented Nov 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kouloumos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

achow101 commented Apr 25, 2023

Uh oh!

LarryRuane commented Apr 27, 2023

Uh oh!

LarryRuane commented Apr 27, 2023

Uh oh!

LarryRuane commented Jun 11, 2023

Uh oh!

cbergqvist left a comment

Choose a reason for hiding this comment

Uh oh!

marcofleon left a comment

Choose a reason for hiding this comment

Uh oh!

davidgumberg commented Mar 7, 2024

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

tdb3 commented Mar 9, 2024

Uh oh!

achow101 commented Mar 11, 2024

Uh oh!

LarryRuane commented Nov 24, 2022 •

edited

Loading

DrahtBot commented Nov 24, 2022 •

edited

Loading

stickies-v Nov 24, 2022 •

edited

Loading