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

pull winder wants to merge 1 commits into bitcoin:master from winder:master changing 5 files +42 −29
  1. winder commented at 1:27 am on May 2, 2018: contributor

    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.

  2. fanquake added the label Tests on May 2, 2018
  3. in src/test/dbwrapper_tests.cpp:30 in 6dd7648447 outdated 
    27 BOOST_AUTO_TEST_CASE(dbwrapper)
28 {
29     // Perform tests both obfuscated and non-obfuscated.
30     for (bool obfuscate : {false, true}) {
31-        fs::path ph = fs::temp_directory_path() / fs::unique_path();
32+        fs::path ph = getPath(boost::unit_test::framework::current_test_case().full_name().append(obfuscate?"_true":"_false"));
    practicalswift commented at 9:17 am on May 2, 2018:
    Nit: obfuscate ? "_true" : "_false" instead of obfuscate?"_true":"_false" to increase readability. Also below :-)
  4. in src/test/test_bitcoin.h:70 in 6dd7648447 outdated 
    66@@ -67,6 +67,7 @@ struct TestingSetup: public BasicTestingSetup {
67 
68     explicit TestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
69     ~TestingSetup();
70+    fs::path getPath(const std::string& name);
    laanwj commented at 9:56 am on May 2, 2018:
    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?)
    winder commented at 12:09 pm on May 2, 2018:
    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.
  5. laanwj commented at 10:56 am on May 2, 2018: member

    Failure if I try to build locally - :

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

    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?
  6. winder commented at 12:38 pm on May 2, 2018: contributor

    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.

  7. winder commented at 3:49 pm on May 2, 2018: contributor

    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:

    0The command "if [ "$RUN_TESTS" = "true" ]; then travis_wait 50 make $MAKEJOBS check VERBOSE=1; fi" exited with 2.
10.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?

  8. MarcoFalke commented at 4:08 pm on May 2, 2018: member

    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

  9. winder commented at 4:47 pm on May 2, 2018: contributor
    @MarcoFalke thanks, that fixed it.
  10. in src/test/test_bitcoin.h:53 in 9bab7e18ab outdated 
    44@@ -45,6 +45,12 @@ struct BasicTestingSetup {
45 
46     explicit BasicTestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
47     ~BasicTestingSetup();
48+
49+    fs::path getPath(const std::string& name);
50+
51+    private:
52+        fs::path pathRoot;
53+        bool cleanupRoot;
    MarcoFalke commented at 4:52 pm on May 2, 2018:

    nit: New members should follow the naming guidelines:

    0m_path_root;
1m_cleanup_root;
    MarcoFalke commented at 4:53 pm on May 2, 2018:
    Also, the members should probably be const and initialized in the constructor initializer list?
  11. in src/test/test_bitcoin.h:78 in 9bab7e18ab outdated 
    74     explicit TestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
75     ~TestingSetup();
76+
77+    private:
78+        // Some tests require -datadir to be set.
79+        fs::path pathTemp = getPath("tempdir");
    MarcoFalke commented at 4:52 pm on May 2, 2018:
    Should be done in the constructor to make sure m_path_root is properly initialized?
    MarcoFalke commented at 4:54 pm on May 2, 2018:
    Also, could be const if possible?
  12. MarcoFalke commented at 4:54 pm on May 2, 2018: member
    Just some style comments
  13. laanwj commented at 12:39 pm on May 3, 2018: member

    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.

  14. winder commented at 1:47 pm on May 3, 2018: contributor

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

  15. MarcoFalke added the label Needs rebase on Jun 6, 2018
  16. MarcoFalke commented at 12:23 pm on June 11, 2018: member
    Needs rebase
  17. winder force-pushed on Jun 11, 2018
  18. MarcoFalke removed the label Needs rebase on Jun 11, 2018
  19. MarcoFalke commented at 2:42 pm on June 11, 2018: member

    Could you please squash the merge commit:

    0git reset --soft origin/master
1git commit -m '$The original commit title
2and text
3...'
  20. winder force-pushed on Jun 11, 2018
  21. winder force-pushed on Jun 11, 2018
  22. winder commented at 4:28 pm on June 11, 2018: contributor
    @MarcoFalke all commits have been squashed, and the branch rebased.
  23. winder commented at 5:43 pm on June 30, 2018: contributor
    @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!
  24. laanwj commented at 7:02 pm on June 30, 2018: member
    Sorry, kind of lost track of this, will take a new look soon.
  25. in src/test/test_bitcoin.cpp:64 in 5f70385f3a outdated 
    71 }
72 
73 BasicTestingSetup::~BasicTestingSetup()
74 {
75-        ECC_Stop();
76+    if (m_cleanup_root) fs::remove_all(m_path_root);
    MarcoFalke commented at 1:36 pm on July 10, 2018:
    nit: no need for m_cleanup_root, since remove_all is a noop on if the dir doesn’t exist.
  26. in src/test/test_bitcoin.h:78 in 5f70385f3a outdated 
    74     explicit TestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
75     ~TestingSetup();
76+
77+    private:
78+        // Some tests require -datadir to be set.
79+        const fs::path m_path_temp;
    MarcoFalke commented at 1:36 pm on July 10, 2018:
    No need for this member, since it is never read?
  27. in src/test/util_tests.cpp:1191 in 5f70385f3a outdated 
    1188@@ -1189,11 +1189,11 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory)
1189 BOOST_AUTO_TEST_CASE(test_DirIsWritable)
1190 {
1191     // Should be able to write to the system tmp dir.
    MarcoFalke commented at 1:37 pm on July 10, 2018:
    “system tmp dir” should be “datadir”, now.
  28. in src/wallet/test/wallet_tests.cpp:133 in 5f70385f3a outdated 
    129@@ -130,6 +130,8 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
130 
131     LOCK(cs_main);
132 
133+    std::string dir = (getPath("importwallet_rescan") / "wallet.backup").string();
    MarcoFalke commented at 1:38 pm on July 10, 2018:
    This is not a dir, but a wallet backup file name
  29. in src/test/test_bitcoin.cpp:68 in 5f70385f3a outdated 
    76+    if (m_cleanup_root) fs::remove_all(m_path_root);
77+    ECC_Stop();
78 }
79 
80-TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(chainName)
81+fs::path BasicTestingSetup::getPath(const std::string& name) {
    MarcoFalke commented at 1:40 pm on July 10, 2018:
    Should rename this to SetDataDir()?
  30. MarcoFalke approved
  31. MarcoFalke commented at 1:50 pm on July 10, 2018: member

    Tested ACK 5f70385f3a16e07263a608d039681e0abdf9658a

    Just some nits about renaming:

      0diff --git a/src/test/dbwrapper_tests.cpp b/src/test/dbwrapper_tests.cpp
  1index f8946ed222..fac7418cba 100644
  2--- a/src/test/dbwrapper_tests.cpp
  3+++ b/src/test/dbwrapper_tests.cpp
  4@@ -27,7 +27,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper)
  5 {
  6     // Perform tests both obfuscated and non-obfuscated.
  7     for (bool obfuscate : {false, true}) {
  8-        fs::path ph = getPath(std::string("dbwrapper").append(obfuscate ? "_true" : "_false"));
  9+        fs::path ph = SetDataDir(std::string("dbwrapper").append(obfuscate ? "_true" : "_false"));
 10         CDBWrapper dbw(ph, (1 << 20), true, false, obfuscate);
 11         char key = 'k';
 12         uint256 in = InsecureRand256();
 13@@ -47,7 +47,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper_batch)
 14 {
 15     // Perform tests both obfuscated and non-obfuscated.
 16     for (bool obfuscate : {false, true}) {
 17-        fs::path ph = getPath(std::string("dbwrapper_batch").append(obfuscate ? "_true" : "_false"));
 18+        fs::path ph = SetDataDir(std::string("dbwrapper_batch").append(obfuscate ? "_true" : "_false"));
 19         CDBWrapper dbw(ph, (1 << 20), true, false, obfuscate);
 20 
 21         char key = 'i';
 22@@ -83,7 +83,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper_iterator)
 23 {
 24     // Perform tests both obfuscated and non-obfuscated.
 25     for (bool obfuscate : {false, true}) {
 26-        fs::path ph = getPath(std::string("dbwrapper_iterator").append(obfuscate ? "_true" : "_false"));
 27+        fs::path ph = SetDataDir(std::string("dbwrapper_iterator").append(obfuscate ? "_true" : "_false"));
 28         CDBWrapper dbw(ph, (1 << 20), true, false, obfuscate);
 29 
 30         // The two keys are intentionally chosen for ordering
 31@@ -123,7 +123,7 @@ BOOST_AUTO_TEST_CASE(dbwrapper_iterator)
 32 BOOST_AUTO_TEST_CASE(existing_data_no_obfuscate)
 33 {
 34     // We're going to share this fs::path between two wrappers
 35-    fs::path ph = getPath("existing_data_no_obfuscate");
 36+    fs::path ph = SetDataDir("existing_data_no_obfuscate");
 37     create_directories(ph);
 38 
 39     // Set up a non-obfuscated wrapper to write some initial data.
 40@@ -164,7 +164,7 @@ BOOST_AUTO_TEST_CASE(existing_data_no_obfuscate)
 41 BOOST_AUTO_TEST_CASE(existing_data_reindex)
 42 {
 43     // We're going to share this fs::path between two wrappers
 44-    fs::path ph = getPath("existing_data_reindex");
 45+    fs::path ph = SetDataDir("existing_data_reindex");
 46     create_directories(ph);
 47 
 48     // Set up a non-obfuscated wrapper to write some initial data.
 49@@ -199,7 +199,7 @@ BOOST_AUTO_TEST_CASE(existing_data_reindex)
 50 
 51 BOOST_AUTO_TEST_CASE(iterator_ordering)
 52 {
 53-    fs::path ph = getPath("iterator_ordering");
 54+    fs::path ph = SetDataDir("iterator_ordering");
 55     CDBWrapper dbw(ph, (1 << 20), true, false, false);
 56     for (int x=0x00; x<256; ++x) {
 57         uint8_t key = x;
 58@@ -277,7 +277,7 @@ BOOST_AUTO_TEST_CASE(iterator_string_ordering)
 59 {
 60     char buf[10];
 61 
 62-    fs::path ph = getPath("iterator_string_ordering");
 63+    fs::path ph = SetDataDir("iterator_string_ordering");
 64     CDBWrapper dbw(ph, (1 << 20), true, false, false);
 65     for (int x=0x00; x<10; ++x) {
 66         for (int y = 0; y < 10; y++) {
 67diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp
 68index 124bdb56b1..6ede65c23a 100644
 69--- a/src/test/test_bitcoin.cpp
 70+++ b/src/test/test_bitcoin.cpp
 71@@ -45,7 +45,8 @@ std::ostream& operator<<(std::ostream& os, const uint256& num)
 72     return os;
 73 }
 74 
 75-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)
 76+BasicTestingSetup::BasicTestingSetup(const std::string& chainName)
 77+    : m_path_root(fs::temp_directory_path() / "test_bitcoin" / strprintf("%lu_%i", (unsigned long)GetTime(), (int)(InsecureRandRange(1 << 30))))
 78 {
 79     SHA256AutoDetect();
 80     RandomInit();
 81@@ -61,20 +62,21 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName): m_path_root(
 82 
 83 BasicTestingSetup::~BasicTestingSetup()
 84 {
 85-    if (m_cleanup_root) fs::remove_all(m_path_root);
 86+    fs::remove_all(m_path_root);
 87     ECC_Stop();
 88 }
 89 
 90-fs::path BasicTestingSetup::getPath(const std::string& name) {
 91+fs::path BasicTestingSetup::SetDataDir(const std::string& name)
 92+{
 93     fs::path ret = m_path_root / name;
 94     fs::create_directories(ret);
 95     gArgs.ForceSetArg("-datadir", ret.string());
 96-    m_cleanup_root = true;
 97     return ret;
 98 }
 99 
100-TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(chainName), m_path_temp(getPath("tempdir"))
101+TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(chainName)
102 {
103+    SetDataDir("tempdir");
104     const CChainParams& chainparams = Params();
105         // Ideally we'd move all the RPC tests to the functional testing framework
106         // instead of unit tests, but for now we need these here.
107diff --git a/src/test/test_bitcoin.h b/src/test/test_bitcoin.h
108index 6797edd960..88b2d37e87 100644
109--- a/src/test/test_bitcoin.h
110+++ b/src/test/test_bitcoin.h
111@@ -46,11 +46,10 @@ struct BasicTestingSetup {
112     explicit BasicTestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
113     ~BasicTestingSetup();
114 
115-    fs::path getPath(const std::string& name);
116+    fs::path SetDataDir(const std::string& name);
117 
118-    private:
119-        const fs::path m_path_root;
120-        bool m_cleanup_root;
121+private:
122+    const fs::path m_path_root;
123 };
124 
125 /** Testing setup that configures a complete environment.
126@@ -72,10 +71,6 @@ struct TestingSetup: public BasicTestingSetup {
127 
128     explicit TestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
129     ~TestingSetup();
130-
131-    private:
132-        // Some tests require -datadir to be set.
133-        const fs::path m_path_temp;
134 };
135 
136 class CBlock;
137diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
138index 3a7a1799a8..d535f74e91 100644
139--- a/src/test/util_tests.cpp
140+++ b/src/test/util_tests.cpp
141@@ -1100,7 +1100,7 @@ static void TestOtherProcess(fs::path dirname, std::string lockname, int fd)
142 
143 BOOST_AUTO_TEST_CASE(test_LockDirectory)
144 {
145-    fs::path dirname = getPath("test_LockDirectory") / fs::unique_path();
146+    fs::path dirname = SetDataDir("test_LockDirectory") / fs::unique_path();
147     const std::string lockname = ".lock";
148 #ifndef WIN32
149     // Revert SIGCHLD to default, otherwise boost.test will catch and fail on
150@@ -1188,8 +1188,8 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory)
151 
152 BOOST_AUTO_TEST_CASE(test_DirIsWritable)
153 {
154-    // Should be able to write to the system tmp dir.
155-    fs::path tmpdirname = getPath("test_DirIsWritable");
156+    // Should be able to write to the data dir.
157+    fs::path tmpdirname = SetDataDir("test_DirIsWritable");
158     BOOST_CHECK_EQUAL(DirIsWritable(tmpdirname), true);
159 
160     // Should not be able to write to a non-existent dir.
161diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
162index 900ab3cca7..a946b565f1 100644
163--- a/src/wallet/test/wallet_tests.cpp
164+++ b/src/wallet/test/wallet_tests.cpp
165@@ -130,7 +130,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
166 
167     LOCK(cs_main);
168 
169-    std::string dir = (getPath("importwallet_rescan") / "wallet.backup").string();
170+    std::string backup_file = (SetDataDir("importwallet_rescan") / "wallet.backup").string();
171 
172     // Import key into wallet and call dumpwallet to create backup file.
173     {
174@@ -141,7 +141,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
175 
176         JSONRPCRequest request;
177         request.params.setArray();
178-        request.params.push_back(dir);
179+        request.params.push_back(backup_file);
180         AddWallet(wallet);
181         ::dumpwallet(request);
182         RemoveWallet(wallet);
183@@ -154,7 +154,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
184 
185         JSONRPCRequest request;
186         request.params.setArray();
187-        request.params.push_back(dir);
188+        request.params.push_back(backup_file);
189         AddWallet(wallet);
190         ::importwallet(request);
191         RemoveWallet(wallet);
  32. MarcoFalke commented at 2:18 pm on July 11, 2018: member
    Feel free to just apply the diff, if you agree with it, and squash it into your commit.
  33. winder commented at 8:23 pm on July 11, 2018: contributor
    @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?
  34. MarcoFalke commented at 11:00 pm on July 11, 2018: member
    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?
  35. Use common SetDataDir method to create temp directory in tests. 075429a482
  36. winder force-pushed on Jul 12, 2018
  37. winder commented at 3:45 am on July 12, 2018: contributor
    True. Patch applied.
  38. MarcoFalke commented at 11:48 am on July 12, 2018: member

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

    re-ACK 075429a482

  39. MarcoFalke merged this on Jul 12, 2018
  40. MarcoFalke closed this on Jul 12, 2018
  41. MarcoFalke referenced this in commit d3dae3ddf9 on Jul 12, 2018
  42. PastaPastaPasta referenced this in commit 6372283967 on Dec 16, 2020
  43. PastaPastaPasta referenced this in commit 5c7c9a00da on Dec 18, 2020
  44. PastaPastaPasta referenced this in commit 2a69d81fae on Dec 18, 2020
  45. furszy referenced this in commit 6b95c76daf on May 15, 2021
  46. MarcoFalke locked this on Sep 8, 2021


winder practicalswift laanwj MarcoFalke

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-05 01:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me