Skip to content
This repository has been archived by the owner on Jul 1, 2021. It is now read-only.

v0.5.1 state tests #552

Merged
merged 9 commits into from
May 13, 2019
Merged

v0.5.1 state tests #552

merged 9 commits into from
May 13, 2019

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Apr 29, 2019

What was wrong?

v0.5.1 state-tests: https://github.com/ethereum/eth2.0-tests/tree/master/state
Add tests/eth2/beacon/state-fixtures/test_minimal_state.py for testing it.

How was it fixed?

  1. Add submodule ethereum/eth2.0-tests
  2. Add tests/eth2/beacon/state-fixtures/test_minimal_state.py
  3. Update BeaconStateMachine.__init__ for supporting setting pre_state in state machine.
  4. Sync the spec constants
  5. Simplify validate_proposer_signature interface

How to test it:

pytest tests/eth2/beacon/state-fixtures/ -s

(And v0.6.0 tests are coming...)

Cute Animal Picture

racoon-1414027_640

@hwwhww hwwhww added the eth2.0 label Apr 29, 2019
@hwwhww hwwhww force-pushed the yaml_test_2 branch 4 times, most recently from 0f94d8f to 4afba49 Compare May 2, 2019 12:21
@hwwhww hwwhww marked this pull request as ready for review May 2, 2019 12:37
Copy link
Contributor

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

Some comments, but the code generally looks good.

)

# Test files
ROOT_PROJECT_DIR = os.path.dirname(os.path.dirname(os.path.dirname(__file__)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Better do the path wrangling with pathlib

tests/eth2/beacon/state-fixtures/test_minimal_state.py Outdated Show resolved Hide resolved
tests/eth2/beacon/state-fixtures/test_minimal_state.py Outdated Show resolved Hide resolved
tests/eth2/beacon/state-fixtures/test_minimal_state.py Outdated Show resolved Hide resolved
def get_test_case(file_names):
all_test_cases = []
for file_name in file_names:
with open(BASE_FIXTURE_PATH + '/' + file_name, 'U') as f:
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip: In py-ssz the file path and line number of yaml, supported by ruamel.yaml, are passed into test_ids, so that devs can jump to the exact line of yaml file in the failing cases.

def make_test_id(filename, test_case):
    return f"{filename}:{test_case.lc.line}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that if I use yaml = YAML(typ='unsafe'), then I cannot use lc feature. 😮
I chose lc for debugging!


FILE_NAMES = [
"sanity-check_small-config_32-vals.yaml",
"sanity-check_default-config_100-vals.yaml",
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the test is slow because loading 100-vals tests yaml file is too slow. I tried CLoader and it can make loading 100-vals significantly faster.

from yaml import CLoader as Loader
with open("eth2-fixtures/state/sanity-check_default-config_100-vals.yaml") as f:
    yaml.load(f, Loader=Loader)

Copy link
Contributor

Choose a reason for hiding this comment

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

In ruamel.yaml, could speed up with yaml = YAML(typ='unsafe')

tests/eth2/beacon/state-fixtures/test_minimal_state.py Outdated Show resolved Hide resolved
Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

i ran over this -- looks good, other than the feedback @ChihChengLiang left!

@hwwhww hwwhww force-pushed the yaml_test_2 branch 2 times, most recently from c94a515 to 8c07f4a Compare May 13, 2019 04:44
@hwwhww hwwhww merged commit e74cc0c into ethereum:master May 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants