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.

Sign up for GitHub

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

Jump to bottom

feat(make): allow creation of a new test module within an existing tests subfolder #1241

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat(make): allow creation of a new test module within an existing tests subfolder #1241

wants to merge 7 commits into from

Conversation

emrhncvsgl
Copy link
Contributor

🗒️ Description

This PR modifies the uv run eest make test command to allow creating new tests in a subfolder or with additional parameters described in #1179.

🔗 Related Issues

resolves #1179
related #973

✅ 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.

@emrhncvsgl
Copy link
Contributor Author

I noticed that the helper function references some forks without corresponding directories. After testing, I encountered a diff FileNotFoundError for those missing directories. A possible solution could be to either update the helper function to only include forks with existing directories or automatically create the missing directories. Let me know what you think is best!

@danceratopz
Copy link
Member

I noticed that the helper function references some forks without corresponding directories. After testing, I encountered a diff FileNotFoundError for those missing directories. A possible solution could be to either update the helper function to only include forks with existing directories or automatically create the missing directories. Let me know what you think is best!

Hey @Emirhan-Cavusoglu-sftw, thanks for this! If the corresponding fork directory doesn't, you can create it.

@danceratopz danceratopz self-assigned this Feb 21, 2025
danceratopz
danceratopz reviewed Feb 25, 2025
View reviewed changes
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 @emrhncvsgl!

A few comments:

  1. It's not clear when adding a new test module name ("Enter module name (snake_case)"), whether the test_ prefix is required or not in the user input (currently, test_the_thing becomes test_test_the_thing). I think the best thing for the user is to only add the test_ prefix if not present, otherwise leave is.
  2. "Select the fork" -> London still results in FileNotFoundError: [Errno 2] No such file or directory: 'tests/london'
  3. Please filter __pycache__ entries out when selecting the directory: 
    ? Choose the type of test to generate State
? Select the fork Osaka
? Select test directory (Use arrow keys)
 » Use current location
   eip123_foo
   eip123_barbar
   __pycache__
   eip7692_eof_v1
   ** Create new sub-directory **

danceratopz
danceratopz reviewed Feb 25, 2025
View reviewed changes
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 really good! I think the one last thing I've spotted is the templating of the test function name in the generated test module looks broken.
image

danceratopz
danceratopz reviewed Feb 26, 2025
View reviewed changes
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 again @emrhncvsgl! I added a couple of small commits. The changes to the templates just mean that the generated files pass our lint checks after creation (this problem was there before 🙂).

If you have time/like you can update the video in docs/writing_tests/img/eest_make_test.mp4 shown here:
https://ethereum.github.io/execution-spec-tests/main/writing_tests/

If not, I can update it tomorrow! Then we can get this merged.

@emrhncvsgl
Copy link
Contributor Author

Gonna update thanks 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danceratopz danceratopz danceratopz left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@danceratopz danceratopz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat(make): allow creation of a new test module within an existing tests subfolder
2 participants
@emrhncvsgl @danceratopz