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

testeth legacy blockchain tests suite#5801

Merged
gumb0 merged 4 commits intomasterfrom
legacybc
Nov 6, 2019
Merged

testeth legacy blockchain tests suite#5801
gumb0 merged 4 commits intomasterfrom
legacybc

Conversation

@winsvega
Copy link
Copy Markdown

@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
Copy Markdown

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

bcGeneralTestsFixture() : StateTestFixtureBase({TestExecution::RequireOptionAll}) {}
};

template <class T>
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

gumb0 commented Oct 31, 2019

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

@winsvega
Copy link
Copy Markdown
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
Copy Markdown
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^

@winsvega
Copy link
Copy Markdown
Author

winsvega commented Nov 4, 2019

updated the change log.

@winsvega winsvega requested a review from gumb0 November 5, 2019 09:13
Copy link
Copy Markdown
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

@winsvega
Copy link
Copy Markdown
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