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

Bridging the spec to the tests generation #793

Closed
protolambda opened this issue Mar 17, 2019 · 7 comments
Closed

Bridging the spec to the tests generation #793

protolambda opened this issue Mar 17, 2019 · 7 comments

Comments

@protolambda
Copy link
Collaborator

Since a while we have the concept of an "executable spec".

And we have two versions:

This weekend I was in Taipei with @hwwhww, @ChihChengLiang, and we discussed a way forward to make it easier to generate tests for more complex parts of the spec, like state transition tests. We don't want to maintain duplicate code here in the test-generators repository.
Instead, we want to utilize the python "executable spec", to create tests with.

Proposal

The following proposal is based on discussion with @ChihChengLiang and @hwwhww, credits to them too.

@djrtwo had plans (or so I heard) to move the spec-pythonizer to the specs repo. This is great, as it enables a clean and straightforward CI design:

eth2.0-specs (repo)
    |
    | CI on merge into dev, master, or release (TBD)
    | job1: run CI sanity check
    | job2: run CI build script. (1)
    |
eth2.0-pyspec (repo)
    |
    | pulled in as a python dependency
    |
eth2.0-test-generators/<generator>/requirements.txt
    |
    | CI on release, generators have individual requirements.txt
    |     with a git dependency ("developer mode") (2)
    | Run generator (generator just imports all the necessary spec code,
    |                          outputs processed generated inputs) (3)
    | 
eth2.0-tests
    |
    | pulled in as git-submodule
    |
your client repository
    |
    | Run test-suite runner locally or in CI, which loads and runs the test YAML files
    |
We achieved conformance with the spec! (or not, fix your code ;P)

1: Build script: runs the code puller, and merges it with the helper/utils definitions for a fully executable spec. Then pushes it to an output repo. This process is very similar to the way the current test-generators repo builds the tests repo.

2: Each generator can switch to new pyspec versions (matches spec versioning) by updating their requirements.txt, and possibly making any necessary changes to the generation code to call the new/changed functions.

3: @hwwhww mentioned that we still have to do some work for creating fake blocks/invalid attestations/etc., but this is not duplicate, it's exactly what we want to do in the test generators :). We just import the spec to be able to super-reliably replicate spec behavior in the test-suite.

@djrtwo
Copy link
Contributor

djrtwo commented Mar 17, 2019

I'm definitely thinking along the same lines, but I wasn't originally planning to create a separate eth2.0-pyspec repo. It might be cleaner to just have a /build and /test in the same eth2.0-specs repo. Something like the following. I'd like the full suite of tests to exist in the spec repo for CI against the spec and just use the generator to wrap up these tests to output the yaml.

eth2.0-specs (repo)
    |
    | CI on merge into dev, master, or release (TBD)
    | job1: run CI build script to output exec spec to `/build` (1)
    | job2: run CI tests of `/build` against tests in `/test` (2)
    |
eth2.0-test-generators/<generator>/requirements.txt
    |
    | CI on release, generators have individual requirements.txt
    |     with a git dependency ("developer mode")
    | Run generator (generator just imports all the necessary spec & necessary test code,
    |                          outputs processed generated inputs)
    | 
eth2.0-tests
    |
    | pulled in as git-submodule
    |
your client repository
    |
    | Run test-suite runner locally or in CI, which loads and runs the test YAML files
    |
  1. pyspec found in /build get's released naturally with spec releases at same version number
  2. all tests that will be used in the generators are found in /test and follow a simple format that can be used in generators.
  3. agreed, but these are the same tools that will be useful in /test. The generator can be relatively dumb, just iterating through the tests and capturing the inputs and outputs.

Edit: do you see a significant advantage in pulling out to a separate repo? My main concern is that it is going to limit the tests that actually get run in the spec CI and instead kick it out to generator repo which is too far away from the spec dev imo.

@djrtwo
Copy link
Contributor

djrtwo commented Mar 17, 2019

additional comment -- Can easily tag tests in the /tests repo as "sanity" or "light" and only run the light weight tests in per PR CI. When doing a release, run everything.

@protolambda
Copy link
Collaborator Author

protolambda commented Mar 17, 2019

It might be cleaner to just have a /build and /test in the same eth2.0-specs repo.

I'm opposed to this. For several reasons:

  • You essentially duplicate (i.e. make a follow-up) commits through a bot. But the commits may not be sequential. It starts to become messy/complicated when updates go quick. And you even have to think about the bot triggering itself. (if you're not using different branches)
  • You could be affecting something you're running, which is something to avoid
  • You are pushing (unsigned) bot commits to a research repo with otherwise human-readable changes only. Let's keep it clean.

Alternatively, you would have it push to a separate disconnected branch if you really have to do it in one repo. But at this point I rather give the pyspec project a repo, and have anything that imports it through git cleanly download it, without dealing with any extra git history or file data. We can reference a version and/or commit hash to make sure we know the spec-source of a particular build.

Also, we made this exact same decision for test-generators <-> test-cases. And it works, is easy to maintain, and has clear rules on where permanent modifications can be made and what isn't desirable.

RE additional comment:

additional comment -- Can easily tag tests in the /tests repo as "sanity" or "light" and only run the light weight tests in per PR CI. When doing a release, run everything.

Good idea! There are multiple ways to implement this:

  • singular: We could add a single tag field. And tests are either "light" or "heavy" (default?), or maybe something else. Easy, but not as extensible.
  • plural: We could add a "tags" list to a suite header. And then you can filter tests by checking which tests contain any/every tag you filter for.
  • change up the typed tests system. Probably a bad idea, as it breaks encapsulation and identity of test-runners.
  • simplify: we may want to come up with a better name than tag, and just implement the idea of light, medium, large, heavy tests etc.

I would go with simplify. Or plural if anyone can come up with non-leveled requirements.

My thoughts about tags:
Defined in a header being interpreted as OR on a filter for a given tag (on specs repo we say: build light tests. In clients, we say: build light and middle tests. Sometimes, like on releases, you say: build any light, middle or heavy tests. And sometimes we may include another special test type). And if multiple tags are supplied, you AND those. (OR can be simulated by iterating the query).
And if you just want a special set of tests on a very particular topic (like having an AND in your header tags), you're likely better off by filtering by name or test-type anyway.

@protolambda
Copy link
Collaborator Author

Posting this here for transparency:

The work on CI:

previous proposal by me: (here, #793)
implementation by Danny: (#800, #809)
new proposal by me, after discussing pros/cons with Danny (i.e. circular dependency between spec and testing): https://hackmd.io/Hod52t2UQ7C3pJlrpbMSMQ#
revised proposal by Danny, after discussing my proposal over a call: https://notes.ethereum.org/vTxnCKjlRC22dpunA6haWg?view#

below is my personal interpretation, it's a lot to go through

Agree on:

  • testing pythonized spec on CI
  • sanity tests as requirement for merging
  • test-generators can use pythonized spec to create test-vectors
  • test-generators ought to be fixed before a new release comes out, so we can keep test-generators releases in sync with spec release (but allow for releases in generators, between to spec versions)

Pain points:

  • Non-sanity tests can also be run, use them to indicate breaking changes
  • YAML formatted tests in specs repo vs. hard-coded but minimal tests

Proposal to resolve pain points:

  • move the "tests" folder to be a subfolder in the scripts folder. If it's just part of a script, to only be consumed by CI, then make that very clear. If it's not, than we should use YAML and conform to standard test format here.

Disagree on:

  • How to initiate a change in the test-generators
    • me: make CI run all tests, indicates what breaks, and make PRs at least create an issue on the generators repo, if it's just tests outside of the sanity checks that break.
    • Danny: make CI on specs lightweight, most tests will break anyway. Arguably, one of the main reasons of breaking tests would be the need to re-run the generator. (kinda agree here). No clear update trigger for a generator.
  • Test-generators will load pyspec
    • me: please build pyspec in CI of specs repo, and output to something like eth2.0-pyspec. Have generators independently consume this dependency in requirements.txt, just like their other dependencies. Don't have all consumers implement their own submodule system and pyspec build-caller.
    • Danny/Hww: make generators rely on a git submodule of the specs repo. Good for versioning (there's always 1 specs submodule), but it will have to be built (i.e. functions pulled from spec) before using it.

Unclear: How do minimal (but hardcoded) tests of the specs repo, copy over to the tests suite? Do we just ignore them?

@djrtwo @hwwhww

@hwwhww
Copy link
Contributor

hwwhww commented Mar 22, 2019

Test-generators will load pyspec

I’d also support #412 in eth2.0-spec so that we can get rid of function_puller.py. 😄

Pros:

  • Much cleaner to the eth2.0-test-generator when importing it.

Cons:

  • The contributors will feel that they are hacking Python instead of proposing an idea. But maybe not that painful since everyone can raise a well-documented issue and maintainers can help implement it.

Unclear: How do minimal (but hardcoded) tests of the specs repo, copy over to the tests suite? Do we just ignore them?

I intuitively think that we can manually update them if the sanity checks are light.

@protolambda
Copy link
Collaborator Author

I’d also support #412 in eth2.0-spec so that we can get rid of function_puller.py. smile

Also brought up the idea of just having a python spec, and referring to line-numbers/function names in the readable spec. Then just have a bot compile it in a webpage/markdown file. It would be neat, but it may be too far from "specification", it would be more like documentation.

#412 Is interesting, and may be a good trade-off. But I would like to see some existing projects using it first, before taking that direction.

My priorities for now:

  1. get the Go spec up to date again this weekend
  2. implement (parts of) my test-suite proposal (Proposal for suite structure, test types, and configs eth2.0-test-generators#29). I want to know when and where my Go spec breaks. I will likely just ignore CI for now and just implement my own generators based on a locally generated pyspec. If there's some sane CI flow for pyspec integration into test-generators, that we agree on, I am happy to start contributing to state/block/sub-state test generators.
  3. write fuzzers for block (incl transactions) and state inputs

@djrtwo
Copy link
Contributor

djrtwo commented Apr 17, 2019

A form of this was implemented in #851

@djrtwo djrtwo closed this as completed Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants