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

--fillchain --filltests should not write the (broken) GeneralStateTest #4468

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
5 participants
@pirapira
Member

pirapira commented Sep 6, 2017

Before this PR, running testeth -t StateTestsGeneral -- --filltests --fillchain modified the GeneralStateTest in addition to the desired file in BlockchainTests/GeneralStateTest. Moreover, the written GeneralStateTest was corrupt. This PR stops testeth -- --filltests --fillchain from writing to the GeneralStateTest file.

Fixes #4258

@chriseth chriseth added the in progress label Sep 6, 2017

@pirapira pirapira requested a review from winsvega Sep 6, 2017

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega Sep 6, 2017

Contributor

I think it should actually not write filled statefiles at all if --fillchain is set.
we had a better plan to split statetests --filltests --fillchain into separate commands.

.1) statetests --fillchain
that would generate blockchai test fillers out of state test fillers in a tmp src folder
2) blockchaintests --filltests
that would generate blockchain tests out of tmp src folder from the previous command.

the tmp src folder should be in src/Blockchaintests/generalstatetests

Contributor

winsvega commented Sep 6, 2017

I think it should actually not write filled statefiles at all if --fillchain is set.
we had a better plan to split statetests --filltests --fillchain into separate commands.

.1) statetests --fillchain
that would generate blockchai test fillers out of state test fillers in a tmp src folder
2) blockchaintests --filltests
that would generate blockchain tests out of tmp src folder from the previous command.

the tmp src folder should be in src/Blockchaintests/generalstatetests

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Sep 6, 2017

Member

it should actually not write filled statefiles at all if --fillchain is set.

That's implemented:
https://github.com/ethereum/cpp-ethereum/pull/4468/files#diff-60b4496c08307cc76ac346ca15cf03ecR146

Member

pirapira commented Sep 6, 2017

it should actually not write filled statefiles at all if --fillchain is set.

That's implemented:
https://github.com/ethereum/cpp-ethereum/pull/4468/files#diff-60b4496c08307cc76ac346ca15cf03ecR146

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Sep 6, 2017

Member

@winsvega please create an issue about the two commands plan. I like it.

Member

pirapira commented Sep 6, 2017

@winsvega please create an issue about the two commands plan. I like it.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 18, 2017

Codecov Report

Merging #4468 into develop will decrease coverage by 0.01%.
The diff coverage is 88.88%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4468      +/-   ##
===========================================
- Coverage       63%   62.98%   -0.02%     
===========================================
  Files          368      368              
  Lines        30517    30523       +6     
  Branches      2755     2756       +1     
===========================================
  Hits         19226    19226              
- Misses       11108    11118      +10     
+ Partials       183      179       -4
Impacted Files Coverage Δ
test/tools/libtesteth/TestSuite.h 100% <ø> (ø) ⬆️
test/tools/libtesteth/TestHelper.h 50% <ø> (ø) ⬆️
test/tools/jsontests/StateTests.cpp 94.73% <100%> (+0.05%) ⬆️
test/tools/jsontests/TransactionTests.cpp 67.28% <100%> (+0.3%) ⬆️
test/tools/jsontests/vm.cpp 62.82% <100%> (+0.12%) ⬆️
test/tools/jsontests/BlockChainTests.cpp 72.84% <80%> (+0.08%) ⬆️
test/tools/libtesteth/TestSuite.cpp 71.42% <85.71%> (+0.29%) ⬆️
test/unittests/libethereum/BlockQueue.cpp 46.12% <0%> (-4.06%) ⬇️
libethereum/BlockQueue.h 70.37% <0%> (-1.24%) ⬇️
... and 3 more

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 4219e17...38bf1a0. Read the comment docs.

codecov-io commented Sep 18, 2017

Codecov Report

Merging #4468 into develop will decrease coverage by 0.01%.
The diff coverage is 88.88%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4468      +/-   ##
===========================================
- Coverage       63%   62.98%   -0.02%     
===========================================
  Files          368      368              
  Lines        30517    30523       +6     
  Branches      2755     2756       +1     
===========================================
  Hits         19226    19226              
- Misses       11108    11118      +10     
+ Partials       183      179       -4
Impacted Files Coverage Δ
test/tools/libtesteth/TestSuite.h 100% <ø> (ø) ⬆️
test/tools/libtesteth/TestHelper.h 50% <ø> (ø) ⬆️
test/tools/jsontests/StateTests.cpp 94.73% <100%> (+0.05%) ⬆️
test/tools/jsontests/TransactionTests.cpp 67.28% <100%> (+0.3%) ⬆️
test/tools/jsontests/vm.cpp 62.82% <100%> (+0.12%) ⬆️
test/tools/jsontests/BlockChainTests.cpp 72.84% <80%> (+0.08%) ⬆️
test/tools/libtesteth/TestSuite.cpp 71.42% <85.71%> (+0.29%) ⬆️
test/unittests/libethereum/BlockQueue.cpp 46.12% <0%> (-4.06%) ⬇️
libethereum/BlockQueue.h 70.37% <0%> (-1.24%) ⬇️
... and 3 more

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 4219e17...38bf1a0. Read the comment docs.

pirapira added some commits Sep 6, 2017

Skip writing the filled GeneralStateTest with --fillchain
Before this commit, testeth -- --filltests --fillchain wrote a broken GeneralStateTest file that should be discarded

Fixes #4258
@winsvega

is this all only because of --filltests --fillchain working together?

In my other PR I am making --filltests --fillchain to create both the blockchain and state test. no need to implements AccessSwitch

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Sep 26, 2017

Member

Which PR is it?

Member

pirapira commented Sep 26, 2017

Which PR is it?

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega
Contributor

winsvega commented Sep 26, 2017

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Sep 26, 2017

Member

I didn't understand what #4527 does. I still don't know why this changes --filltests --fillchain behavior. I need to try and read the code.

Member

pirapira commented Sep 26, 2017

I didn't understand what #4527 does. I still don't know why this changes --filltests --fillchain behavior. I need to try and read the code.

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega Sep 26, 2017

Contributor

ok. so should we close this PR ?

Contributor

winsvega commented Sep 26, 2017

ok. so should we close this PR ?

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Sep 26, 2017

Member

Yes.

Member

pirapira commented Sep 26, 2017

Yes.

@pirapira pirapira closed this Sep 26, 2017

@pirapira pirapira deleted the fillchain-dont-modify-statetest branch Sep 26, 2017

@chfast chfast removed the in progress label Dec 19, 2017

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