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

split BlockchainTests into Valid and Invalid #5648

Merged
merged 4 commits into from
Jul 11, 2019
Merged

Conversation

winsvega
Copy link
Contributor

@winsvega winsvega commented Jul 3, 2019

split BlockchainTests into BlockchainTests/ValidBlocks and BlockchainTests/InvalidBlocks
move TransitionTests under BlockchainTests

ValidBlocks contain only tests with valid rlps
InvalidBlocks contain tests where some block rlp's are invalid and expect and exception upon import

Update test subfolder to this PR ethereum/tests#605

@winsvega winsvega requested a review from gumb0 July 3, 2019 19:40
@gumb0
Copy link
Member

gumb0 commented Jul 4, 2019

We should first fix failing tests where we disagree with geth, I try to work on that in #5652

@@ -1024,6 +1054,20 @@ class bcTestFixture {
}
};

class bcInvalidTestFixture
Copy link
Member

Choose a reason for hiding this comment

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

This class is almost identical to the one above, it looks like you could have a base class common for both to avoid code repetition. Just a suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a plan for malicious block blockchain testsing, when the test tool will produce invlid block rlps and try to import.
Thats for the future. for now just separate tests with invalid blocks that we have.

@codecov-io
Copy link

Codecov Report

Merging #5648 into master will increase coverage by 0.05%.
The diff coverage is 92.4%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5648      +/-   ##
==========================================
+ Coverage      63%   63.05%   +0.05%     
==========================================
  Files         350      350              
  Lines       29915    29944      +29     
  Branches     3352     3353       +1     
==========================================
+ Hits        18847    18882      +35     
+ Misses       9850     9847       -3     
+ Partials     1218     1215       -3

@@ -60,6 +60,7 @@ class TestOutputHelper
TestOutputHelper() {}
void checkUnfinishedTestFolders(); // Checkup that all test folders are active during the test
// run
std::string detectFilterForMinusTArgument();
Copy link
Member

Choose a reason for hiding this comment

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

this is not needed anymore?

@@ -95,95 +95,95 @@ BOOST_AUTO_TEST_CASE(basicGasPricer_RPC_API_Test_Frontier)
{
u256 _expectedAsk = 16056883295;
u256 _expectedBid = 1;
dev::test::executeGasPricerTest("RPC_API_Test_Frontier", 30.679, 15.0, "/BlockchainTests/bcGasPricerTest/RPC_API_Test.json", TransactionPriority::Medium, _expectedAsk, _expectedBid, eth::Network::FrontierTest);
dev::test::executeGasPricerTest("RPC_API_Test_Frontier", 30.679, 15.0, "/BlockchainTests/ValidBlocks/bcGasPricerTest/RPC_API_Test.json", TransactionPriority::Medium, _expectedAsk, _expectedBid, eth::Network::FrontierTest);
Copy link
Member

Choose a reason for hiding this comment

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

Would be better to get rid of repetition here, moving "/BlockchainTests/ValidBlocks/" to a constant

Copy link
Member

Choose a reason for hiding this comment

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

or by moving it inside executeGasPricerTest function

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 ok in general

@winsvega winsvega merged commit 089e7dd into master Jul 11, 2019
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