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

d27e2d8 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 bitcoin/bitcoin@d27e2d8
  tdb3:
    re-ACK for d27e2d8
  achow101:
    ACK d27e2d8
  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.
  furszy:
    ACK d27e2d8

Tree-SHA512: a8f535f34a48b6699cb440f97f5562ec643f3bfba4ea685768980b871fc8b6e1135f70fc05dbe19aa2c8bacb1ddeaff212d63473605a7422ff76332b3a6b1f68

Author: achow101

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)

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

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[])
 {

src/test/README.md

@@ -42,25 +42,61 @@ 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:
 
 ```bash
 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
 

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`.

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();
+};

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();
 }
 

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;
 };