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

d27e2d87b95b7982c05b4c88e463cc9626ab9f0a test: test_bitcoin: allow -testdatadir=<datadir> (Larry Ruane)

Pull request description:

  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.

ACKs for top commit:
  davidgumberg:
    ACK https://github.com/bitcoin/bitcoin/pull/26564/commits/d27e2d87b95b7982c05b4c88e463cc9626ab9f0a
  tdb3:
    re-ACK for d27e2d87b95b7982c05b4c88e463cc9626ab9f0a
  achow101:
    ACK d27e2d87b95b7982c05b4c88e463cc9626ab9f0a
  cbergqvist:
    ACK d27e2d87b95b7982c05b4c88e463cc9626ab9f0a! (Already did some testing with `fs::remove()` to make sure it was compatible with the `util::Lock/UnlockDirectory` implementation).
  marcofleon:
    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.
  furszy:
    ACK d27e2d8

Tree-SHA512: a8f535f34a48b6699cb440f97f5562ec643f3bfba4ea685768980b871fc8b6e1135f70fc05dbe19aa2c8bacb1ddeaff212d63473605a7422ff76332b3a6b1f68

8 files changed, 121 insertions, 14 deletions

diff --git a/src/bench/bench.cpp b/src/bench/bench.cpp
index 84b66bc4b2..a13a693ad7 100644
--- a/src/bench/bench.cpp
+++ b/src/bench/bench.cpp
@@ -23,6 +23,8 @@ const std::function<void(const std::string&)> G_TEST_LOG_FUN{};
 
 const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS{};
 
+const std::function<std::string()> G_TEST_GET_FULL_NAME{};
+
 namespace {
 
 void GenerateTemplateResults(const std::vector<ankerl::nanobench::Result>& benchmarkResults, const fs::path& file, const char* tpl)
diff --git a/src/qt/main.cpp b/src/qt/main.cpp
index c84dd78b44..ded057dbfa 100644
--- a/src/qt/main.cpp
+++ b/src/qt/main.cpp
@@ -19,6 +19,8 @@ extern const std::function<std::string(const char*)> G_TRANSLATION_FUN = [](cons
 };
 UrlDecodeFn* const URL_DECODE = urlDecode;
 
+const std::function<std::string()> G_TEST_GET_FULL_NAME{};
+
 MAIN_FUNCTION
 {
     return GuiMain(argc, argv);
diff --git a/src/qt/test/test_main.cpp b/src/qt/test/test_main.cpp
index 8decc27bd7..c5405cca98 100644
--- a/src/qt/test/test_main.cpp
+++ b/src/qt/test/test_main.cpp
@@ -50,6 +50,8 @@ const std::function<void(const std::string&)> G_TEST_LOG_FUN{};
 
 const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS{};
 
+const std::function<std::string()> G_TEST_GET_FULL_NAME{};
+
 // This is all you need to run all the tests
 int main(int argc, char* argv[])
 {
diff --git a/src/test/README.md b/src/test/README.md
index 0876db7a72..bab1a28f61 100644
--- a/src/test/README.md
+++ b/src/test/README.md
@@ -42,17 +42,18 @@ test_bitcoin --log_level=all --run_test=getarg_tests
 ```
 
 `log_level` controls the verbosity of the test framework, which logs when a
-test case is entered, for example. `test_bitcoin` also accepts the command
-line arguments accepted by `bitcoind`. Use `--` to separate both types of
-arguments:
+test case is entered, for example.
+
+`test_bitcoin` also accepts some of the command line arguments accepted by
+`bitcoind`. Use `--` to separate these sets of arguments:
 
 ```bash
 test_bitcoin --log_level=all --run_test=getarg_tests -- -printtoconsole=1
 ```
 
-The `-printtoconsole=1` after the two dashes redirects the debug log, which
-would normally go to a file in the test datadir
-(`BasicTestingSetup::m_path_root`), to the standard terminal output.
+The `-printtoconsole=1` after the two dashes sends debug logging, which
+normally goes only to `debug.log` within the data directory, also to the
+standard terminal output.
 
 ... or to run just the doubledash test:
 
@@ -60,7 +61,42 @@ would normally go to a file in the test datadir
 test_bitcoin --run_test=getarg_tests/doubledash
 ```
 
-Run `test_bitcoin --help` for the full list.
+`test_bitcoin` creates a temporary working (data) directory with a randomly
+generated pathname within `test_common_Bitcoin Core/`, which in turn is within
+the system's temporary directory (see
+[`temp_directory_path`](https://en.cppreference.com/w/cpp/filesystem/temp_directory_path)).
+This data directory looks like a simplified form of the standard `bitcoind` data
+directory. Its content will vary depending on the test, but it will always
+have a `debug.log` file, for example.
+
+The location of the temporary data directory can be specified with the
+`-testdatadir` option. This can make debugging easier. The directory
+path used is the argument path appended with
+`/test_common_Bitcoin Core/<test-name>/datadir`.
+The directory path is created if necessary.
+Specifying this argument also causes the data directory
+not to be removed after the last test. This is useful for looking at
+what the test wrote to `debug.log` after it completes, for example.
+(The directory is removed at the start of the next test run,
+so no leftover state is used.)
+
+```bash
+$ test_bitcoin --run_test=getarg_tests/doubledash -- -testdatadir=/somewhere/mydatadir
+Test directory (will not be deleted): "/somewhere/mydatadir/test_common_Bitcoin Core/getarg_tests/doubledash/datadir
+Running 1 test case...
+
+*** No errors detected
+$ ls -l '/somewhere/mydatadir/test_common_Bitcoin Core/getarg_tests/doubledash/datadir'
+total 8
+drwxrwxr-x 2 admin admin 4096 Nov 27 22:45 blocks
+-rw-rw-r-- 1 admin admin 1003 Nov 27 22:45 debug.log
+```
+
+If you run an entire test suite, such as `--run_test=getarg_tests`, or all the test suites
+(by not specifying `--run_test`), a separate directory
+will be created for each individual test.
+
+Run `test_bitcoin --help` for the full list of tests.
 
 ### Adding test cases
 
diff --git a/src/test/fuzz/fuzz.cpp b/src/test/fuzz/fuzz.cpp
index 6de480ff15..d1a67cb0d8 100644
--- a/src/test/fuzz/fuzz.cpp
+++ b/src/test/fuzz/fuzz.cpp
@@ -35,6 +35,8 @@ __AFL_FUZZ_INIT();
 
 const std::function<void(const std::string&)> G_TEST_LOG_FUN{};
 
+const std::function<std::string()> G_TEST_GET_FULL_NAME{};
+
 /**
  * A copy of the command line arguments that start with `--`.
  * First `LLVMFuzzerInitialize()` is called, which saves the arguments to `g_args`.
diff --git a/src/test/main.cpp b/src/test/main.cpp
index 0809f83c93..67740ece93 100644
--- a/src/test/main.cpp
+++ b/src/test/main.cpp
@@ -39,3 +39,10 @@ const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS =
     }
     return args;
 };
+
+/**
+ * Retrieve the boost unit test name.
+ */
+const std::function<std::string()> G_TEST_GET_FULL_NAME = []() {
+    return boost::unit_test::framework::current_test_case().full_name();
+};
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index d26a440074..2c18184261 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -52,6 +52,7 @@
 #include <txmempool.h>
 #include <util/chaintype.h>
 #include <util/check.h>
+#include <util/fs_helpers.h>
 #include <util/rbf.h>
 #include <util/strencodings.h>
 #include <util/string.h>
@@ -100,9 +101,22 @@ struct NetworkSetup
 };
 static NetworkSetup g_networksetup_instance;
 
+/** Register test-only arguments */
+static void SetupUnitTestArgs(ArgsManager& argsman)
+{
+    argsman.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);
+}
+
+/** Test setup failure */
+static void ExitFailure(std::string_view str_err)
+{
+    std::cerr << str_err << std::endl;
+    exit(EXIT_FAILURE);
+}
+
 BasicTestingSetup::BasicTestingSetup(const ChainType chainType, 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_args{}
+    : m_args{}
 {
     m_node.shutdown = &m_interrupt;
     m_node.args = &gArgs;
@@ -123,18 +137,49 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, const std::vecto
         arguments = Cat(arguments, G_TEST_COMMAND_LINE_ARGUMENTS());
     }
     util::ThreadRename("test");
-    fs::create_directories(m_path_root);
-    m_args.ForceSetArg("-datadir", fs::PathToString(m_path_root));
-    gArgs.ForceSetArg("-datadir", fs::PathToString(m_path_root));
     gArgs.ClearPathCache();
     {
         SetupServerArgs(*m_node.args);
+        SetupUnitTestArgs(*m_node.args);
         std::string error;
         if (!m_node.args->ParseParameters(arguments.size(), arguments.data(), error)) {
             m_node.args->ClearArgs();
             throw std::runtime_error{error};
         }
     }
+
+    if (!m_node.args->IsArgSet("-testdatadir")) {
+        // By default, the data directory has a random name
+        const auto rand_str{g_insecure_rand_ctx_temp_path.rand256().ToString()};
+        m_path_root = fs::temp_directory_path() / "test_common_" PACKAGE_NAME / rand_str;
+        TryCreateDirectories(m_path_root);
+    } else {
+        // Custom data directory
+        m_has_custom_datadir = true;
+        fs::path root_dir{m_node.args->GetPathArg("-testdatadir")};
+        if (root_dir.empty()) ExitFailure("-testdatadir argument is empty, please specify a path");
+
+        root_dir = fs::absolute(root_dir);
+        const std::string test_path{G_TEST_GET_FULL_NAME ? G_TEST_GET_FULL_NAME() : ""};
+        m_path_lock = root_dir / "test_common_" PACKAGE_NAME / fs::PathFromString(test_path);
+        m_path_root = m_path_lock / "datadir";
+
+        // Try to obtain the lock; if unsuccessful don't disturb the existing test.
+        TryCreateDirectories(m_path_lock);
+        if (util::LockDirectory(m_path_lock, ".lock", /*probe_only=*/false) != util::LockResult::Success) {
+            ExitFailure("Cannot obtain a lock on test data lock directory " + fs::PathToString(m_path_lock) + '\n' + "The test executable is probably already running.");
+        }
+
+        // Always start with a fresh data directory; this doesn't delete the .lock file located one level above.
+        fs::remove_all(m_path_root);
+        if (!TryCreateDirectories(m_path_root)) ExitFailure("Cannot create test data directory");
+
+        // Print the test directory name if custom.
+        std::cout << "Test directory (will not be deleted): " << m_path_root << std::endl;
+    }
+    m_args.ForceSetArg("-datadir", fs::PathToString(m_path_root));
+    gArgs.ForceSetArg("-datadir", fs::PathToString(m_path_root));
+
     SelectParams(chainType);
     SeedInsecureRand();
     if (G_TEST_LOG_FUN) LogInstance().PushBackCallback(G_TEST_LOG_FUN);
@@ -162,7 +207,13 @@ BasicTestingSetup::~BasicTestingSetup()
     m_node.kernel.reset();
     SetMockTime(0s); // Reset mocktime for following tests
     LogInstance().DisconnectTestLogger();
-    fs::remove_all(m_path_root);
+    if (m_has_custom_datadir) {
+        // Only remove the lock file, preserve the data directory.
+        UnlockDirectory(m_path_lock, ".lock");
+        fs::remove(m_path_lock / ".lock");
+    } else {
+        fs::remove_all(m_path_root);
+    }
     gArgs.ClearArgs();
 }
 
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index 9ff4c372a5..8ccf9b571c 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -32,6 +32,9 @@ extern const std::function<void(const std::string&)> G_TEST_LOG_FUN;
 /** Retrieve the command line arguments. */
 extern const std::function<std::vector<const char*>()> G_TEST_COMMAND_LINE_ARGUMENTS;
 
+/** Retrieve the unit test name. */
+extern const std::function<std::string()> G_TEST_GET_FULL_NAME;
+
 // Enable BOOST_CHECK_EQUAL for enum class types
 namespace std {
 template <typename T>
@@ -53,7 +56,9 @@ struct BasicTestingSetup {
     explicit BasicTestingSetup(const ChainType chainType = ChainType::MAIN, const std::vector<const char*>& extra_args = {});
     ~BasicTestingSetup();
 
-    const fs::path m_path_root;
+    fs::path m_path_root;
+    fs::path m_path_lock;
+    bool m_has_custom_datadir{false};
     ArgsManager m_args;
 };