✨ cli(make): Add `test` command for interactively creating new tests #950

raxhvl · 2024-11-13T00:37:32Z

🗒️ 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: All converted JSON/YML tests from ethereum/tests have been added to converted-ethereum-tests.txt.
Tests: A PR with removal of converted JSON/YML tests from ethereum/tests have been opened.
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.

danceratopz

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

I'll get back to the fork options in a bit 🙂

src/cli/make/cli.py

src/cli/make/templates/blockchain_test.py.j2

src/cli/make/templates/state_test.py.j2

src/cli/make/templates/blockchain_test.py.j2

src/cli/make/templates/state_test.py.j2

src/cli/make/cli.py

raxhvl · 2024-11-18T13:14:58Z

@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).

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.

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

danceratopz

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 🙂

danceratopz · 2024-11-19T15:08:43Z

src/cli/make/templates/blockchain_test.py.j2

@@ -0,0 +1,23 @@
+"""
+A blockchain test for [EIP-{{eip_number}} {{eip_name}}](https://eips.ethereum.org/EIPS/eip-{eip_number}}).


Ooh, I think that was my mistake.

Suggested change

A blockchain test for [EIP-{{eip_number}} {{eip_name}}](https://eips.ethereum.org/EIPS/eip-{eip_number}}).

A blockchain test for [EIP-{{eip_number}} {{eip_name}}](https://eips.ethereum.org/EIPS/eip-{{eip_number}}).

danceratopz · 2024-11-19T15:08:52Z

src/cli/make/templates/state_test.py.j2

@@ -0,0 +1,23 @@
+"""
+A state test for [EIP-{{eip_number}} {{eip_name}}](https://eips.ethereum.org/EIPS/eip-{eip_number}}).


Suggested change

A state test for [EIP-{{eip_number}} {{eip_name}}](https://eips.ethereum.org/EIPS/eip-{eip_number}}).

A state test for [EIP-{{eip_number}} {{eip_name}}](https://eips.ethereum.org/EIPS/eip-{{eip_number}}).

danceratopz · 2024-11-19T15:12:46Z

src/cli/make/commands/test.py

+is saved in the appropriate directory with a rendered template using Jinja2.
+"""
+
+import os


To exit the command with an error we'll use sys.exit(1)

Suggested change

import os

import os

import sys

danceratopz · 2024-11-19T15:14:28Z

src/cli/make/commands/test.py

+    file_name = f"test_{eip_name.lower().replace(' ', '_')}.py"
+
+    directory_path = f"tests/{fork.lower()}/eip{eip_number}_{eip_name.lower().replace(' ', '_')}"
+    file_path = f"{directory_path}/{file_name}"


We shouldn't clobber the file if it exists, please check this runs as expected:

Suggested change

file_path = f"{directory_path}/{file_name}"

file_path = f"{directory_path}/{file_name}"

if file_path.exists():

click.echo(f"The target test module {file_path} already exists!", err=True)

sys.exit(1)

Ah sorry, this won't work as-is as file_path is a str, not a pathlib.Path

danceratopz · 2024-11-19T15:16:23Z

src/cli/make/commands/test.py

+        fork=fork,
+        eip_number=eip_number,
+        eip_name=eip_name,
+        test_name=eip_name.lower().replace(" ", "_"),


I think we should prompt users for a test_module and a test_name. They might want to run this wizard multiple times for the same EIP.

This has implications for the UX though. If the folder (e.g. tests/prague/eip123_example_x/) exists, it would be more user-friendly to provided the folder as the first argument and the corresponding prompts skipped? Feel free to keep it simple for now. Just ideas.

I think we should prompt users for a test_module and a test_name. They might want to run this wizard multiple times for the same EIP.

But perhaps only if the "default" test module doesn't exits? E.g. if tests/prague/eip124_exhibit_x/test_exhibit_x.py exits then prompt for a test_module and test_name, otherwise keep it simple.

raxhvl · 2024-11-20T10:53:14Z

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:

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

OR

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 · 2024-11-20T11:06:26Z

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 · 2024-11-22T08:16:56Z

Done

Wrapped this command inside et 👽
The command throws an error if the file exists.
Using pathlib
Removed quotes from this CLI.
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 · 2024-11-22T09:13:50Z

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

danceratopz

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.

danceratopz · 2024-11-22T09:21:18Z

src/config/docs.py

+    DOCS_BASE_URL: str = f"https://ethereum.github.io/execution-spec-tests/v{AppConfig().version}"
+
+    # Documentation URLs prefixed with `DOCS_URL__` to avoid conflicts with other URLs
+    DOCS_URL__WRITING_TESTS: str = f"{DOCS_BASE_URL}/writing_tests/"


Great idea to use the config for this.

I think I'd prefer to point to main (for now) as we're pushing improvements to the docs much faster than we are releasing.

danceratopz · 2024-11-22T09:22:35Z

src/cli/et/make/templates/blockchain_test.py.j2

+
+from ethereum_test_tools import Alloc, Block, BlockchainTestFiller, Transaction
+
+REFERENCE_SPEC_GIT_PATH = "DUMMY/eip-DUMMY.md"


You could actually populate the correct information for REFERENCE_SPEC_GIT_PATH, but as mentioned, don't waste too much time as I'd prefer to remove this feature from EEST entirely.

Yes. I have created an issue to remove it #963.

Once we merge this, I will reference these lines in the issue.

raxhvl · 2024-11-23T07:30:47Z

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

danceratopz reviewed Nov 15, 2024

View reviewed changes

danceratopz self-assigned this Nov 18, 2024

danceratopz added type:feat type: Feature scope:gentest Scope: gentest CLI labels Nov 18, 2024

raxhvl force-pushed the feat/make-test branch from 52896cb to 1a877dd Compare November 18, 2024 13:00

raxhvl marked this pull request as ready for review November 18, 2024 13:28

raxhvl requested a review from danceratopz November 18, 2024 13:28

raxhvl changed the title ~~✨(Make): Test~~ ✨cli(make): Add test command to interactively new create test files Nov 19, 2024

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

danceratopz mentioned this pull request Nov 19, 2024

feat(fw): add eest utils and improve user repo init experience #461

Draft

10 tasks

danceratopz reviewed Nov 19, 2024

View reviewed changes

raxhvl mentioned this pull request Nov 22, 2024

🧹chore(test): Deprecate version references from tests #963

Open

danceratopz reviewed Nov 22, 2024

View reviewed changes

danceratopz mentioned this pull request Nov 22, 2024

fix: upgrade questionary to allow compatbility with ipython #966

Merged

2 tasks

raxhvl added 8 commits November 22, 2024 13:25

🚧 wip: Make test

51154db

✨ feat: Make test cli

d576291

🥢 nit: Docstring

62eb22d

🥢 nit: Quotes

0892ad9

✨ feat(CLI): et make test

1381001

🥢 nit: templates

153bbf3

✨ feat: et make test fill passing

d0f6f5b

🥢 nit: make: Update sender

c93a9bf

raxhvl force-pushed the feat/make-test branch from dac9049 to c93a9bf Compare November 22, 2024 12:25

🐞 fix: Ensure fill passes for all forks

a87edd5

raxhvl requested a review from danceratopz November 23, 2024 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ cli(make): Add `test` command for interactively creating new tests #950

✨ cli(make): Add `test` command for interactively creating new tests #950

raxhvl commented Nov 13, 2024 •

edited

Loading

danceratopz left a comment

raxhvl commented Nov 18, 2024 •

edited

Loading

danceratopz left a comment

danceratopz Nov 19, 2024

danceratopz Nov 19, 2024

danceratopz Nov 19, 2024

danceratopz Nov 19, 2024

danceratopz Nov 19, 2024

danceratopz Nov 19, 2024

danceratopz Nov 19, 2024

raxhvl commented Nov 20, 2024

danceratopz commented Nov 20, 2024

re: quotes

raxhvl commented Nov 22, 2024 •

edited

Loading

danceratopz commented Nov 22, 2024 •

edited

Loading

Done

Pending

1. UX with test module name

2. Ensure the new test file passes fill command.

danceratopz left a comment

danceratopz Nov 22, 2024

danceratopz Nov 22, 2024

raxhvl Nov 22, 2024

raxhvl commented Nov 23, 2024

		@@ -0,0 +1,23 @@
		"""
		A blockchain test for [EIP-{{eip_number}} {{eip_name}}](https://eips.ethereum.org/EIPS/eip-{eip_number}}).

		@@ -0,0 +1,23 @@
		"""
		A state test for [EIP-{{eip_number}} {{eip_name}}](https://eips.ethereum.org/EIPS/eip-{eip_number}}).

-    file_path = f"{directory_path}/{file_name}"
+    file_path = f"{directory_path}/{file_name}"
+    if file_path.exists():
+        click.echo(f"The target test module {file_path} already exists!", err=True)
+        sys.exit(1)


		from ethereum_test_tools import Alloc, Block, BlockchainTestFiller, Transaction

		REFERENCE_SPEC_GIT_PATH = "DUMMY/eip-DUMMY.md"

✨ cli(make): Add test command for interactively creating new tests #950

Are you sure you want to change the base?

✨ cli(make): Add test command for interactively creating new tests #950

Conversation

raxhvl commented Nov 13, 2024 • edited Loading

🗒️ Description

🔗 Related Issues

✅ Checklist

danceratopz left a comment

Choose a reason for hiding this comment

raxhvl commented Nov 18, 2024 • edited Loading

The make command group

A bit of flare 🖌️

danceratopz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raxhvl commented Nov 20, 2024

re: command structure

re: quotes

danceratopz commented Nov 20, 2024

re: quotes

raxhvl commented Nov 22, 2024 • edited Loading

Done

Pending

1. UX with test module name

2. Ensure the new test file passes fill command.

danceratopz commented Nov 22, 2024 • edited Loading

Done

Pending

1. UX with test module name

2. Ensure the new test file passes fill command.

danceratopz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raxhvl commented Nov 23, 2024

✨ cli(make): Add `test` command for interactively creating new tests #950

✨ cli(make): Add `test` command for interactively creating new tests #950

raxhvl commented Nov 13, 2024 •

edited

Loading

raxhvl commented Nov 18, 2024 •

edited

Loading

The `make` command group

raxhvl commented Nov 22, 2024 •

edited

Loading

danceratopz commented Nov 22, 2024 •

edited

Loading