Use common getPath method to create temp directory in tests. by winder · Pull Request #13145 · bitcoin/bitcoin

winder · 2018-05-02T01:27:26Z

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.

practicalswift · 2018-05-02T09:17:19Z

src/test/dbwrapper_tests.cpp

Nit: obfuscate ? "_true" : "_false" instead of obfuscate?"_true":"_false" to increase readability. Also below :-)

laanwj · 2018-05-02T09:56:03Z

src/test/test_bitcoin.h

I suppose if you move getPath to BasicTestingSetup, you don't have to move util_tests and dbwrapper_tests to TestingSetup? (or am I missing something here?)

I was using the existing (unused?) pathTemp in TestingSetup as a root directory, I could move that and the new method to BasicTestingSetup if that would be more appropriate.

laanwj · 2018-05-02T10:56:27Z

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?

winder · 2018-05-02T12:38:12Z

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.

winder · 2018-05-02T15:49:54Z

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?

maflcko · 2018-05-02T16:08:35Z

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

winder · 2018-05-02T16:47:21Z

@MarcoFalke thanks, that fixed it.

maflcko

Just some style comments

maflcko · 2018-05-02T16:52:29Z

src/test/test_bitcoin.h

nit: New members should follow the naming guidelines:

m_path_root; m_cleanup_root;

Also, the members should probably be const and initialized in the constructor initializer list?

maflcko · 2018-05-02T16:52:34Z

src/test/test_bitcoin.h

Should be done in the constructor to make sure m_path_root is properly initialized?

Also, could be const if possible?

laanwj · 2018-05-03T12:39:42Z

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.

winder · 2018-05-03T13:47:40Z

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

the directory is now only created if getPath is called, and getPath is available to any test using anything extending the basic fixture.
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.

maflcko · 2018-06-11T12:23:54Z

Needs rebase

maflcko · 2018-06-11T14:42:06Z

Could you please squash the merge commit:

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

winder · 2018-06-11T16:28:18Z

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

winder · 2018-06-30T17:43:06Z

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

laanwj · 2018-06-30T19:02:11Z

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

maflcko

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

maflcko · 2018-07-10T13:36:26Z

src/test/test_bitcoin.cpp

nit: no need for m_cleanup_root, since remove_all is a noop on if the dir doesn't exist.

maflcko · 2018-07-10T13:36:57Z

src/test/test_bitcoin.h

No need for this member, since it is never read?

maflcko · 2018-07-10T13:37:40Z

src/test/util_tests.cpp

"system tmp dir" should be "datadir", now.

maflcko · 2018-07-10T13:38:22Z

src/wallet/test/wallet_tests.cpp

This is not a dir, but a wallet backup file name

maflcko · 2018-07-10T13:40:11Z

src/test/test_bitcoin.cpp

Should rename this to SetDataDir()?

maflcko · 2018-07-11T14:18:59Z

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

winder · 2018-07-11T20:23:55Z

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

maflcko · 2018-07-11T23:00:28Z

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?

winder · 2018-07-12T03:45:16Z

True. Patch applied.

maflcko · 2018-07-12T11:48:42Z

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

re-ACK 075429a

…ests. 075429a Use common SetDataDir method to create temp directory in tests. (winder) Pull request description: 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. Tree-SHA512: b51d0f5fada5d652ccc9362596cf98a742aa47f5daf94f189b5f034d8c035c85d095377befdcff7fb4247154d5160e8c500d70f554a2158e2c185a9d24f694f1

…ry in tests. 075429a Use common SetDataDir method to create temp directory in tests. (winder) Pull request description: Took a stab at bitcoin#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. Tree-SHA512: b51d0f5fada5d652ccc9362596cf98a742aa47f5daf94f189b5f034d8c035c85d095377befdcff7fb4247154d5160e8c500d70f554a2158e2c185a9d24f694f1

