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(et): add et make test for interactively creating new tests #950

Merged
merged 16 commits into from
Nov 26, 2024

Conversation

raxhvl
Copy link
Contributor

@raxhvl raxhvl commented Nov 13, 2024

πŸ—’οΈ Description

Scaffold tests using CLI.

πŸ”— Related Issues

closes #926, closes #946

βœ… 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: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

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.

Love it, really fun to use. Few minor changes.

I'll get back to the fork options in a bit πŸ™‚

@danceratopz danceratopz self-assigned this Nov 18, 2024
@danceratopz danceratopz added type:feat type: Feature scope:gentest Scope: Changes to gentest CLI command labels Nov 18, 2024
@raxhvl
Copy link
Contributor Author

raxhvl commented Nov 18, 2024

@danceratopz I have patched in all your feedback.

The make command group

I made a major change: Add a parent command called make. make_test was a make shift interface. This leaves room for future make commands ( test being on of the subcommands).

This also provides a nice UX (using click) . If an invalid subcommand is chosen, it throws an error and shows a list of valid subcommands. If no subcommand is present, it shows a list of valid subcommands to choose from (along with nice descriptions).

image

A bit of flare πŸ–ŒοΈ

Shows a quote when using this CLI. I know this a little extra. But I have pleasant memories of how Laravel would sprinkle quotes accross their dev CLI.

image

I can remove it if you think this is too much.

@raxhvl raxhvl marked this pull request as ready for review November 18, 2024 13:28
@raxhvl raxhvl requested a review from danceratopz November 18, 2024 13:28
@raxhvl raxhvl changed the title ✨(Make): Test ✨cli(make): Add test command to interactively new create test files Nov 19, 2024
@raxhvl raxhvl changed the title ✨cli(make): Add test command to interactively new create test files ✨ cli(make): Add test command for interactively creating new tests Nov 19, 2024
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.

Thanks, I really like the idea of a more generalized command structure!

As we discussed async, @spencer-tb proposed an eest namespace in #461. Inspired by uv I would personally propose a shorter top-level command et and add make as a sub-command of that. Then we'd have uv run et make test. I'm not a fan of using "make" without a top-level command (as GNU Make is a very popular command - GNU Make), but not if it's a sub-command. Should we add et in the scope of this PR with the sole sub-command et and then add other sub-commands in follow-up PRs?

I'm enjoying the flair πŸ–ŒοΈ, I'm not sure I like it in the context of make test, as I want to prominently see the module name I'm creating (and perhaps the command to fill it, including --until=<fork> if <fork> is a development fork). Could you remove the quotes from the make test output for now and we see where we can add something like this later on? But, feel free to leave src/cli/make/commands/quotes.py as part of the PR if you like.

There's a couple of comments below, I'm starting to have more ideas, but perhaps we should try and not get too carried away for now πŸ™‚

@raxhvl
Copy link
Contributor Author

raxhvl commented Nov 20, 2024

re: command structure

Strongly agree with uv run et make test . I will do it this PR.

re: quotes

I wasn't too sure if this is the right place. We could either:

  1. I bring the config command evm_init under make (uv run et make config ) and add the quote there in a separate PR.

OR

  1. We remove the quote.py file for now. My fork will have the code and we could add it back when we find a right place for it. We shouldn't keep files that are not referenced, this is a critical repository.

For this CLI, we could show a message like so:

πŸŽ‰ Success! Test file created at: {file_path}

πŸ“ Get started with tests:  https://ethereum.github.io/execution-spec-tests/v3.0.0/writing_tests

β›½ To fill this test, run: `uv run fill {file_path} --until={fork}`

@danceratopz
Copy link
Member

Strongly agree with uv run et make test . I will do it this PR.
Nice!

re: quotes

1. I bring the config command `evm_init` under `make` (`uv run et make config` ) and add the quote there in a separate PR.

This sounds good! I would prefer uv run et make env to create env.yaml though.
In the future, I'd quite like to add the ability to add a (per-use) config profile. Perhaps we can use make config for this. We don't need to decide now, but env feels right for this specific file.

For this CLI, we could show a message like so:

πŸŽ‰ Success! Test file created at: {file_path}

πŸ“ Get started with tests:  https://ethereum.github.io/execution-spec-tests/v3.0.0/writing_tests

β›½ To fill this test, run: `uv run fill {file_path} --until={fork}`

This looks great.

Please note that, since we fixed the the test module path in the reviews above, fill will now give an error:

