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

State tests for Attestations #999

Closed
wants to merge 5 commits into from
Closed

Conversation

zilm13
Copy link
Contributor

@zilm13 zilm13 commented Apr 25, 2019

I have tried to implement all cases or something similar for Attestations from #927
Also this PR includes some changes in everything I've noticed mistyped or incorrect on the way, related to attestations. If it's better to detach something to separate PR, just ask me.
full_suite with mainnet config is made with reduced set of cases, included only one heavy for example, because most of them need a lot of time to complete, however I've tried all cases and they finished after half an hour or so.

@protolambda
Copy link
Collaborator

protolambda commented Apr 26, 2019

Also this PR includes some changes in everything I've noticed mistyped or incorrect on the way, related to attestations. If it's better to detach something to separate PR, just ask me.

I would prefer to have a separate PR for unrelated changes (like the validator doc update). I.e. copy the branch (checkout as new), git rebase -i HEAD~n, change pick -> drop for unrelated changes, and push the unrelated commits to a separate branch.

full_suite with mainnet config is made with reduced set of cases, included only one heavy for example, because most of them need a lot of time to complete, however I've tried all cases and they finished after half an hour or so.

Honestly, if individual test suites take longer than a minute to run, they should probably not be included. In that case it is more productive to ensure that the tests based on the minimal config cover the same edge cases.

At first glance the attestation test-generation code looks good. However, I think we may need to change things a bit, as part of an effort to reduce total testing code in the specs repository (sorry 😞).

@djrtwo and I are looking into getting some of the test case "setup" work in test-generators and py-tests unified. To reduce duplicate effort. As you may know, we are testing the spec itself too, and the edge-cases being tested are mostly the same. See https://github.com/ethereum/eth2.0-specs/tree/dev/test_libs/pyspec/tests/block_processing
This is not to say it is 100% the same, but the "setup" part of the tests (edge case scenario being created) is. You can find an earlier proposal of mine to generalize it here: https://notes.ethereum.org/5N7MrvOESrKK3AIHYYvYww?view Since then I updated it with some newer ideas, but did not have the chance to fully implement it.
And we're still exploring the idea, there may be better ways to generalize it. Suggestions welcome :). I hope to implement something stable next week. I'm putting this PR on hold for now.

@zilm13
Copy link
Contributor Author

zilm13 commented Apr 27, 2019

@protolambda ok, waiting for further instruction about this PR when merging of pyspec tests and state tests is over. Checked your proposal: everything looks good, but anyway there could be cases in the future which will not be able to fit in this decorator. However if it could be used for most of the tests, it's great already.

As for speed of generator - I could comment this line https://github.com/ethereum/eth2.0-specs/pull/999/files#diff-559e6b9bef3fe6d1a20b7945f38b1c00R335 to be sure, that generator runs under 1 minute.

@protolambda
Copy link
Collaborator

Status update: we've unified the pytests; we can generate test-vectors from these tests now. So the format in this PR is not quite right anymore, as it would be more useful to make these tests part of the unified tests. I'll port over the code from here that presents new edge case coverage 👍

This was referenced May 11, 2019
@protolambda
Copy link
Collaborator

New tests included in #1052, closing this.

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.

3 participants