…tem.h 3e48fc1 Renamed and moved util.h/cpp files to util/system.h & util/system.cpp (furszy) 7954cef Style cleanup. (Jim Posen) 48801a9 Use common SetDataDir method to create temp directory in tests. (winder) 06464d3 flatfile: Unit tests for FlatFileSeq methods. (Jim Posen) bbb9a90 scripted-diff: Rename CBlockDiskPos to FlatFilePos. (Jim Posen) 472857a Move CDiskBlockPos from chain to flatfile. (Jim Posen) 7fa47bb validation: Refactor file flush logic into FlatFileSeq. (Jim Posen) bf09076 validation: Refactor block file pre-allocation into FlatFileSeq. (Jim Posen) c813080 validation: Refactor OpenDiskFile into method on FlatFileSeq. (Jim Posen) 2619fe8 validation: Extract basic block file logic into FlatFileSeq class. (Jim Posen) e65acf0 util: Move CheckDiskSpace to util. (Jim Posen) Pull request description: This is part of the block logic refactoring and encapsulation work coming from upstream (work started in #2326 and #2328 due the possible blocks db corruption issue). Needed for two future works: (1) faster block validation using a new cache structure and (2) the future possible custom BIP157 (new sync protocol that supersedes BIP37 for light clients -- pending research needed after finishing v6.0 priorities --) Essentially, it's encapsulating the sequenced files open/read/write functionalities inside a new `flatfile` module, and extending the unit test coverage to validate its correct behavior. Adapted the following PRs: * bitcoin#15118. * bitcoin#13145. * Plus added `util.h/cpp` files mv to `util/system.h` & `util/system.cpp`. ACKs for top commit: random-zebra: utACK 3e48fc1 Tree-SHA512: c21ffb6ede054a279f9792c1cbe645b948078bd6bc607d32438f0601fd4df3650c0a34b349e46b4ea69b48f9e6c7bb18d258e139c6e1a47452ac9ea4f3bbee25

fanquake added the Tests label May 2, 2018

practicalswift reviewed May 2, 2018

View reviewed changes

laanwj reviewed May 2, 2018

View reviewed changes

maflcko reviewed May 2, 2018

View reviewed changes

maflcko added the Needs rebase label Jun 6, 2018

winder force-pushed the master branch from 28f6d55 to f115f7d Compare June 11, 2018 14:33

maflcko removed the Needs rebase label Jun 11, 2018

winder force-pushed the master branch 2 times, most recently from f115f7d to 5f70385 Compare June 11, 2018 15:43

ccdle12 mentioned this pull request Jun 12, 2018

[WIP] Unit test sub-directories - Continued #13067

Closed

This was referenced Jun 14, 2018

[bugfix] Fix encoding issue for Windows #13426

Closed

[net,mempool] Call AcceptToMemoryPool() asynchronously in p2p #13413

Closed

maflcko approved these changes Jul 10, 2018

View reviewed changes

Use common SetDataDir method to create temp directory in tests.

075429a

winder force-pushed the master branch from 5f70385 to 075429a Compare July 12, 2018 03:44

maflcko merged commit 075429a into bitcoin:master Jul 12, 2018

Bushstar mentioned this pull request Jul 13, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#354

Merged

furszy mentioned this pull request Apr 23, 2021

Refactor block file logic + util.h/cpp moved to util/system.h PIVX-Project/PIVX#2336

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

winder commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj May 2, 2018 • 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!

winder May 2, 2018 • 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!

laanwj commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

winder commented May 2, 2018

Uh oh!

winder commented May 2, 2018

Uh oh!

maflcko commented May 2, 2018

Uh oh!

winder commented May 2, 2018

Uh oh!

maflcko left a comment

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!

Choose a reason for hiding this comment

Uh oh!

laanwj commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

winder commented May 3, 2018

Uh oh!

maflcko commented Jun 11, 2018

Uh oh!

maflcko commented Jun 11, 2018

Uh oh!

winder commented Jun 11, 2018

Uh oh!

winder commented Jun 30, 2018

Uh oh!

laanwj commented Jun 30, 2018

Uh oh!

maflcko left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 11, 2018

Uh oh!

winder commented Jul 11, 2018

Uh oh!

maflcko commented Jul 11, 2018

Uh oh!

winder commented Jul 12, 2018

Uh oh!

maflcko commented Jul 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

winder commented May 2, 2018 •

edited

Loading

laanwj May 2, 2018 •

edited

Loading

winder May 2, 2018 •

edited

Loading

laanwj commented May 2, 2018 •

edited

Loading

laanwj commented May 3, 2018 •

edited

Loading