_______ ERROR at setup of test_exhibit_x[fork_Prague-blockchain_test] _________
src/pytest_plugins/spec_version_checker/spec_version_checker.py:70: in reference_spec
    return get_ref_spec_from_module(request.module)
src/pytest_plugins/spec_version_checker/spec_version_checker.py:59: in get_ref_spec_from_module
    raise Exception("Test doesn't define REFERENCE_SPEC_GIT_PATH and REFERENCE_SPEC_VERSION")
E   Exception: Test doesn't define REFERENCE_SPEC_GIT_PATH and REFERENCE_SPEC_VERSION

For now we can populate these expected global variables with dummy values to make allow fill to run on these tests. I don't think it's worth trying to retrieve the required hash from ethereum/EIPs from Github in make test as I think we should remove this feature from fill sooner rather than later. It's not maintained or actively used.

@raxhvl
Copy link
Contributor Author

raxhvl commented Nov 22, 2024

Done

  1. Wrapped this command inside et πŸ‘½
  2. The command throws an error if the file exists.
  3. Using pathlib
  4. Removed quotes from this CLI.
  5. Fetching the list of forks dynamically.

Pending

1. UX with test module name

I'm not too sure how this should be because I haven't had a lot of experience writing tests.

2. Ensure the new test file passes fill command.

Unsure how to get the fill to pass. But I'm looking into it.

@danceratopz
Copy link
Member

danceratopz commented Nov 22, 2024

Done

1. Wrapped this command inside `et` πŸ‘½

2. The command throws an error if the file exists.

3. Using `pathlib`

4. Removed quotes from this CLI.

5. Fetching the list of forks dynamically.

Niiice! The fully populated fork selection menu is so nice.

Pending

1. UX with test module name

I'm not too sure how this should be because I haven't had a lot of experience writing tests

We can leave this for now, gather feedback from the team & close collaborators and iterate in follow-up PR(s).

2. Ensure the new test file passes fill command.

Unsure how to get the fill to pass. But I'm looking into it.

Had a quick look, sorry to gatecrash.

It looks like the default to field in a Transaction is a contract, so we should explicitly set this in the template to a harmless value, e.g.,

     env = Environment()

    sender = pre.fund_eoa()

    tx = Transaction(to=sender, sender=sender)

    post: dict[str, dict] = {}

    state_test(env=env, pre=pre, post=post, tx=tx)

I don't understand why this triggers the exception it does (TransactionException.TYPE_4_TX_CONTRACT_CREATION) and complaining about a type 4 transaction here is particularly fishy. So if you want to dig deeper, go for it. If not, please just create an issue for this, I think it'd be worth understanding.

I got this far quite quickly by running the generated test with the --evm-dump-dir flag and checking the input objects to t8n:

 uv run fill tests/constantinople/eip123_x_of_y/test_x_of_y.py --until=Paris --evm-dump-dir=/tmp/tx4

More info here:
https://ethereum.github.io/execution-spec-tests/main/filling_tests/debugging_t8n_tools/#evm-dump-directory

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.

Thanks a lot for adding et up front here. @spencer-tb gonna love it!

Just a couple of quick comments after trying the CLI out.

@raxhvl
Copy link
Contributor Author

raxhvl commented Nov 23, 2024

Fill now passes for all forks. This PR is now ready.

@raxhvl raxhvl requested a review from danceratopz November 23, 2024 07:30
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.

@raxhvl this is looking great. Love the et umbrella command and uv run et make test rolls off the fingers well. It's a fun feature that contributors will really love imo. Thanks for all your efforts on this!

I tested it out today and had a few small ideas. As we've already iterated so much on this I thought I'd join the party and add a few suggestions up front. I put them here:
raxhvl#1

…d-post-check

feat(make): add a contract to templates and a few other nits
@raxhvl
Copy link
Contributor Author

raxhvl commented Nov 26, 2024

@danceratopz the top tier documentation made me feel as if I wrote these enhancements! I have merged these nice additions.

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.

LGTM! πŸ™‚

@danceratopz danceratopz changed the title ✨ cli(make): Add test command for interactively creating new tests ✨ feat(et): add et make test for interactively creating new tests (#950) Nov 26, 2024
@danceratopz danceratopz changed the title ✨ feat(et): add et make test for interactively creating new tests (#950) ✨ feat(et): add et make test for interactively creating new tests Nov 26, 2024
@danceratopz danceratopz merged commit 8539345 into ethereum:main Nov 26, 2024
3 checks passed
@raxhvl raxhvl deleted the feat/make-test branch December 11, 2024 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:gentest Scope: Changes to gentest CLI command type:feat type: Feature
Projects
None yet
2 participants