-
Notifications
You must be signed in to change notification settings - Fork 36.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use common getPath method to create temp directory in tests. #13145
Conversation
src/test/dbwrapper_tests.cpp
Outdated
|
||
BOOST_AUTO_TEST_CASE(dbwrapper) | ||
{ | ||
// Perform tests both obfuscated and non-obfuscated. | ||
for (bool obfuscate : {false, true}) { | ||
fs::path ph = fs::temp_directory_path() / fs::unique_path(); | ||
fs::path ph = getPath(boost::unit_test::framework::current_test_case().full_name().append(obfuscate?"_true":"_false")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: obfuscate ? "_true" : "_false"
instead of obfuscate?"_true":"_false"
to increase readability. Also below :-)
src/test/test_bitcoin.h
Outdated
@@ -67,6 +67,7 @@ struct TestingSetup: public BasicTestingSetup { | |||
|
|||
explicit TestingSetup(const std::string& chainName = CBaseChainParams::MAIN); | |||
~TestingSetup(); | |||
fs::path getPath(const std::string& name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Failure if I try to build locally - :
Guess it is only for newer boost? As this is in the test case itself,
|
Looks like
I'll go ahead and hardcode the names. |
All the feedback should be implemented now:
It looked like moving the temp directory business into After all of this, one of the Travis targets still fails:
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 C.f. https://dev.visucore.com/bitcoin/doxygen/txdb_8cpp_source.html#l00429 |
@MarcoFalke thanks, that fixed it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some style comments
src/test/test_bitcoin.h
Outdated
|
||
private: | ||
fs::path pathRoot; | ||
bool cleanupRoot; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: New members should follow the naming guidelines:
m_path_root;
m_cleanup_root;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the members should probably be const and initialized in the constructor initializer list?
src/test/test_bitcoin.h
Outdated
|
||
private: | ||
// Some tests require -datadir to be set. | ||
fs::path pathTemp = getPath("tempdir"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be done in the constructor to make sure m_path_root
is properly initialized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, could be const if possible?
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:
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 There is a Please let me know what you think the next step should be. |
Needs rebase |
Could you please squash the merge commit:
|
f115f7d
to
5f70385
Compare
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
src/test/test_bitcoin.cpp
Outdated
} | ||
|
||
BasicTestingSetup::~BasicTestingSetup() | ||
{ | ||
ECC_Stop(); | ||
if (m_cleanup_root) fs::remove_all(m_path_root); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: no need for m_cleanup_root
, since remove_all
is a noop on if the dir doesn't exist.
src/test/test_bitcoin.h
Outdated
|
||
private: | ||
// Some tests require -datadir to be set. | ||
const fs::path m_path_temp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this member, since it is never read?
src/test/util_tests.cpp
Outdated
@@ -1189,11 +1189,11 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory) | |||
BOOST_AUTO_TEST_CASE(test_DirIsWritable) | |||
{ | |||
// Should be able to write to the system tmp dir. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"system tmp dir" should be "datadir", now.
src/wallet/test/wallet_tests.cpp
Outdated
@@ -130,6 +130,8 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) | |||
|
|||
LOCK(cs_main); | |||
|
|||
std::string dir = (getPath("importwallet_rescan") / "wallet.backup").string(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a dir, but a wallet backup file name
src/test/test_bitcoin.cpp
Outdated
} | ||
|
||
TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(chainName) | ||
fs::path BasicTestingSetup::getPath(const std::string& name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should rename this to SetDataDir()
?
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 |
I am pretty sure that this is already being done in current master c.f. |
True. Patch applied. |
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
…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
…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
Took a stab at #12574
Created a
getPath
method which can be used with theTestingSetup
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 includingboost/test/unit_test.hpp
intest_bitcoin.cpp
. Even if I had gotten the linking to work, it looks likemake check
invokes the test binary a bunch of times, so it may not have worked anyway.