Skip to content
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

Rewrite the test runner #842

Closed
7 tasks done
jochem-brouwer opened this issue Aug 21, 2020 · 5 comments · Fixed by #849
Closed
7 tasks done

Rewrite the test runner #842

jochem-brouwer opened this issue Aug 21, 2020 · 5 comments · Fixed by #849

Comments

@jochem-brouwer
Copy link
Member

jochem-brouwer commented Aug 21, 2020

Placeholder issue to rewrite the test runner. It is too verbose right now and the code itself should be cleaned up.

Key points:

  • Some tests are not being ran (transition tests, except to Homestead to DAO)
  • Not all data available in tests are being used (postStateHash in Blockchain tests) (does not matter: if we verify state roots then this data is checked)
  • Explicitly verify that if a test is supposed to fail, it does fail
  • VM tests are not being ran (ethereum/tests/VMTests) Moved to BlockchainTests
  • Verify if the tests in config which are marked as "failing" are still failing (and why). Also verify that slow tests are being ran on nightly.
  • Check all CLI arguments for tester.js. They are not documented and most seem to be of (little) use.
  • Make tests run on both LegacyTests and on default tests
@jochem-brouwer
Copy link
Member Author

There's something I noticed when writing the DAO HF. The test runner checks if the chain should throw (or not). However, the error itself is not checked (this is obviously hard to do since this implies we need to "copy" the error strings from tests). However, this might mean that a package throws an error for a different reason than is expected.

Example in DAO: the expected errors are that if you create a block within 10 blocks after (and on) the DAO fork, the extraData field should be dao-hard-fork (you thus cannot manually put extraData here). However, without adding this requirement the chain would still throw, because the chain would throw an "invalid receipts trie" error (thus the VM did run transactions while it should immediately reject the block as it has a wrong extraData field). Not sure how we should fix this.

@holgerd77
Copy link
Member

@jochem-brouwer yeah, noticed this as well when adding the error checking to the test runner. This error checking is a bit blurry, when I once asked winsvega from the ethereum/tests team on this he mentioned that the error field in the tests was just internally and not meant to be used by the client test runners at all (lol 😛 ).

Due to this situation and the level of effort it would take for an integration (comparison of the specific ERROR keywords provided or the like) I would tend to not following up on this for the moment.

@jochem-brouwer
Copy link
Member Author

I agree with you here because this would be a massive effort. However in this test runner rewrite we should add "pass" tests if we expect an error and then explicitly write the expected error plus the actual error. If we then at some point glance over the test output manually we might spot an inconsistency when comparing these errors.

@holgerd77
Copy link
Member

@jochem-brouwer yeah, that's a good idea, the test output can generally be made more explicit and meaningful on various fronts.

@jochem-brouwer
Copy link
Member Author

jochem-brouwer commented Aug 26, 2020

I found something disturbing which is why I marked it P2.

After a single error, the test runner returns after it found this error. There are test files which have blocks past these faulty blocks which should also be checked (they could either be valid or also faulty).

This is problematic: this test has multiple faulty blocks in it. The test runs on the DAO fork and should import all these blocks and throw on the faulty ones.

What should happen:

Import block 1..4. Throw on block 5 (invalid extra data). Throw on block 5 (invalid gas used). Execute valid block 5. Throw on block 6 (invalid extra data). Import valid block 6... etc.

@jochem-brouwer jochem-brouwer self-assigned this Aug 26, 2020
This was referenced Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants