Merge #16252: test: Log to debug.log in all unit tests

fabc57e07dc34c103552cb51c9277bb48ef97739 test: Log to debug.log in all tests (MarcoFalke)
fa4a04a5a942d582c62773d815c7e1e9897975d0 test: use common setup in gui tests (MarcoFalke)
fad3d2a624377de4b0311e6ddd446c36dafd1ddc test: Create data dir in BasicTestingSetup (MarcoFalke)

Pull request description:

  This makes it easier to debug a frozen test or a test that failed. To debug a failed test, remove the line `fs::remove_all(m_path_root);`.

  The pull is done in three commits:
  * Create a datadir for every unit test once (and only once). This requires the `SetDataDir` function to go away.
  * Use the common setup in the gui unit tests. Some of those tests are testing the init sequence, so we'd have to undo some of what the testing setup did.
  * Log to the debug.log in all tests

ACKs for top commit:
  laanwj:
    ACK fabc57e07dc34c103552cb51c9277bb48ef97739

Tree-SHA512: 73444210b88172669e2cd22c2703a1e30e105185d2d5f03decbdedcfd09c64ed208d3716c59c8bebb0e44214cee5c8095e3e995d049e1572ee98f1017e413665

src/logging.cpp

@@ -76,6 +76,14 @@ bool BCLog::Logger::StartLogging()
     return true;
 }
 
+void BCLog::Logger::DisconnectTestLogger()
+{
+    StdLockGuard scoped_lock(m_cs);
+    m_buffering = true;
+    if (m_fileout != nullptr) fclose(m_fileout);
+    m_fileout = nullptr;
+}
+
 void BCLog::Logger::EnableCategory(BCLog::LogFlags flag)
 {
     m_categories |= flag;

src/logging.h

@@ -125,6 +125,8 @@ namespace BCLog {
 
         /** Start logging (and flush all buffered messages) */
         bool StartLogging();
+        /** Only for testing */
+        void DisconnectTestLogger();
 
         void ShrinkDebugFile();
 

src/qt/test/apptests.cpp

@@ -6,6 +6,7 @@
 
 #include <chainparams.h>
 #include <init.h>
+#include <key.h>
 #include <qt/dash.h>
 #include <qt/bitcoingui.h>
 #include <qt/networkstyle.h>
@@ -67,6 +68,9 @@ void AppTests::appTests()
     }
 #endif
 
+    ECC_Stop(); // Already started by the common test setup, so stop it to avoid interference
+    LogInstance().DisconnectTestLogger();
+
     m_app.parameterSetup();
     GUIUtil::loadFonts();
     m_app.createOptionsModel(true /* reset settings */);

src/qt/test/rpcnestedtests.cpp

@@ -41,6 +41,7 @@ void RPCNestedTests::rpcNestedTests()
     tableRPC.appendCommand("rpcNestedTest", &vRPCCommands[0]);
     //mempool.setSanityCheck(1.0);
 
+    LogInstance().DisconnectTestLogger(); // Already started by the common test setup, so stop it to avoid interference
     TestingSetup test;
 
     if (RPCIsInWarmup(nullptr)) SetRPCWarmupFinished();

src/qt/test/test_main.cpp

@@ -12,10 +12,10 @@
 #include <qt/dash.h>
 #include <qt/test/apptests.h>
 #include <qt/test/rpcnestedtests.h>
-#include <util/system.h>
 #include <qt/test/uritests.h>
 #include <qt/test/compattests.h>
 #include <qt/test/trafficgraphdatatests.h>
+#include <test/setup_common.h>
 
 #ifdef ENABLE_WALLET
 #include <qt/test/addressbooktests.h>
@@ -50,14 +50,8 @@ extern void noui_connect();
 // This is all you need to run all the tests
 int main(int argc, char *argv[])
 {
-    SetupEnvironment();
-    SetupNetworking();
-    SelectParams(CBaseChainParams::REGTEST);
-    noui_connect();
-    ClearDatadirCache();
-    fs::path pathTemp = fs::temp_directory_path() / strprintf("test_dash-qt_%lu_%i", (unsigned long)GetTime(), (int)GetRand(1 << 30));
-    fs::create_directories(pathTemp);
-    gArgs.ForceSetArg("-datadir", pathTemp.string());
+    BasicTestingSetup test{CBaseChainParams::REGTEST};
+
     auto node = interfaces::MakeNode();
 
     bool fInvalid = false;

src/test/blockfilter_index_tests.cpp

@@ -267,8 +267,6 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, TestChain100Setup)
 
 BOOST_FIXTURE_TEST_CASE(blockfilter_index_init_destroy, BasicTestingSetup)
 {
-    SetDataDir("tempdir");
-
     BlockFilterIndex* filter_index;
 
     filter_index = GetBlockFilterIndex(BlockFilterType::BASIC_FILTER);

src/test/dbwrapper_tests.cpp

@@ -27,7 +27,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper)
 {
     // Perform tests both obfuscated and non-obfuscated.
     for (const bool obfuscate : {false, true}) {
-        fs::path ph = SetDataDir(std::string("dbwrapper").append(obfuscate ? "_true" : "_false"));
+        fs::path ph = GetDataDir() / (obfuscate ? "dbwrapper_obfuscate_true" : "dbwrapper_obfuscate_false");
         CDBWrapper dbw(ph, (1 << 20), true, false, obfuscate);
         char key = 'k';
         uint256 in = InsecureRand256();
@@ -46,7 +46,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper_basic_data)
 {
     // Perform tests both obfuscated and non-obfuscated.
     for (bool obfuscate : {false, true}) {
-        fs::path ph = SetDataDir(std::string("dbwrapper_1").append(obfuscate ? "_true" : "_false"));
+        fs::path ph = GetDataDir() / (obfuscate ? "dbwrapper_1_obfuscate_true" : "dbwrapper_1_obfuscate_false");
         CDBWrapper dbw(ph, (1 << 20), false, true, obfuscate);
 
         uint256 res;
@@ -127,7 +127,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper_batch)
 {
     // Perform tests both obfuscated and non-obfuscated.
     for (const bool obfuscate : {false, true}) {
-        fs::path ph = SetDataDir(std::string("dbwrapper_batch").append(obfuscate ? "_true" : "_false"));
+        fs::path ph = GetDataDir() / (obfuscate ? "dbwrapper_batch_obfuscate_true" : "dbwrapper_batch_obfuscate_false");
         CDBWrapper dbw(ph, (1 << 20), true, false, obfuscate);
 
         char key = 'i';
@@ -163,7 +163,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper_iterator)
 {
     // Perform tests both obfuscated and non-obfuscated.
     for (const bool obfuscate : {false, true}) {
-        fs::path ph = SetDataDir(std::string("dbwrapper_iterator").append(obfuscate ? "_true" : "_false"));
+        fs::path ph = GetDataDir() / (obfuscate ? "dbwrapper_iterator_obfuscate_true" : "dbwrapper_iterator_obfuscate_false");
         CDBWrapper dbw(ph, (1 << 20), true, false, obfuscate);
 
         // The two keys are intentionally chosen for ordering
@@ -203,7 +203,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper_iterator)
 BOOST_AUTO_TEST_CASE(existing_data_no_obfuscate)
 {
     // We're going to share this fs::path between two wrappers
-    fs::path ph = SetDataDir("existing_data_no_obfuscate");
+    fs::path ph = GetDataDir() / "existing_data_no_obfuscate";
     create_directories(ph);
 
     // Set up a non-obfuscated wrapper to write some initial data.
@@ -244,7 +244,7 @@ BOOST_AUTO_TEST_CASE(existing_data_no_obfuscate)
 BOOST_AUTO_TEST_CASE(existing_data_reindex)
 {
     // We're going to share this fs::path between two wrappers
-    fs::path ph = SetDataDir("existing_data_reindex");
+    fs::path ph = GetDataDir() / "existing_data_reindex";
     create_directories(ph);
 
     // Set up a non-obfuscated wrapper to write some initial data.
@@ -279,7 +279,7 @@ BOOST_AUTO_TEST_CASE(existing_data_reindex)
 
 BOOST_AUTO_TEST_CASE(iterator_ordering)
 {
-    fs::path ph = SetDataDir("iterator_ordering");
+    fs::path ph = GetDataDir() / "iterator_ordering";
     CDBWrapper dbw(ph, (1 << 20), true, false, false);
     for (int x=0x00; x<256; ++x) {
         uint8_t key = x;
@@ -359,7 +359,7 @@ BOOST_AUTO_TEST_CASE(iterator_string_ordering)
 {
     char buf[10];
 
-    fs::path ph = SetDataDir("iterator_string_ordering");
+    fs::path ph = GetDataDir() / "iterator_string_ordering";
     CDBWrapper dbw(ph, (1 << 20), true, false, false);
     for (int x=0x00; x<10; ++x) {
         for (int y = 0; y < 10; y++) {
@@ -405,7 +405,7 @@ BOOST_AUTO_TEST_CASE(unicodepath)
     // On Windows this test will fail if the directory is created using
     // the ANSI CreateDirectoryA  call and the code page isn't UTF8.
     // It will succeed if the created with  CreateDirectoryW.
-    fs::path ph = SetDataDir("test_runner_₿_🏃_20191128_104644");
+    fs::path ph = GetDataDir() / "test_runner_₿_🏃_20191128_104644";
     CDBWrapper dbw(ph, (1 << 20));
 
     fs::path lockPath = ph / "LOCK";

src/test/flatfile_tests.cpp

@@ -11,7 +11,7 @@ BOOST_FIXTURE_TEST_SUITE(flatfile_tests, BasicTestingSetup)
 
 BOOST_AUTO_TEST_CASE(flatfile_filename)
 {
-    auto data_dir = SetDataDir("flatfile_test");
+    const auto data_dir = GetDataDir();
 
     FlatFilePos pos(456, 789);
 
@@ -24,7 +24,7 @@ BOOST_AUTO_TEST_CASE(flatfile_filename)
 
 BOOST_AUTO_TEST_CASE(flatfile_open)
 {
-    auto data_dir = SetDataDir("flatfile_test");
+    const auto data_dir = GetDataDir();
     FlatFileSeq seq(data_dir, "a", 16 * 1024);
 
     std::string line1("A purely peer-to-peer version of electronic cash would allow online "
@@ -85,7 +85,7 @@ BOOST_AUTO_TEST_CASE(flatfile_open)
 
 BOOST_AUTO_TEST_CASE(flatfile_allocate)
 {
-    auto data_dir = SetDataDir("flatfile_test");
+    const auto data_dir = GetDataDir();
     FlatFileSeq seq(data_dir, "a", 100);
 
     bool out_of_space;
@@ -105,7 +105,7 @@ BOOST_AUTO_TEST_CASE(flatfile_allocate)
 
 BOOST_AUTO_TEST_CASE(flatfile_flush)
 {
-    auto data_dir = SetDataDir("flatfile_test");
+    const auto data_dir = GetDataDir();
     FlatFileSeq seq(data_dir, "a", 100);
 
     bool out_of_space;

src/test/fs_tests.cpp

@@ -12,7 +12,7 @@ BOOST_FIXTURE_TEST_SUITE(fs_tests, BasicTestingSetup)
 
 BOOST_AUTO_TEST_CASE(fsbridge_fstream)
 {
-    fs::path tmpfolder = SetDataDir("fsbridge_fstream");
+    fs::path tmpfolder = GetDataDir();
     // tmpfile1 should be the same as tmpfile2
     fs::path tmpfile1 = tmpfolder / "fs_tests_₿_🏃";
     fs::path tmpfile2 = tmpfolder / "fs_tests_₿_🏃";

src/test/net_tests.cpp

@@ -93,7 +93,6 @@ BOOST_AUTO_TEST_CASE(cnode_listen_port)
 
 BOOST_AUTO_TEST_CASE(caddrdb_read)
 {
-    SetDataDir("caddrdb_read");
     CAddrManUncorrupted addrmanUncorrupted;
     addrmanUncorrupted.MakeDeterministic();
 
@@ -138,7 +137,6 @@ BOOST_AUTO_TEST_CASE(caddrdb_read)
 
 BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted)
 {
-    SetDataDir("caddrdb_read_corrupted");
     CAddrManCorrupted addrmanCorrupted;
     addrmanCorrupted.MakeDeterministic();
 

src/test/setup_common.cpp

@@ -11,6 +11,7 @@
 #include <consensus/validation.h>
 #include <crypto/sha256.h>
 #include <index/txindex.h>
+#include <init.h>
 #include <miner.h>
 #include <net_processing.h>
 #include <noui.h>
@@ -43,6 +44,13 @@ std::ostream& operator<<(std::ostream& os, const uint256& num)
 BasicTestingSetup::BasicTestingSetup(const std::string& chainName)
     : m_path_root(fs::temp_directory_path() / "test_common_" PACKAGE_NAME / strprintf("%lu_%i", (unsigned long)GetTime(), (int)(InsecureRandRange(1 << 30))))
 {
+    fs::create_directories(m_path_root);
+    gArgs.ForceSetArg("-datadir", m_path_root.string());
+    ClearDatadirCache();
+    SelectParams(chainName);
+    gArgs.ForceSetArg("-printtoconsole", "0");
+    InitLogging();
+    LogInstance().StartLogging();
     SHA256AutoDetect();
     ECC_Start();
     BLSInit();
@@ -52,7 +60,6 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName)
     InitScriptExecutionCache();
     CCoinJoin::InitStandardDenominations();
     fCheckBlockIndex = true;
-    SelectParams(chainName);
     evoDb.reset(new CEvoDB(1 << 20, true, true));
     deterministicMNManager.reset(new CDeterministicMNManager(*evoDb));
     static bool noui_connected = false;
@@ -67,26 +74,17 @@ BasicTestingSetup::~BasicTestingSetup()
     deterministicMNManager.reset();
     evoDb.reset();
 
+    LogInstance().DisconnectTestLogger();
     fs::remove_all(m_path_root);
     ECC_Stop();
 }
 
-fs::path BasicTestingSetup::SetDataDir(const std::string& name)
-{
-    fs::path ret = m_path_root / name;
-    fs::create_directories(ret);
-    gArgs.ForceSetArg("-datadir", ret.string());
-    return ret;
-}
-
 TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(chainName)
 {
-    SetDataDir("tempdir");
     const CChainParams& chainparams = Params();
     // Ideally we'd move all the RPC tests to the functional testing framework
     // instead of unit tests, but for now we need these here.
     RegisterAllCoreRPCCommands(tableRPC);
-    ClearDatadirCache();
 
     // We have to run a scheduler thread to prevent ActivateBestChain
     // from blocking due to queue overrun.

src/test/setup_common.h

@@ -54,22 +54,19 @@ static inline bool InsecureRandBool() { return g_insecure_rand_ctx.randbool(); }
 static constexpr CAmount CENT{1000000};
 
 /** Basic testing setup.
- * This just configures logging and chain parameters.
+ * This just configures logging, data dir and chain parameters.
  */
 struct BasicTestingSetup {
     ECCVerifyHandle globalVerifyHandle;
 
     explicit BasicTestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
     ~BasicTestingSetup();
-
-    fs::path SetDataDir(const std::string& name);
-
 private:
     const fs::path m_path_root;
 };
 
 /** Testing setup that configures a complete environment.
- * Included are data directory, coins database, script check threads setup.
+ * Included are coins database, script check threads setup.
  */
 struct TestingSetup : public BasicTestingSetup {
     boost::thread_group threadGroup;

src/test/util_tests.cpp

@@ -1136,7 +1136,7 @@ static constexpr char ExitCommand = 'X';
 
 BOOST_AUTO_TEST_CASE(test_LockDirectory)
 {
-    fs::path dirname = SetDataDir("test_LockDirectory") / fs::unique_path();
+    fs::path dirname = GetDataDir() / "lock_dir";
     const std::string lockname = ".lock";
 #ifndef WIN32
     // Revert SIGCHLD to default, otherwise boost.test will catch and fail on
@@ -1225,7 +1225,7 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory)
 BOOST_AUTO_TEST_CASE(test_DirIsWritable)
 {
     // Should be able to write to the data dir.
-    fs::path tmpdirname = SetDataDir("test_DirIsWritable");
+    fs::path tmpdirname = GetDataDir();
     BOOST_CHECK_EQUAL(DirIsWritable(tmpdirname), true);
 
     // Should not be able to write to a non-existent dir.

src/wallet/test/db_tests.cpp

@@ -16,7 +16,7 @@ BOOST_FIXTURE_TEST_SUITE(db_tests, BasicTestingSetup)
 BOOST_AUTO_TEST_CASE(getwalletenv_file)
 {
     std::string test_name = "test_name.dat";
-    fs::path datadir = SetDataDir("tempdir");
+    const fs::path datadir = GetDataDir();
     fs::path file_path = datadir / test_name;
     std::ofstream f(file_path.BOOST_FILESYSTEM_C_STR);
     f.close();
@@ -30,7 +30,7 @@ BOOST_AUTO_TEST_CASE(getwalletenv_file)
 BOOST_AUTO_TEST_CASE(getwalletenv_directory)
 {
     std::string expected_name = "wallet.dat";
-    fs::path datadir = SetDataDir("tempdir");
+    const fs::path datadir = GetDataDir();
 
     std::string filename;
     std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(datadir, filename);
@@ -40,8 +40,8 @@ BOOST_AUTO_TEST_CASE(getwalletenv_directory)
 
 BOOST_AUTO_TEST_CASE(getwalletenv_g_dbenvs_multiple)
 {
-    fs::path datadir = SetDataDir("tempdir");
-    fs::path datadir_2 = SetDataDir("tempdir_2");
+    fs::path datadir = GetDataDir() / "1";
+    fs::path datadir_2 = GetDataDir() / "2";
     std::string filename;
 
     std::shared_ptr<BerkeleyEnvironment> env_1 = GetWalletEnv(datadir, filename);
@@ -54,8 +54,8 @@ BOOST_AUTO_TEST_CASE(getwalletenv_g_dbenvs_multiple)
 
 BOOST_AUTO_TEST_CASE(getwalletenv_g_dbenvs_free_instance)
 {
-    fs::path datadir = SetDataDir("tempdir");
-    fs::path datadir_2 = SetDataDir("tempdir_2");
+    fs::path datadir = GetDataDir() / "1";
+    fs::path datadir_2 = GetDataDir() / "2";
     std::string filename;
 
     std::shared_ptr <BerkeleyEnvironment> env_1_a = GetWalletEnv(datadir, filename);

src/wallet/test/init_test_fixture.cpp

@@ -13,7 +13,7 @@ InitWalletDirTestingSetup::InitWalletDirTestingSetup(const std::string& chainNam
     std::string sep;
     sep += fs::path::preferred_separator;
 
-    m_datadir = SetDataDir("tempdir");
+    m_datadir = GetDataDir();
     m_cwd = fs::current_path();
 
     m_walletdir_path_cases["default"] = m_datadir / "wallets";

src/wallet/test/wallet_tests.cpp

@@ -195,7 +195,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
     auto locked_chain = chain->lock();
     LockAnnotation lock(::cs_main);
 
-    std::string backup_file = (SetDataDir("importwallet_rescan") / "wallet.backup").string();
+    std::string backup_file = (GetDataDir() / "wallet.backup").string();
 
     // Import key into wallet and call dumpwallet to create backup file.
     {