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

Further refactoring on doBlockchainTest #4382

Merged
merged 11 commits into from Aug 24, 2017

Conversation

Projects
None yet
4 participants
@pirapira
Member

pirapira commented Aug 23, 2017

This PR further separates input and output in blockchaintest filling. The code became a bit shorter.

@pirapira pirapira requested review from winsvega and chfast Aug 23, 2017

json_spirit::mObject jObj = o;
if (o.count("expect"))
json_spirit::mObject jObjOutput = inputTest;
if (inputTest.count("expect"))
{
//prepare the corresponding expect section for the test

This comment has been minimized.

@winsvega

winsvega Aug 23, 2017

Contributor

need to add a comment here that from one test with general expect section we write 'forknumber' tests with individual expect section to the output test which will again be used as a filler and overwriten.

@winsvega

winsvega Aug 23, 2017

Contributor

need to add a comment here that from one test with general expect section we write 'forknumber' tests with individual expect section to the output test which will again be used as a filler and overwriten.

This comment has been minimized.

@pirapira

pirapira Aug 23, 2017

Member

What is a "forknumber" test?

@pirapira

pirapira Aug 23, 2017

Member

What is a "forknumber" test?

This comment has been minimized.

@winsvega

winsvega Aug 23, 2017

Contributor

from one test in a filler we produce a test filler for Frontier, Homestead, and so on. total number 'forknumber' and then run a test filler function on those to get final tests

@winsvega

winsvega Aug 23, 2017

Contributor

from one test in a filler we produce a test filler for Frontier, Homestead, and so on. total number 'forknumber' and then run a test filler function on those to get final tests

This comment has been minimized.

@pirapira

pirapira Aug 23, 2017

Member

I see, when there are seven forks, "forknumber" is equial to seven, so "forknumber" tests mean 7 tests.

@pirapira

pirapira Aug 23, 2017

Member

I see, when there are seven forks, "forknumber" is equial to seven, so "forknumber" tests mean 7 tests.

@winsvega

so to cover this with the test coverage. we might want a test that execute a filler procedure on example test. with expect section set. and check that final test contains forknumber tests and those tests are failing when filling if expectations from expect section not meet.

the expect section in the test should contain array.
"network" : ["Frontier", "Homestead"]

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Aug 23, 2017

Member

@winsvega do you want it as a unit test or a testeth command invocation?

Member

pirapira commented Aug 23, 2017

@winsvega do you want it as a unit test or a testeth command invocation?

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega Aug 23, 2017

Contributor

a unit test would be nice. we could run doTest function for a particular test file.
however need to disable mining use sealEngine NoProof so it to run faster

Contributor

winsvega commented Aug 23, 2017

a unit test would be nice. we could run doTest function for a particular test file.
however need to disable mining use sealEngine NoProof so it to run faster

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Aug 23, 2017

Member

OK, will do.

Member

pirapira commented Aug 23, 2017

OK, will do.

@pirapira pirapira changed the title from Further refactoring on doBlockchainTest to [WIP] Further refactoring on doBlockchainTest Aug 23, 2017

@pirapira pirapira removed the request for review from chfast Aug 23, 2017

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 23, 2017

Codecov Report

Merging #4382 into develop will increase coverage by 0.44%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4382      +/-   ##
===========================================
+ Coverage    67.73%   68.18%   +0.44%     
===========================================
  Files          303      304       +1     
  Lines        23205    23210       +5     
===========================================
+ Hits         15718    15825     +107     
+ Misses        7487     7385     -102
Impacted Files Coverage Δ
test/tools/jsontests/BlockChainTests.cpp 44.93% <100%> (+14.26%) ⬆️
test/unittests/libtesteth/blockchainTest.cpp 100% <100%> (ø)
test/tools/libtesteth/ImportTest.cpp 58.71% <0%> (+6.15%) ⬆️

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 9bd9318...d010cfc. Read the comment docs.

codecov-io commented Aug 23, 2017

Codecov Report

Merging #4382 into develop will increase coverage by 0.44%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4382      +/-   ##
===========================================
+ Coverage    67.73%   68.18%   +0.44%     
===========================================
  Files          303      304       +1     
  Lines        23205    23210       +5     
===========================================
+ Hits         15718    15825     +107     
+ Misses        7487     7385     -102
Impacted Files Coverage Δ
test/tools/jsontests/BlockChainTests.cpp 44.93% <100%> (+14.26%) ⬆️
test/unittests/libtesteth/blockchainTest.cpp 100% <100%> (ø)
test/tools/libtesteth/ImportTest.cpp 58.71% <0%> (+6.15%) ⬆️

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 9bd9318...d010cfc. Read the comment docs.

pirapira added some commits Aug 18, 2017

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Aug 24, 2017

Member

@winsvega I added such unit tests.

Member

pirapira commented Aug 24, 2017

@winsvega I added such unit tests.

@pirapira pirapira changed the title from [WIP] Further refactoring on doBlockchainTest to Further refactoring on doBlockchainTest Aug 24, 2017

@pirapira pirapira requested a review from chfast Aug 24, 2017

BOOST_CHECK_MESSAGE(output.get_obj().size() == getNetworks().size(), "A wrong number of tests were generated.");
}
BOOST_AUTO_TEST_CASE_EXPECTED_FAILURES(fillingWithWrongExpectation, 2)

This comment has been minimized.

@winsvega

winsvega Aug 24, 2017

Contributor

what about BOOST_CHEK_THROW ?

@winsvega

winsvega Aug 24, 2017

Contributor

what about BOOST_CHEK_THROW ?

This comment has been minimized.

@pirapira

pirapira Aug 24, 2017

Member

It didn't work. doBlockchainTest() already registered failures even inside BOOST_CHECK_THROW.

@pirapira

pirapira Aug 24, 2017

Member

It didn't work. doBlockchainTest() already registered failures even inside BOOST_CHECK_THROW.

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega Aug 24, 2017

Contributor

very nice!!

Contributor

winsvega commented Aug 24, 2017

very nice!!

{
std::string const s = R"(
{
"BLOCKHASH_Bounds" : {

This comment has been minimized.

@winsvega

winsvega Aug 24, 2017

Contributor

I think we could define this template once and then use a replace function from fuzzTest.cpp
you could define your own map for replacement. even something like
[EXPECT] -> "{ "network" : "Frontier", ... }"

@winsvega

winsvega Aug 24, 2017

Contributor

I think we could define this template once and then use a replace function from fuzzTest.cpp
you could define your own map for replacement. even something like
[EXPECT] -> "{ "network" : "Frontier", ... }"

This comment has been minimized.

@pirapira

pirapira Aug 24, 2017

Member

I don't like these templates. I'll visit there some day.

@pirapira

pirapira Aug 24, 2017

Member

I don't like these templates. I'll visit there some day.

@pirapira pirapira merged commit bde8ba6 into develop Aug 24, 2017

6 checks passed

codecov/patch 100% of diff hit (target 50%)
Details
codecov/project 68.18% (+0.44%) compared to 9bd9318
Details
codecov/project/app 63.03% remains the same compared to 9bd9318
Details
codecov/project/tests 77.57% (+1.25%) compared to 9bd9318
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pirapira pirapira deleted the doBlockchainTest branch Aug 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment