Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

testeth legacy blockchain tests suite #5801

Merged
merged 4 commits into from
Nov 6, 2019
Merged

testeth legacy blockchain tests suite #5801

merged 4 commits into from
Nov 6, 2019

Conversation

winsvega
Copy link
Contributor

@winsvega winsvega commented Oct 28, 2019

test repo update is in progress

@winsvega winsvega requested a review from gumb0 October 28, 2019 17:00
@gumb0 gumb0 requested review from chfast and halfalicious October 28, 2019 17:01
@codecov-io
Copy link

codecov-io commented Oct 28, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@0a311f1). Click here to learn what that means.
The diff coverage is 94.64%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5801   +/-   ##
=========================================
  Coverage          ?   64.07%           
=========================================
  Files             ?      359           
  Lines             ?    30810           
  Branches          ?     3417           
=========================================
  Hits              ?    19742           
  Misses            ?     9845           
  Partials          ?     1223

@@ -44,6 +44,52 @@ class bcGeneralTestsFixture : public StateTestFixtureBase<BCGeneralStateTestsSui
bcGeneralTestsFixture() : StateTestFixtureBase({TestExecution::RequireOptionAll}) {}
};

template <class T>
Copy link
Member

@gumb0 gumb0 Oct 31, 2019

Choose a reason for hiding this comment

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

I think you can easy get rid of duplication here like this

template <class T>
class BlockChainTestFixture
{
public:
    BlockChainTestFixture(std::set<TestExecution> const& _execFlags = {}, std::vector<std::string> const& _timeConsumingCases)
    {
        T suite;
        if (_execFlags.count(TestExecution::NotRefillable) &&
            (Options::get().fillchain || Options::get().filltests))
            BOOST_FAIL("Tests are sealed and not refillable!");

        string const casename = boost::unit_test::framework::current_test_case().p_name;
        boost::filesystem::path const suiteFillerPath = suite.getFullPathFiller(casename).parent_path();

        // skip time consuming tests, unless --all option is given
        if (!test::Options::get().all && contains(_timeConsumingCases, casename))
        {
            cnote << "Skipping " << casename << " because --all option is not specified.\n";
            test::TestOutputHelper::get().markTestFolderAsFinished(suiteFillerPath, casename);
            return;
        }

        suite.runAllTestsInFolder(casename);
        test::TestOutputHelper::get().markTestFolderAsFinished(suiteFillerPath, casename);
    }
};
class BlockChainValidBlocksTestFixture
  : public BlockChainTestFixture<BlockchainValidTestSuite>
{
public:
    BlockChainValidBlocksTestFixture()
      : BlockChainValidBlocksTestFixture({TestExecution::NotRefillable}, {"bcWalletTest"})
    {}
};

Copy link
Member

@gumb0 gumb0 Oct 31, 2019

Choose a reason for hiding this comment

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

Actually, if stWallet exists in only in either valid or invalid suite, you can use the exact same code with stWallet check for both suites (it won't skip stWallet if it doesn't exist there)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is ok to use just one fixture class in this case

@gumb0
Copy link
Member

gumb0 commented Oct 31, 2019

Does this change anything regarding how to run the tests? (command line)

@winsvega
Copy link
Contributor Author

winsvega commented Nov 3, 2019

Does this change anything regarding how to run the tests? (command line)

to run the old blockchain tests now use command:
./testeth -t LegacyTests/Constantinople/BlockchainTests

@winsvega winsvega requested a review from gumb0 November 4, 2019 15:47
Copy link
Member

@gumb0 gumb0 left a comment

Choose a reason for hiding this comment

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

Looks good, need to add changelog item

"testeth -t BlockchainTests command now doesn't run the tests for the forks before Istanbul. To run those tests use a separate LegacyTests suite with command testeth -t LegacyTests/Constantinople/BlockchainTests"

check the commands^

test/tools/jsontests/BlockChainTests.h Outdated Show resolved Hide resolved
@winsvega
Copy link
Contributor Author

winsvega commented Nov 4, 2019

updated the change log.

@winsvega winsvega requested a review from gumb0 November 5, 2019 09:13
Copy link
Member

@gumb0 gumb0 left a comment

Choose a reason for hiding this comment

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

Don't merge, please rebase on master instead

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@winsvega
Copy link
Contributor Author

winsvega commented Nov 5, 2019

the errors seems to be in libp2p test again. merge?

@gumb0 gumb0 merged commit 39faebc into master Nov 6, 2019
@gumb0 gumb0 deleted the legacybc branch November 6, 2019 10:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants