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

Combine specs and test-generators #851

Merged
merged 67 commits into from
Apr 16, 2019

Conversation

protolambda
Copy link
Collaborator

@protolambda protolambda commented Mar 28, 2019

See Issue #836 for motivation and reasoning on basic structure.

Blocked by #877 (Supporting new format, work in progress, see comment)

specs

The specs directory, unchanged. Where the markdown specs live.

test_generators

Full documentation on generators can be found in /test_generators/README.md.
If there is anything magic to remember, it's that you run make gen_yaml_tests to run the generators, the makefile will handle virtual environments/dependencies etc.

py-tests

Py-tests are meant to validate basic consistency and sanity expectations of the spec itself.
Documentation can be found here: /pytests/README.md. Generally not used by client teams, it just helps to make sure a PR is valid.

scripts

This contains the (basic, no dependencies) code to generate the pyspec from the markdown version.

test_libs

This contains all the python packages that one may use during any kind of testing (pyspec, test-generator utilities, and more in the future)

pyspec

Most of the pyspec is rather static, it is just the eth2/phase0/spec.py file being dynamically generated from the markdown for now. This file is git-ignored. The other files are just kept in place, to keep the build system simple. You run make pyspec to build the pyspec from the markdown.

More documentation in /test_libs/pyspec/README.md

gen_helpers

This is just a module to require in your yaml generator to make use of the base generator etc. Documentation can be found in /test_generators/README.md.

.circleci

The Circle CI configuration. Similar to before, but updated to handle yaml test generation, and make use of the new makefile commands.

@protolambda protolambda changed the title Combine specs and test-generators [WIP - do not merge] Combine specs and test-generators Mar 28, 2019
@protolambda
Copy link
Collaborator Author

Waiting for a review from @djrtwo on the CI script. I updated it to make use of the new makefile commands, but I have no access to the settings panel to make any changes, or verify if it runs.

Since the code is heavy on python configuration, I would appreciate a review from @hwwhww and @ChihChengLiang, and anyone else from Trinity, to make sure it meets their standards.

Anyone that loves writing make files, please comment as well. I tried to improve on the old one by creating the necessary aliases, auto detection of generators, and make good use of dependencies. But there may be room for improvement still.

The base-test-generator was discussed and reviewed before in ethereum/eth2.0-test-generators#31, but I'm still open for improvements on it.

Every review request aside, I can't wait to see the from eth2.phase0 import spec, and see tests directly being derived from the spec 🎉

@protolambda
Copy link
Collaborator Author

Also; the old YAML generators still work, and should output the same format as earlier. The new base generator aims at standardizing the header format, as described in ethereum/eth2.0-test-generators#29 (comment) (See section: test-suites > format). Other parts of that proposal are not included, but I would want to see more standardization, and see PRs for pyspec+base-gen based test generators.

Note: The test-format proposal is a bit out-of-date, as we (suggestion by Danny) are looking to make "operations" (previously "transactions") tested individually, rather than testing the sub-transition of the block that processes multiple at a time.

@djrtwo
Copy link
Contributor

djrtwo commented Mar 28, 2019

Closing and reopening to trigger circleci

@djrtwo djrtwo closed this Mar 28, 2019
@djrtwo djrtwo reopened this Mar 28, 2019
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My strong wish about module naming conflicts 😅: Py-EVM (Trinity) is using eth2 namespace and we may upload it to pypi; any chance that you can rename eth2 directory to pyspec? That means:

pyspec/  # project name
pyspec/pyspec/  # pyspec is the module name
pyspec/pyspec/phase0/state_transtion.py`  # import path: pyspec.phase0.state_transition

And, great work! 👍

.circleci/config.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
scripts/phase0/function_puller.py Show resolved Hide resolved
@protolambda
Copy link
Collaborator Author

My strong wish about module naming conflicts sweat_smile: Py-EVM (Trinity) is using eth2 namespace and we may upload it to pypi; any chance that you can rename eth2 directory to pyspec?

@hwwhww Yes, was thinking about that. Just wanted to not duplicate folder names. pyspec/pyspec does seem worth it, given the collision here. Will change it in a bit.

Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! Could I provide a bit of feedback as I'm working with tests in our team.
Hope it will be merged soon

scripts/phase0/build_spec.py Show resolved Hide resolved
specs/test_formats/README.md Outdated Show resolved Hide resolved
specs/test_formats/README.md Outdated Show resolved Hide resolved
test_generators/README.md Outdated Show resolved Hide resolved
test_generators/README.md Outdated Show resolved Hide resolved
zilm13 and others added 3 commits April 15, 2019 22:29
Co-Authored-By: protolambda <proto@protolambda.com>
Co-Authored-By: protolambda <proto@protolambda.com>
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!
Noted a bunch of very minor things. Generally happy with the structure as discussed in our many conversations irl

specs/test_formats/README.md Outdated Show resolved Hide resolved
specs/test_formats/operations/deposits.md Outdated Show resolved Hide resolved
specs/test_formats/shuffling/README.md Outdated Show resolved Hide resolved
specs/test_formats/ssz/uint.md Show resolved Hide resolved
test_generators/README.md Outdated Show resolved Hide resolved
test_generators/ssz/renderers.py Outdated Show resolved Hide resolved
test_generators/ssz/uint_test_cases.py Outdated Show resolved Hide resolved
test_libs/config_helpers/README.md Outdated Show resolved Hide resolved
test_libs/pyspec/eth2spec/debug/decode.py Show resolved Hide resolved
test_libs/pyspec/eth2spec/phase0/state_transition.py Outdated Show resolved Hide resolved
djrtwo and others added 13 commits April 15, 2019 23:05
Co-Authored-By: protolambda <proto@protolambda.com>
Co-Authored-By: protolambda <proto@protolambda.com>
Co-Authored-By: protolambda <proto@protolambda.com>
Co-Authored-By: protolambda <proto@protolambda.com>
Co-Authored-By: protolambda <proto@protolambda.com>
Co-Authored-By: protolambda <proto@protolambda.com>
Co-Authored-By: protolambda <proto@protolambda.com>
Co-Authored-By: protolambda <proto@protolambda.com>
Co-Authored-By: protolambda <proto@protolambda.com>
Co-Authored-By: protolambda <proto@protolambda.com>
Co-Authored-By: protolambda <proto@protolambda.com>
@protolambda
Copy link
Collaborator Author

protolambda commented Apr 15, 2019

Looks like this PR is finally ready to be merged. 🎉

If anyone wants more time to review, this is the time. However, I've been extensively discussing testing with Danny IRL here in Sydney, and I'm sure we could fix any remaining typos or style improvements later.
Also, I'm happy to announce that ZRNT has a working test-suite consumption system, with compile-time configurations, and a passing deposit tests suite.
With this PR we have the strong tooling, standardization, documentation and POC to get more state-transition test generators set up. Can't wait to make ZRNT fully conformant, and get full-client implementers up to speed with testing their spec-conformance.

Next up:

  • CI: yaml-test-generation trigger #928 CI triggering, output to new LFS repo (handle larger test vectors size 🚀 )
  • Test-generator: operations from blocks #927 More test-suites, very similar to the POC deposits test generation. Now that the tooling is in-place, it should be straight-forward to get test vectors for the other block-operations.
  • Configurations need to be hosted somewhere for clients to use easily (or they would need to submodule the specs itself to access). Yet, we can't just git-submodule the configs directory, as it needs to be updated here. I propose we set up a CI job to just put them in an automatically maintained git repository, similar as with the yaml-test-generation.

Possibly next up, depending on status (need confirmation on target SSZ version, and PR status):

Future steps:

  • Other types of tests, like:
    • fork-choice
    • BLS is still missing a few cases for verification of sigs
    • non-operation (transaction) block-processing sub-transitions
    • epoch-transition
      • handlers for each sub-transition, similar to block-processing
    • full state-transition with multiple blocks

Future ideas:

  • clients report test-coverage from CI to some shared service, and we gamify things a little bit (dashboard? charts? 🤔 )

Release plan:

  • This PR for v0.6
  • Some more operations being tested in v0.6
  • A stable v0.6; keep adding more tests for clients to test their work on this stable version. (non-spec-breaking)
  • See CI: yaml-test-generation trigger #928 for thoughts about deprecation tests and generators repo, and the new outputs becoming available from a Git LFS repo.

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the scope of this huge PR, I think we can merge it ASAP to avoiding more conflicts (especially many files are renamed/moved), and iterate the details later. :)

scripts/phase0/function_puller.py Outdated Show resolved Hide resolved
Co-Authored-By: protolambda <proto@protolambda.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants