Reintegrate blockchain tests #193
Conversation
holgerd77
force-pushed
the
reintegrate-blockchain-tests
branch
4 times, most recently
from
39fe9c1 to
872d935
Compare
There was a problem hiding this comment.
I think this looks mostly good. One suggestion that I do have is that we consider renaming --dir to --testcase.
It would also be useful to be able to pass more than one value to --excludeDir and --dir.
So in summary, I would rename --dir to --testcases and --excludeDir to --exclude. It would probably be best to do this on a separate PR.
| @@ -45,6 +45,9 @@ | |||
| "testVM": "node ./tests/tester -v", | |||
| "testState": "node ./tests/tester -s", | |||
| "testBlockchain": "node --stack-size=1500 ./tests/tester -b", | |||
| "testBlockchainBlockGasLimit": "node --stack-size=1500 ./tests/tester -b --dir='bcBlockGasLimitTest'", | |||
| "testBlockchainValid": "node --stack-size=1500 ./tests/tester -b --dir='bcValidBlockTest'", | |||
| "testBlockchainTotalDifficulty": "node --stack-size=1500 ./tests/tester -b --dir='bcTotalDifficultyTest'", | |||
There was a problem hiding this comment.
Ideally, these could be one line:
"testBlockchainTravis": "node --stack-size=1500 ./tests/tester -b --dir='bcBlockGasLimitTest testBlockchainValidtest BlockchainTotalDifficulty'
There was a problem hiding this comment.
-
--dirreally is a directory option - directly passed through toethereumjs-testing, where you can select e.g. all theGeneralStateTestsinBlockChainTestswith--dir='GeneralStateTests'or a subfolder (test cases) with--dir='GeneralStateTests/stCallCodes'. For just renaming--excludeDirthis doesn't seems worth the effort, since this would have to be routed through the already merged PR onethereumjs-testing. -
Since including several directories isn't supported in the current version of
ethereumjs-testingthis would need a wrapper in theethereumjs-vmtests implementation and I think this is more confusing than helpful for people reading the code and expecting the same API as inethereumjs-testing. So I also would say this isn't worth the effort here, this doesn't have to scale or something, it's just a practical solution to get three test folders runnning.
There was a problem hiding this comment.
I actually thought longer than it might seem on how to implement the --dir option (e.g. not using the filters from npm-dir). The selected solution has some flexibility in some directions (e.g. the subfolder selection from above wouldn't be possible with the current filter implementation in npm-dir), and it is generally easier to use (with npm-dir filter I actually experimented over half an hour and didn't get the results I expected).
So the --dir implementation lacks a filter functionality but I think the use case for this for directories is relatively limited, though one can probably come up with use cases (all directories with memory in it, hmm :-)). If this should be needed in the future an option --dirFilter could additionally be added.
It is also possible to expand the current --dir implementation with the possibility to add several directories in the future and not loosing backwards compatibility. I wouldn't want to do this now though.
To sum up: for our current use cases I decided that this is the better choice. :-)
There was a problem hiding this comment.
The change to make --dir accept several directories can be made in a future PR. It would be nice to be able to condense the test suites into one.
holgerd77
force-pushed
the
reintegrate-blockchain-tests
branch
from
872d935 to
b7549db
Compare
holgerd77
force-pushed
the
reintegrate-blockchain-tests
branch
2 times, most recently
from
f3e5cf7 to
df28ff6
Compare
…ed BlockchainTests for travis, circle
holgerd77
force-pushed
the
reintegrate-blockchain-tests
branch
from
df28ff6 to
0d54e95
Compare
|
I've rebased this and tests are now passing (see comments on Gitter), this can now be merged. |
This reintegrates the blockchain tests to be run on travis builds (leaving
BlockchainTests/GeneralStateTests).PR should be merged also on travis build failure since blockchain tests are failing for other reasons atm and this is just a basis for testing/fixing this.