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

Add ledger save/load test (RIPD-1378) #1955

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
5 participants
@mellery451
Copy link
Contributor

commented Jan 5, 2017

Provide unit test to invoke ledger load at startup.

//------------------------------------------------------------------------------
/*
This file is part of rippled: https://github.com/ripple/rippled
Copyright (c) 2012-2017 Ripple Labs Inc.

This comment has been minimized.

Copy link
@vinniefalco

vinniefalco Jan 5, 2017

Contributor

Formatting is off here..

This comment has been minimized.

Copy link
@mellery451

mellery451 Jan 5, 2017

Author Contributor

will fix.

@codecov-io

This comment has been minimized.

Copy link

commented Jan 5, 2017

Current coverage is 67.11% (diff: 100%)

Merging #1955 into develop will increase coverage by 0.60%

@@            develop      #1955   diff @@
==========================================
  Files           687        687          
  Lines         49314      49314          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          32801      33097   +296   
+ Misses        16513      16217   -296   
  Partials          0          0          

Powered by Codecov. Last update 87273e2...c667c81

@vinniefalco

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2017

Nice coverage bump

@mellery451

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2017

This test mainly serves to offset the lost coverage in #1948, but it also provides slightly better overall coverage of the config LOAD scenarios.

@mellery451 mellery451 requested a review from ximinez Jan 5, 2017

@mellery451 mellery451 force-pushed the mellery451:ledger_load branch from 13d60ce to dd0f867 Jan 10, 2017

@ximinez
Copy link
Contributor

left a comment

Nice looking tests, and a nice boost in coverage. None of these suggestions are show stoppers, but will make the tests faster and IMHO clearer.


#include <BeastConfig.h>
#include <test/support/jtx.h>
#include <test/support/jtx/Env.h>

This comment has been minimized.

Copy link
@ximinez

ximinez Jan 10, 2017

Contributor

This #include may be redundant.

test::setupConfigForUnitTests(*p);
p->START_LEDGER = ledger;
p->START_UP = type;
p->legacy("database_path", dbPath_.string());

This comment has been minimized.

Copy link
@ximinez

ximinez Jan 10, 2017

Contributor

Part of me wants you to make ledgerConfig a static function, and pass dbPath_ as another param, just so there can be no accidental calls before dbPath_ is initialized. One reason for that is that we don't have many other unit tests that have any state at all.

This comment has been minimized.

Copy link
@mellery451

mellery451 Jan 10, 2017

Author Contributor

I see your point - what about just adding an assert() to verify dbPath_ is set?

This comment has been minimized.

Copy link
@ximinez

ximinez Jan 10, 2017

Contributor

Yeah, that's certainly simpler. 👍

This comment has been minimized.

Copy link
@vinniefalco

vinniefalco Jan 10, 2017

Contributor

idk about all that, the rules for test code are more relaxed compared to production code. We should not worry about "accidental" calls. For unit tests, we prioritize convenience and compactness of notation over robustness

}
env(offer(acct, XRP(100), acct["USD"](1)));
env.close();
}

This comment has been minimized.

Copy link
@ximinez

ximinez Jan 10, 2017

Contributor

IIRC, building a new Account is expensive.

        Account prev;
        for(auto i = 0; i < 20; ++i)
        {
            Account acct {"A" + std::to_string(i)};
            env.fund(XRP(10000), acct);
            env.close();
            if(i > 0)
            {
                env.trust(acct["USD"](1000), prev);
                env(pay(acct, prev, acct["USD"](5)));
            }
            env(offer(acct, XRP(100), acct["USD"](1)));
            env.close();
            prev = std::move(acct);
        }

This comment has been minimized.

Copy link
@mellery451

mellery451 Jan 10, 2017

Author Contributor

sure - will use the move

}

jvLedger_ = env.rpc ("ledger", "current", "full") [jss::result];
BEAST_EXPECT(jvLedger_[jss::ledger][jss::accountState].size() > 0);

This comment has been minimized.

Copy link
@ximinez

ximinez Jan 10, 2017

Contributor

Any reason not to compare == 101 here?

This comment has been minimized.

Copy link
@mellery451

mellery451 Jan 10, 2017

Author Contributor

had that originally and opted for a simpler check, but I'll go back to stricter checking.

This comment has been minimized.

Copy link
@ximinez

ximinez Jan 10, 2017

Contributor

Thanks. I like strict checks as long as the thing is deterministic and doesn't trivially change.

jvHashes_ = it[sfHashes.fieldName];
}
}
BEAST_EXPECT(jvHashes_.size() > 0);

This comment has been minimized.

Copy link
@ximinez

ximinez Jan 10, 2017

Contributor

Any reason not to compare == 41 here?

}

void
testSaveLoad ()

This comment has been minimized.

Copy link
@ximinez

ximinez Jan 10, 2017

Contributor

The ledger has already been saved, so I think testLoad is a better name.

This comment has been minimized.

Copy link
@mellery451

mellery451 Jan 10, 2017

Author Contributor

will change

resize_file(
ledgerFileCorrupt,
file_size(ledgerFileCorrupt, ec) - 10,
ec);

This comment has been minimized.

Copy link
@ximinez

ximinez Jan 10, 2017

Contributor

Two functions that can overwrite ec here. Probably better to store file_size on the stack, or use and check two different error_codes.

This comment has been minimized.

Copy link
@mellery451

mellery451 Jan 10, 2017

Author Contributor

will fix

using namespace test::jtx;

// create a new env with the ledger "latest" specified for startup
Env env(*this, ledgerConfig("43", Config::LOAD));

This comment has been minimized.

Copy link
@ximinez

ximinez Jan 10, 2017

Contributor

Comment does not match code. Maybe can get rid of comment. Or Start up the env at ledger 43

BEAST_EXPECT(jrb[jss::ledger][jss::accountState].size() > 0);
BEAST_EXPECT(
jrb[jss::ledger][jss::accountState].size() <=
jvLedger_[jss::ledger][jss::accountState].size());

This comment has been minimized.

Copy link
@ximinez

ximinez Jan 10, 2017

Contributor

Any reason to not compare jrb...size() == 97 here?

setupLedger();
BOOST_SCOPE_EXIT( (&dbPath_) ) {
boost::system::error_code ec;
boost::filesystem::remove_all(dbPath_, ec);

This comment has been minimized.

Copy link
@ximinez

ximinez Jan 10, 2017

Contributor

expect(!ec, ec.message());

This comment has been minimized.

Copy link
@vinniefalco

vinniefalco Jan 10, 2017

Contributor
if(! BEAST_EXPECTS(! ec, ec.message()))
    return;
@ximinez
Copy link
Contributor

left a comment

Updates look good. BOOST_SCOPE_EXIT( this_ ) threw me off for a bit, but the boost docs explain that this_ is a special symbol, and I haven't really dug beyond that. 👍

@mellery451 mellery451 closed this Jan 17, 2017

@mellery451 mellery451 deleted the mellery451:ledger_load branch Jan 17, 2017

@mellery451 mellery451 restored the mellery451:ledger_load branch Jan 17, 2017

@mellery451

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2017

merged as be0fb67

boost::system::error_code ec;
boost::filesystem::remove_all(this_->dbPath_, ec);
this_->expect(!ec, ec.message(), __FILE__, __LINE__);
} BOOST_SCOPE_EXIT_END

This comment has been minimized.

Copy link
@HowardHinnant

HowardHinnant Jan 17, 2017

Contributor

Here's a possible replacement with the same functionality but no macros:

        auto on_exit = [](auto this_)
        {
            boost::system::error_code ec;
            boost::filesystem::remove_all(this_->dbPath_, ec);
            this_->expect(!ec, ec.message(), __FILE__, __LINE__);
        };
        std::unique_ptr<LedgerLoad_test, decltype(on_exit)> hold_scope{this, on_exit};

Tested, seems to work fine.

This comment has been minimized.

Copy link
@mellery451

mellery451 Jan 17, 2017

Author Contributor

...and we can change this_ to this in the capture, right ?

This comment has been minimized.

Copy link
@HowardHinnant

HowardHinnant Jan 17, 2017

Contributor

I don't think so. The deleter has to take a single LedgerLoad_test* as a parameter. Since we send this into the unique_ptr, then this gets passed to the deleter on exit.

This comment has been minimized.

Copy link
@mellery451

mellery451 Jan 17, 2017

Author Contributor

got it - thanks.

This comment has been minimized.

Copy link
@HowardHinnant

HowardHinnant Jan 17, 2017

Contributor

It's a little frustrating that make_unique is not available to help with this. We could write a little make_scope factory function around this idiom to play the role of make_unique. That's probably not worth it just for this one instance. But if we find ourselves doing this more places in the future, then it would be worth it.

This comment has been minimized.

Copy link
@HowardHinnant

HowardHinnant Jan 17, 2017

Contributor

C++17 auto deduction will then get rid of the need for make_scope. :-)

std::unique_ptr hold_scope{this, on_exit};

@mellery451 mellery451 deleted the mellery451:ledger_load branch Jul 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.