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

feat(docs,tools): Add fixture formats specifications to the documentation; Refactor types #375

Merged
merged 11 commits into from
Jan 23, 2024

Conversation

marioevz
Copy link
Member

@marioevz marioevz commented Jan 9, 2024

🗒️ Description

Documentation Changes

Adds the fixture formats description to the documentation in the new "Consuming Tests" section.

The section contains 4 different pages at the moment:

The first three are the current fixture formats that the execution-spec-tests repository is able to generate.

The last page contains type descriptions that all fixture formats use.

Framework changes

This PR also performs some minor refactors to some of the types in order for them to be more coherent.

  • Fixture-related types such as FixtureWithdrawal and FixtureTransaction are moved from common into the spec/blockchain/types.py file with the rest of the definitions that are exclusively used for blockchain fixture generation.
  • Static methods were removed from FixtureBlock and replaced with lambda functions.
  • Optional was removed from many fixture fields, such as those where there was never an instance of None being used as value.

🔗 Related Issues

None

✅ Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

@spencer-tb
Copy link
Collaborator

Really nice! I'm liking the path this is going in :)

How do you guys feel about having separate pages for:

  1. A detailed consume/execution/using fixtures section. My assumption is that we can grow this in the future, having multiple consume X commands. We can add this within a follow up PR to: feat(pytest): add two commands that consume blockchain test fixtures #339
  2. A fixture format & types section in detail just as is added here, but only specific to the latter. Although the fixture consumption and format is very related, my gauge is that it could confuse the basic/first time users of the repo, hence the preference to seperate the 2.

I think we should also keep a basic consume pipeline command at the end of the getting started section?

Copy link
Member

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

It's really nice to have this documented and readily readable!

I'm not sure this is the right approach though. I'm not very keen on the data structures and their fields being manually written (copied) in the markdown. The source will likely get incrementally improved over time and these changes should ideally be propagated to the docs. I think the descriptions under each field should be extracted from the source instead of re-writing a docstring in the markdown. For example, we have:

https://github.com/marioevz/execution-spec-tests/blob/ded9147a19447e341e066e2832028fc9a6f72a26/src/ethereum_test_tools/spec/state/types.py#L30-L34

and:

https://github.com/marioevz/execution-spec-tests/blob/ded9147a19447e341e066e2832028fc9a6f72a26/docs/consuming_tests/state_test.md?plain=1#L19-L20

I would prefer it if the markdown is replaced with docstrings extracted from the python source. Happy to have a look at this and see if there's a reasonable approach to achieve it!

README.md Outdated Show resolved Hide resolved
docs/consuming_tests/state_test.md Outdated Show resolved Hide resolved
@marioevz
Copy link
Member Author

marioevz commented Jan 9, 2024

It's really nice to have this documented and readily readable!

I'm not sure this is the right approach though. I'm not very keen on the data structures and their fields being manually written (copied) in the markdown. The source will likely get incrementally improved over time and these changes should ideally be propagated to the docs. I think the descriptions under each field should be extracted from the source instead of re-writing a docstring in the markdown. For example, we have:

https://github.com/marioevz/execution-spec-tests/blob/ded9147a19447e341e066e2832028fc9a6f72a26/src/ethereum_test_tools/spec/state/types.py#L30-L34

and:

https://github.com/marioevz/execution-spec-tests/blob/ded9147a19447e341e066e2832028fc9a6f72a26/docs/consuming_tests/state_test.md?plain=1#L19-L20

I would prefer it if the markdown is replaced with docstrings extracted from the python source. Happy to have a look at this and see if there's a reasonable approach to achieve it!

Absolutely, this is the long term solution, and I think should be doable, but I could not figure out a way of extracting and formatting the information yet.

@marioevz marioevz force-pushed the feat/statetest-doc-spec-types branch from b183d54 to abd9ba4 Compare January 10, 2024 01:53
@marioevz
Copy link
Member Author

Added blockchain_test spec, only blockchain_test_hive is missing now.

@marioevz marioevz force-pushed the feat/statetest-doc-spec-types branch from abd9ba4 to 8530db4 Compare January 10, 2024 18:33
@marioevz marioevz changed the base branch from feat/statetest to main January 10, 2024 18:33
@marioevz marioevz changed the title [WIP] feat/statetest: Add fixture formats specifications to the documentation [WIP] Add fixture formats specifications to the documentation Jan 18, 2024
@marioevz marioevz force-pushed the feat/statetest-doc-spec-types branch from 8530db4 to 51fd03e Compare January 22, 2024 15:22
@marioevz marioevz force-pushed the feat/statetest-doc-spec-types branch from caf2ad3 to b9526ad Compare January 22, 2024 23:08
@marioevz marioevz changed the title [WIP] Add fixture formats specifications to the documentation Add fixture formats specifications to the documentation Jan 22, 2024
@marioevz marioevz changed the title Add fixture formats specifications to the documentation feat(docs,tools): Add fixture formats specifications to the documentation; Refactor types Jan 22, 2024
@marioevz marioevz added scope:tools Scope: ethereum_test_tools package scope:docs Scope: Documentation type:refactor Type: Refactor type:feat type: Feature labels Jan 22, 2024
@marioevz marioevz marked this pull request as ready for review January 23, 2024 01:07
@marioevz
Copy link
Member Author

This is ready for review @danceratopz @spencer-tb

The markdown lint is complaining a lot, and I've managed to fix most of the complains, but some I have not been able to fix, such as this long line: https://github.com/marioevz/execution-spec-tests/blob/b9526adcc278fbc27fe7407769f350e0a077ddc2/docs/consuming_tests/blockchain_test.md?plain=1#L157

I think the format could be improved but let me know what you think!

Copy link
Member

@danceratopz danceratopz 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, really love the "Description" and "Consumption" sections for each format.

I tried to make the intro a bit more compact in 068595c5675a665de83597e56a923a5597640773. Feel free to revert if you don't like it.

LGTM, except the one comment below in the Forks section in common types.

docs/consuming_tests/common_types.md Outdated Show resolved Hide resolved
Co-authored-by: danceratopz <danceratopz@gmail.com>
@marioevz marioevz merged commit 7b67353 into ethereum:main Jan 23, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:docs Scope: Documentation scope:tools Scope: ethereum_test_tools package type:feat type: Feature type:refactor Type: Refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants