Use common getPath method to create temp directory in tests. #13145

Took a stab at #12574

Created a getPath method which can be used with the TestingSetup fixture to create a temp directory. Updated tests using temp directories to use this method.

I tried setting up a BOOST_GLOBAL_FIXTURE to create a truly global path for all tests but was getting linker errors when including boost/test/unit_test.hpp in test_bitcoin.cpp. Even if I had gotten the linking to work, it looks like make check invokes the test binary a bunch of times, so it may not have worked anyway.

Failure if I try to build locally - :

/home/orion/projects/bitcoin/bitcoin/src/test/dbwrapper_tests.cpp:30:80: error: no member named 'full_name' in 'boost::unit_test::test_case'
        fs::path ph = getPath(boost::unit_test::framework::current_test_case().full_name().append(obfuscate?"_true":"_false"));
                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^

Guess it is only for newer boost? As this is in the test case itself,

• might be just as well to simply hardcode the name instead of boost::unit_test::framework::current_test_case().full_name().
• or maybe __func__ works?

Looks like full_name() might be new in boost v1.60 which is much higher than the 1.47 minimum listed here.

__func__ doesn't work, after the macro is done it always returned test_method.

I'll go ahead and hardcode the names.

All the feedback should be implemented now:

• Moved temp directory business into BasicTestingSetup
• Switched from boost::unit_test::framework::current_test_case().full_name() to a hard coded name.
• Style recommendations.

It looked like moving the temp directory business into BasicTestingSetup created quite a bit of overhead since directories were created/deleted for every test. So I added a flag and now that is only done if getPath is called.

After all of this, one of the Travis targets still fails:

The command "if [ "$RUN_TESTS" = "true" ]; then travis_wait 50 make $MAKEJOBS check VERBOSE=1; fi" exited with 2.
0.01s$ if [ "$TRAVIS_EVENT_TYPE" = "cron" ]; then extended="--extended --exclude feature_pruning,feature_dbcrash"; fi

Is this a regression due to the duration? I'm not really sure how to proceed, any suggestions for how to speed this up?

Note that you only set the datadir when calling getPath, so I'd assume that the test fails because TxIndexDB is set up with the wrong datadir?

C.f. https://dev.visucore.com/bitcoin/doxygen/txdb_8cpp_source.html#l00429

@MarcoFalke thanks, that fixed it.

Just some style comments

It looked like moving the temp directory business into BasicTestingSetup created quite a bit of overhead since directories were created/deleted for every test. So I added a flag and now that is only done if getPath is called.

Okay, whoops - maybe that wasn't such a good idea then. I was confused. Maybe the test fixture isn't a good place because indeed, it's re-created for every suite (or maybe even every test?).

My intent with #13145 was to create a test directory globally, so that all tests (at least those running in the same `test_bitcoin) invocation put their output in subdirectories of one directory, cleaned up at the end. If it still ends up creating a directory per test, this is no win.

@laanwj I think my latest changes accomplish this with these two additions:

1. the directory is now only created if getPath is called, and getPath is available to any test using anything extending the basic fixture.
2. the default root path now starts with fs::temp_directory_path() / "test_bitcoin", so all temp directories will be nested under the same path. This means that multiple invokations will re-use the same directory - which may not be desirable.

If this doesn't sound good enough, I could explore an alternate solution. It looks like a custom parameter could be created for the test binary, so perhaps make should create/delete the temporary directory and pass that into test_bitcoin.

There is a GLOBAL_TEST_FIXTURE feature in boost test which is probably "the right way" to implement pre/post tasks. However, it isn't compatible with the make script we use to enable running suites in parallel.

Please let me know what you think the next step should be.

Needs rebase

Could you please squash the merge commit:

git reset --soft origin/master
git commit -m '$The original commit title
and text
...'

@MarcoFalke all commits have been squashed, and the branch rebased.

@laanwj Are there any additional changes needed to get this merged/closed? If this isn't the right approach, and the reasoning I mentioned on May 3 doesn't seem sufficient, please go ahead and close the PR. Thanks!

Sorry, kind of lost track of this, will take a new look soon.

Tested ACK 5f70385f3a16e07263a608d039681e0abdf9658a

Just some nits about renaming:

diff --git a/src/test/dbwrapper_tests.cpp b/src/test/dbwrapper_tests.cpp
index f8946ed222..fac7418cba 100644
--- a/src/test/dbwrapper_tests.cpp
+++ b/src/test/dbwrapper_tests.cpp
@@ -27,7 +27,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper)
 {
     // Perform tests both obfuscated and non-obfuscated.
     for (bool obfuscate : {false, true}) {
-        fs::path ph = getPath(std::string("dbwrapper").append(obfuscate ? "_true" : "_false"));
+        fs::path ph = SetDataDir(std::string("dbwrapper").append(obfuscate ? "_true" : "_false"));
         CDBWrapper dbw(ph, (1 << 20), true, false, obfuscate);
         char key = 'k';
         uint256 in = InsecureRand256();
@@ -47,7 +47,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper_batch)
 {
     // Perform tests both obfuscated and non-obfuscated.
     for (bool obfuscate : {false, true}) {
-        fs::path ph = getPath(std::string("dbwrapper_batch").append(obfuscate ? "_true" : "_false"));
+        fs::path ph = SetDataDir(std::string("dbwrapper_batch").append(obfuscate ? "_true" : "_false"));
         CDBWrapper dbw(ph, (1 << 20), true, false, obfuscate);
 
         char key = 'i';
@@ -83,7 +83,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper_iterator)
 {
     // Perform tests both obfuscated and non-obfuscated.
     for (bool obfuscate : {false, true}) {
-        fs::path ph = getPath(std::string("dbwrapper_iterator").append(obfuscate ? "_true" : "_false"));
+        fs::path ph = SetDataDir(std::string("dbwrapper_iterator").append(obfuscate ? "_true" : "_false"));
         CDBWrapper dbw(ph, (1 << 20), true, false, obfuscate);
 
         // The two keys are intentionally chosen for ordering
@@ -123,7 +123,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 = getPath("existing_data_no_obfuscate");
+    fs::path ph = SetDataDir("existing_data_no_obfuscate");
     create_directories(ph);
 
     // Set up a non-obfuscated wrapper to write some initial data.
@@ -164,7 +164,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 = getPath("existing_data_reindex");
+    fs::path ph = SetDataDir("existing_data_reindex");
     create_directories(ph);
 
     // Set up a non-obfuscated wrapper to write some initial data.
@@ -199,7 +199,7 @@ BOOST_AUTO_TEST_CASE(existing_data_reindex)
 
 BOOST_AUTO_TEST_CASE(iterator_ordering)
 {
-    fs::path ph = getPath("iterator_ordering");
+    fs::path ph = SetDataDir("iterator_ordering");
     CDBWrapper dbw(ph, (1 << 20), true, false, false);
     for (int x=0x00; x<256; ++x) {
         uint8_t key = x;
@@ -277,7 +277,7 @@ BOOST_AUTO_TEST_CASE(iterator_string_ordering)
 {
     char buf[10];
 
-    fs::path ph = getPath("iterator_string_ordering");
+    fs::path ph = SetDataDir("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++) {
diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp
index 124bdb56b1..6ede65c23a 100644
--- a/src/test/test_bitcoin.cpp
+++ b/src/test/test_bitcoin.cpp
@@ -45,7 +45,8 @@ std::ostream& operator<<(std::ostream& os, const uint256& num)
     return os;
 }
 
-BasicTestingSetup::BasicTestingSetup(const std::string& chainName): m_path_root(fs::temp_directory_path() / "test_bitcoin" / strprintf("%lu_%i", (unsigned long)GetTime(), (int)(InsecureRandRange(1 << 30)))), m_cleanup_root(false)
+BasicTestingSetup::BasicTestingSetup(const std::string& chainName)
+    : m_path_root(fs::temp_directory_path() / "test_bitcoin" / strprintf("%lu_%i", (unsigned long)GetTime(), (int)(InsecureRandRange(1 << 30))))
 {
     SHA256AutoDetect();
     RandomInit();
@@ -61,20 +62,21 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName): m_path_root(
 
 BasicTestingSetup::~BasicTestingSetup()
 {
-    if (m_cleanup_root) fs::remove_all(m_path_root);
+    fs::remove_all(m_path_root);
     ECC_Stop();
 }
 
-fs::path BasicTestingSetup::getPath(const std::string& name) {
+fs::path BasicTestingSetup::SetDataDir(const std::string& name)
+{
     fs::path ret = m_path_root / name;
     fs::create_directories(ret);
     gArgs.ForceSetArg("-datadir", ret.string());
-    m_cleanup_root = true;
     return ret;
 }
 
-TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(chainName), m_path_temp(getPath("tempdir"))
+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.
diff --git a/src/test/test_bitcoin.h b/src/test/test_bitcoin.h
index 6797edd960..88b2d37e87 100644
--- a/src/test/test_bitcoin.h
+++ b/src/test/test_bitcoin.h
@@ -46,11 +46,10 @@ struct BasicTestingSetup {
     explicit BasicTestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
     ~BasicTestingSetup();
 
-    fs::path getPath(const std::string& name);
+    fs::path SetDataDir(const std::string& name);
 
-    private:
-        const fs::path m_path_root;
-        bool m_cleanup_root;
+private:
+    const fs::path m_path_root;
 };
 
 /** Testing setup that configures a complete environment.
@@ -72,10 +71,6 @@ struct TestingSetup: public BasicTestingSetup {
 
     explicit TestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
     ~TestingSetup();
-
-    private:
-        // Some tests require -datadir to be set.
-        const fs::path m_path_temp;
 };
 
 class CBlock;
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 3a7a1799a8..d535f74e91 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -1100,7 +1100,7 @@ static void TestOtherProcess(fs::path dirname, std::string lockname, int fd)
 
 BOOST_AUTO_TEST_CASE(test_LockDirectory)
 {
-    fs::path dirname = getPath("test_LockDirectory") / fs::unique_path();
+    fs::path dirname = SetDataDir("test_LockDirectory") / fs::unique_path();
     const std::string lockname = ".lock";
 #ifndef WIN32
     // Revert SIGCHLD to default, otherwise boost.test will catch and fail on
@@ -1188,8 +1188,8 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory)
 
 BOOST_AUTO_TEST_CASE(test_DirIsWritable)
 {
-    // Should be able to write to the system tmp dir.
-    fs::path tmpdirname = getPath("test_DirIsWritable");
+    // Should be able to write to the data dir.
+    fs::path tmpdirname = SetDataDir("test_DirIsWritable");
     BOOST_CHECK_EQUAL(DirIsWritable(tmpdirname), true);
 
     // Should not be able to write to a non-existent dir.
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 900ab3cca7..a946b565f1 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -130,7 +130,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
 
     LOCK(cs_main);
 
-    std::string dir = (getPath("importwallet_rescan") / "wallet.backup").string();
+    std::string backup_file = (SetDataDir("importwallet_rescan") / "wallet.backup").string();
 
     // Import key into wallet and call dumpwallet to create backup file.
     {
@@ -141,7 +141,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
 
         JSONRPCRequest request;
         request.params.setArray();
-        request.params.push_back(dir);
+        request.params.push_back(backup_file);
         AddWallet(wallet);
         ::dumpwallet(request);
         RemoveWallet(wallet);
@@ -154,7 +154,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
 
         JSONRPCRequest request;
         request.params.setArray();
-        request.params.push_back(dir);
+        request.params.push_back(backup_file);
         AddWallet(wallet);
         ::importwallet(request);
         RemoveWallet(wallet);

Feel free to just apply the diff, if you agree with it, and squash it into your commit.

@MarcoFalke sounds good. My only question is that you put SetDataDir("tempdir"); in the testing setup, so a directory will be created/deleted for every test regardless of whether or not it's needed. Do you think that matters?

I am pretty sure that this is already being done in current master c.f. fs::create_directories(pathTemp); or the part in your patch where you initialize m_path_temp(getPath("tempdir")) in the constructor, no?

True. Patch applied.

Excellent first-time contribution! Thanks for sticking with this!

re-ACK 075429a482