Skip to content
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

Config test uses unique directories for each test #1984

Closed
wants to merge 14 commits into from

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Jan 27, 2017

  • This fixes an uncommon, but annoying, spurious failure running this
    test, particularly in release builds. This appears to be an issue with
    Windows of the FS where quickly creating and deleting the same
    directory repeatedly will eventually fail.

@codecov-io
Copy link

codecov-io commented Jan 28, 2017

Codecov Report

Merging #1984 into develop will increase coverage by 0.22%.

@@             Coverage Diff             @@
##           develop    #1984      +/-   ##
===========================================
+ Coverage     67.2%   67.42%   +0.22%     
===========================================
  Files          683      687       +4     
  Lines        49208    49640     +432     
===========================================
+ Hits         33071    33472     +401     
- Misses       16137    16168      +31
Impacted Files Coverage Δ
src/ripple/net/HTTPClient.h 0% <ø> (ø)
src/ripple/server/Port.h 100% <ø> (ø)
src/ripple/app/tx/impl/Payment.cpp 78.87% <ø> (-0.1%)
src/ripple/rpc/impl/Handler.cpp 97.67% <ø> (ø)
src/ripple/core/Config.h 100% <ø> (ø)
src/ripple/app/paths/Pathfinder.cpp 86.18% <ø> (-0.08%)
src/ripple/app/misc/TxQ.h 96.77% <ø> (ø)
src/ripple/protocol/impl/BuildInfo.cpp 82.6% <ø> (ø)
src/ripple/app/ledger/LedgerMaster.h 83.33% <ø> (ø)
src/ripple/app/ledger/LedgerConsensus.h 100% <ø> (ø)
... and 87 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3eada1...de3ddcf. Read the comment docs.

path const dataDirAbs (cwd / path ("test_db") / dataDirRel);
detail::RippledCfgGuard g (*this, "test_db", dataDirAbs.string (), "");
path const dataDirAbs (cwd / path ("test_db00") / dataDirRel);
detail::RippledCfgGuard g (*this, "test_db00",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could avoid manually managing counters by adding a path subdir() function to ConfigGuard.
In this case, you would initialize the RippledCfgGuard first and then initialize dataDirAbs as:

path const dataDirAbs (cwd / g.subdir() / dataDirRel);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a dependency loop because dataDirAbs is a param to create g, but I am pushing a nice solution.

@ximinez
Copy link
Collaborator Author

ximinez commented Jan 30, 2017

Rebased on 0.50.1

Copy link
Contributor

@wilsonianb wilsonianb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -184,12 +188,17 @@ class ConfigGuard
test_.log << "Error in ~ConfigGuard: " << e.what () << std::endl;
};
}

path subdir() const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider returning path const&

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

detail::RippledCfgGuard g (*this, "test_db", dataDirAbs.string (), "");
path const dataDirAbs(cwd / g0.subdir () / dataDirRel);
detail::RippledCfgGuard g (*this, g0.subdir().string(),
dataDirAbs.string (), "", false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing subDir in the ConfigGuard/RippledCfgGuard/ValidatorsTxtGuard constructors to a path would save you from having to call .string() on all these subdirectory paths.
(Same with RippledCfgGuard's dbPath.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Will do.

Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 The pull request looks like it will do what it intended to do. I made a number of minor notes about const and the like. If you like those comments you could cherry-pick this commit: scottschurr@8d3ad47. But that's purely optional.

std::string subDir, std::string const& dbPath,
std::string const& validatorsFile)
: ConfigGuard (test, std::move (subDir)), dataDir_ (dbPath)
path subDir, path const& dbPath,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note you are (appropriately) turning many things that were formerly strings into paths. Do you want to do that with the configContents (bdPath, ...) argument as well (line 32)? Just wondering, not insisting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not in this case, because configContents is using those params to build a string, and it doesn't make sense to me to force the input to be a path when we only want a string. If it was more widely used, and not just test code, I'd argue that a caller might want to just pass in a string, and promoting it to path only to just use a string is unnecessary.

I am going to take the rest of your const changes, though. Those are nice!

: subDir_ (std::move (subDir))
, test_ (test)
{
using namespace boost::filesystem;
{
static auto subDirCounter = 0;
if (useCounter)
subDir_ += to_string(++subDirCounter);
if (!exists (subDir_))
{
create_directory (subDir_);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Between lines 171 and 172, it might be nice to add some vertical white space between the constructor and destructor implementations.

@@ -213,7 +224,7 @@ class RippledCfgGuard : ConfigGuard
if (!exists (configFile_))
{
std::ofstream o (configFile_.string ());
o << configContents (dbPath, validatorsFile);
o << configContents (dbPath.string (), validatorsFile.string ());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you convert the dbPath argument on line 32 to a path, then you can lose the .string() on this dbPath.

@@ -422,9 +433,11 @@ port_wss_admin
{
// read from file absolute path
auto const cwd = current_path ();
detail::ConfigGuard g0(*this, "test_db");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const g0?

@@ -223,7 +234,7 @@ class RippledCfgGuard : ConfigGuard
}

rmDataDir_ = !exists (dataDir_);
config_.setup (configFile_.string (), /*bQuiet*/ false,
config_.setup (configFile_.string (), /*bQuiet*/ true,
/* bSilent */ false, /* bStandalone */ false);
}
Config& config ()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making this member Config const& config() const. Then I think you can make your RippledCfgGuard instances const.

@@ -481,7 +494,7 @@ port_wss_admin
detail::ValidatorsTxtGuard vtg (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const vtg?

@@ -481,7 +494,7 @@ port_wss_admin
detail::ValidatorsTxtGuard vtg (
*this, "test_cfg", "validators.cfg", quorum);
Config c;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving Config c inside the try where it is first used.

@ximinez
Copy link
Collaborator Author

ximinez commented Feb 1, 2017

Thanks for the suggestions, @scottschurr. I've pulled them in, made some tweaks, and pushed. Marking this PR as passed.

@ximinez ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Feb 1, 2017
@ximinez ximinez force-pushed the testdirretry branch 2 times, most recently from 5e20751 to 76eaab6 Compare February 1, 2017 22:54
@ximinez
Copy link
Collaborator Author

ximinez commented Feb 1, 2017

Rebased on 0.60.0-b2 and squashed.

@ximinez
Copy link
Collaborator Author

ximinez commented Feb 2, 2017

Rebased on 0.60.0-b3

wilsonianb and others added 11 commits February 7, 2017 17:10
Instead of specifying a static list of trusted validators in the config
or validators file, the configuration can now include trusted validator
list publisher keys.

The trusted validator list and quorum are now reset each consensus
round using the latest validator lists and the list of recent
validations seen. The minimum validation quorum is now only
configurable via the command line.
Validator lists from configured remote sites are fetched at a regular
interval. Fetched lists are expected to be in JSON format and contain the
following fields:

* "manifest": Base64-encoded serialization of a manifest containing the
  validator publisher's master and signing public keys.

* "blob": Base64-encoded JSON string containing a "sequence",
  "expiration" and "validators" field. "expiration" contains the Ripple
   timestamp (seconds since January 1st, 2000 (00:00 UTC)) for when the
  list expires. "validators" contains an array of objects with a
  "validation_public_key" field.

* "signature": Hex-encoded signature of the blob using the publisher's
  signing key.

* "version": 1

* "refreshInterval" (optional)
Change some uses of std::cerr to log or cout.
It is often difficult to get access to the preset features in the config. Adding
the preset features solves this problem.
vinniefalco and others added 3 commits February 7, 2017 17:13
This also fixes a defect where the Server HTTP header was
incorrectly set in WebSocket Upgrade handshake responses.
* This fixes an uncommon, but annoying, spurious failure running this
  test, particularly in release builds. This appears to be an issue with
  Windows of the FS where quickly creating and deleting the same
  directory repeatedly will eventually fail.
* RIPD-1390
@ximinez
Copy link
Collaborator Author

ximinez commented Feb 8, 2017

Merged as b514f1a

@ximinez ximinez closed this Feb 8, 2017
@ximinez ximinez deleted the testdirretry branch February 8, 2017 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants