Use the state tests embedded in the blockchain tests #1577

lithp · 2018-12-11T23:05:11Z

When state tests fail they tell you the root state hash has changed, but there's no easy way of telling which parts of the state have changed. However, a copy of the state tests is also available under the blockchain tests, and that copy includes the full expected state tree.

This PR changes which version of the state tests are run. In combination with #1573 this should make diagnosing test failures much easier.

test_state.py skipped some tests which take too long to run, those changes are now reproduced in test_blockchain.py
test_blockchain.py now also supports the --fork flag, to allow running just a subset of the tests
the tox commands have been updated to only run blockchain tests, not the state tests.
circleci has been updated in-step with tox
SLOWEST_TESTS has been regenerated. There are now more tests (each of the indexes appears as a separate blockchain test) so the list is a little longer now. The 60ish tests skipped here (out of 10851) account for 15 of the 21 minutes it takes to run the test suite on my machine.

Cute Animal Picture

cburgdorf · 2018-12-12T12:38:03Z

tests/json-fixtures/test_blockchain.py

@@ -34,6 +36,84 @@
 BASE_FIXTURE_PATH = os.path.join(ROOT_PROJECT_DIR, 'fixtures', 'BlockchainTests')


+# These are the slowest tests from the full blockchain test run. This list
+# should be regenerated occasionally using `--durations 100`.
+SLOWEST_TESTS = {


Not for this PR but just as an idea: Isn't there something in Circle CI that we can differentiate between CI runs for PRs and CI runs on master? I think it would be nice if we could at least achieve that all these tests run from time to time. So that, even if we don't get immediate feedback that our PR broke one of these, we at least get notified about it some time after.

It's possible with Travis and I would be surprised if it wasn't also possible with Circle CI. It could be nice to have a nightly build which takes master and runs all the tests, and maybe even does benchmarking?

I dug around a bit and couldn't find anything in the CircleCI interface to do this

CircleCI exposes some useful environment variables such as CIRCLE_PULL_REQUEST and CIRCLE_BRANCH you could use to detect whether you're building a branch or not.

I've opened a PR which attempts to schedule a nightly build which runs all the tests, even the slowest ones.

cburgdorf

This looks good to me. I would feel a bit better if I knew the reason why the GeneralStateTests exist twice 😅

cburgdorf · 2018-12-12T13:05:34Z

tox.ini

@@ -1,10 +1,10 @@
 [tox]
 envlist=
-    py{35,36}-{core,database,transactions,vm,native-blockchain}


I'm still a bit confused why the GeneralStateTests also exists outside of the BlockchainTest and if there is something we might be forgetting about dropping those. Do you know why they exist in the first place? Did you check if the number of tests that are run still matches after this change?

Upstream docs:

https://ethereum-tests.readthedocs.io/en/latest/test_types/state_tests.html
https://ethereum-tests.readthedocs.io/en/latest/test_types/blockchain_tests.html

So, you should be confused as to why GeneralStateTests exist inside BlockchainTests, not the other way around. ^_^

Anyway, according to the docs, all of the former should eventually make their way into the latter:

https://ethereum-tests.readthedocs.io/en/latest/generating-tests.html#advanced-converting-a-generalstatetest-case-into-a-blockchaintest-case

I wish I had taken better notes, I saw somewhere that winsvega wanted to remove the state tests entirely, given that they've been duplicated to the blockchain tests. I neglected to save the link unfortunately.

And that last link is a little scary. It's possible that there's been an error in test generation and that some of the state tests are missing from BlockchainTests/GeneralStateTests, but the idea is definitely that they be perfect duplicates.

carver · 2018-12-12T18:03:31Z

I just did a quick scan, and I'm missing something: what happened to the blockchain tests that are not in the state tests. Are they distributed in the different forks now?

veox · 2018-12-12T18:12:14Z

@carver Good question. You mean like this one?..

pytest -k 'test_blockchain_fixtures[/home/veox/src/py-evm/fixtures/BlockchainTests/bcUncleTest/uncleWithSameBlockNumber.json:uncleWithSameBlockNumber_Homestead]'

They are in the same submodule that gets pulled in as fixtures.

(I haven't tested this particular PR yet, and whether it has inadvertently removed something.)

carver

Just marking this 👎 so no one merges by accident while we confirm that none of the blockchain tests got dropped.

carver · 2018-12-12T18:00:47Z

.circleci/config.yml

-      - py36-native-state-frontier
-      - py36-native-state-homestead
-      - py36-native-state-eip150
-      - py36-native-state-eip158


These are still state tests, they just happen to be in a different folder. I think the job name could stay the same. But it's a minor opinion.

They're blockchain tests now! Blockchain tests which happen to also contain state tests. The command for this specific one is now:

native-blockchain-eip158: pytest {posargs:tests/json-fixtures/test_blockchain.py --fork EIP158}

Somewhat orthogonal but it would be good to rename the EIPXXX to their long form fork names.

What are their long form fork names? The EIP's page just calls it 158 and doesn't say when it was scheduled.

150 is apparently Tangerine Whistle and yeah, that name is 200% more exciting than EIP-150 😀

Do you want me to do that in this PR?

I think "Spurious Dragon" is the other one. Look in eth.vm.forks and you can find them (and there should be things within each module to hint at which EIPXXX they correspond to.

carver · 2018-12-12T18:14:31Z

tox.ini

@@ -35,14 +35,13 @@ commands=
    rpc-state-quadratic: pytest {posargs:tests/trinity/json-fixtures-over-rpc/test_rpc_fixtures.py -k 'GeneralStateTests and stQuadraticComplexityTest'}
    transactions: pytest {posargs:tests/json-fixtures/test_transactions.py}
    vm: pytest {posargs:tests/json-fixtures/test_virtual_machine.py}
-    native-blockchain: pytest {posargs:tests/json-fixtures/test_blockchain.py}


Good question. You mean like this one?..

pytest -k 'test_blockchain_fixtures[/home/veox/src/py-evm/fixtures/BlockchainTests/bcUncleTest/uncleWithSameBlockNumber.json:uncleWithSameBlockNumber_Homestead]'

They are in the same submodule that gets pulled in as fixtures.

(I haven't tested this particular PR yet, and whether it has inadvertently removed something.)

@veox right, the complete removal of this test (non-state blockchain tests) is what threw me off. Just wanted to make sure we had confirmation that nothing got dropped in the refactor.

A weak indicator is bcForkStressTest/ForkStressTest.json making it into the list of slowest tests:

https://github.com/ethereum/py-evm/pull/1577/files#diff-c5ffb862ee7554c09f7aea9d6f6a679aR75

(Got to run right now, so can't take a proper look.)

SLOWEST_TESTS isn't a great indication because I generated those myself and didn't use tox/circleci. But here's proof that regular blockchain tests are being run:

(py-evm-ojdIf4Yr) brian@rhythm:~/ef/py-evm$ pytest tests/json-fixtures/test_blockchain.py --fork Byzantium --collect-only | grep -v GeneralStateTests | head ============================= test session starts ============================== platform linux -- Python 3.6.6, pytest-3.6.4, py-1.7.0, pluggy-0.7.1 rootdir: /home/brian/ef/py-evm, inifile: pytest.ini plugins: xdist-1.18.1, profiling-1.3.0, cov-2.5.1, asyncio-0.9.0, asyncio-network-simulator-0.1.0a2, hypothesis-3.69.5 collected 4945 items <Module 'tests/json-fixtures/test_blockchain.py'> <Function 'test_blockchain_fixtures[/home/brian/ef/py-evm/fixtures/BlockchainTests/bcBlockGasLimitTest/BlockGasLimit2p63m1.json:BlockGasLimit2p63m1_Byzantium:Byzantium]'> <Function 'test_blockchain_fixtures[/home/brian/ef/py-evm/fixtures/BlockchainTests/bcBlockGasLimitTest/GasUsedHigherThanBlockGasLimitButNotWithRefundsSuicideFirst.json:GasUsedHigherThanBlockGasLimitButNotWithRefundsSuicideFirst_Byzantium:Byzantium]'> <Function 'test_blockchain_fixtures[/home/brian/ef/py-evm/fixtures/BlockchainTests/bcBlockGasLimitTest/GasUsedHigherThanBlockGasLimitButNotWithRefundsSuicideLast.json:GasUsedHigherThanBlockGasLimitButNotWithRefundsSuicideLast_Byzantium:Byzantium]'> <Function 'test_blockchain_fixtures[/home/brian/ef/py-evm/fixtures/BlockchainTests/bcBlockGasLimitTest/SuicideTransaction.json:SuicideTransaction_Byzantium:Byzantium]'>

Each of these native-blockchain-* jobs is now running all the blockchain tests for a given fork, which (primarily) includes state tests, but also includes the blockchain tests which were previously being run by the native-blockchain job.

The only way we would be missing some tests would be if this collection of forks doesn't cover the full set of blockchain tests: frontier, homestead, eip150, eip158, byzantium, constantinople, metropolis. Let me think of a way to test whether that's true.

Well shucks, looks like we're missing some tests:

(py-evm-ojdIf4Yr) brian@rhythm:~/ef/py-evm$ pytest tests/json-fixtures/test_blockchain.py --collect-only 2>/dev/null | grep -oP '\[\K.*(?=\])' | cut -d':' -f3 | sort | uniq Byzantium Constantinople EIP150 EIP158 EIP158ToByzantiumAt5 Frontier FrontierToHomesteadAt5 Homestead HomesteadToDaoAt5 HomesteadToEIP150At5

Okay, thanks for catching this Carver! I've pushed a commit which adds the missing tests.

Haha, well I pretty much just pointed and grumbled. Thanks for hunting it down!

Mostly a copy-paste from `py-evm` codebase; relevant PRs that introduced the functionality there: ethereum/py-evm#1470 ethereum/py-evm#1577

lithp force-pushed the lithp/use-blockchain-state-tests branch 2 times, most recently from 7ee7041 to a8b1a74 Compare December 11, 2018 23:07

carver mentioned this pull request Dec 11, 2018

Failing blockchain tests now emit a state diff #1573

Merged

lithp added 5 commits December 11, 2018 16:57

Add support for the --fork flag, enable BC/GeneralStateTests

98666e9

Update tox, it should only run blockchain tests

06b2c89

Remove test_state.py

fb69f0d

Properly skip the slowest tests

eb43281

Update list of circleci jobs

5bb8b02

lithp force-pushed the lithp/use-blockchain-state-tests branch from 19d47a1 to 5bb8b02 Compare December 12, 2018 01:00

veox mentioned this pull request Dec 12, 2018

Update ethereum/tests fixtures to v6.0.0-beta.2 + fix revealed consensus failures #1579

Merged

Update the list of slowest tests

5c15617

lithp force-pushed the lithp/use-blockchain-state-tests branch from b767a6e to 5c15617 Compare December 12, 2018 02:52

cburgdorf reviewed Dec 12, 2018

View reviewed changes

cburgdorf approved these changes Dec 12, 2018

View reviewed changes

carver requested changes Dec 12, 2018

View reviewed changes

Also run the BlockchainTests/TransitionTests

9d28e83

lithp force-pushed the lithp/use-blockchain-state-tests branch from 86fa931 to 9d28e83 Compare December 12, 2018 19:03

carver approved these changes Dec 12, 2018

View reviewed changes

carver merged commit 10d62b5 into ethereum:master Dec 12, 2018

lithp deleted the lithp/use-blockchain-state-tests branch December 12, 2018 22:43

carver mentioned this pull request Dec 21, 2018

Rename the EIPXXX test jobs to their long form fork names ethereum/trinity#102

Closed

This was referenced Jan 7, 2019

CI: rename EIP15{0,8} tox jobs to their long-form fork names. #1699

Merged

CI: rename EIP15{0,8} tox jobs to their long-form fork names. ethereum/trinity#116

Merged

veox mentioned this pull request May 15, 2019

tests: update to match py-evm: don't skip Constantinople, add Petersburg, use --fork; break out long-running categories ethereum/trinity#608

Merged

8 tasks

veox added a commit to veox/trinity that referenced this pull request May 16, 2019

tests: ignore tests not matching --fork arg given to pytest.

c141e31

Mostly a copy-paste from `py-evm` codebase; relevant PRs that introduced the functionality there: ethereum/py-evm#1470 ethereum/py-evm#1577

veox added a commit to veox/trinity that referenced this pull request May 20, 2019

tests: ignore tests not matching --fork arg given to pytest.

2e5a311

Mostly a copy-paste from `py-evm` codebase; relevant PRs that introduced the functionality there: ethereum/py-evm#1470 ethereum/py-evm#1577

cburgdorf mentioned this pull request May 28, 2020

CI does not run all the RPC tests ethereum/trinity#1755

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the state tests embedded in the blockchain tests #1577

Use the state tests embedded in the blockchain tests #1577

lithp commented Dec 11, 2018 •

edited

Loading

cburgdorf Dec 12, 2018

lithp Dec 12, 2018

pipermerriam Dec 12, 2018

lithp Dec 13, 2018

cburgdorf left a comment

cburgdorf Dec 12, 2018

veox Dec 12, 2018

lithp Dec 12, 2018

carver commented Dec 12, 2018

veox commented Dec 12, 2018

carver left a comment

carver Dec 12, 2018 •

edited

Loading

lithp Dec 12, 2018

pipermerriam Dec 12, 2018

lithp Dec 12, 2018

lithp Dec 12, 2018

lithp Dec 12, 2018

pipermerriam Dec 12, 2018

carver Dec 12, 2018

veox Dec 12, 2018

lithp Dec 12, 2018 •

edited

Loading

lithp Dec 12, 2018

lithp Dec 12, 2018

lithp Dec 12, 2018

carver Dec 12, 2018

Use the state tests embedded in the blockchain tests #1577

Use the state tests embedded in the blockchain tests #1577

Conversation

lithp commented Dec 11, 2018 • edited Loading

Cute Animal Picture

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cburgdorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carver commented Dec 12, 2018

veox commented Dec 12, 2018

carver left a comment

Choose a reason for hiding this comment

carver Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lithp Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lithp commented Dec 11, 2018 •

edited

Loading

carver Dec 12, 2018 •

edited

Loading

lithp Dec 12, 2018 •

edited

Loading