Skip to content

Conversation

@winsvega
Copy link
Contributor

@winsvega winsvega commented Jan 5, 2024

πŸ—’οΈ Description

Test requested by chfast about suicide reentrancy and revert.

πŸ”— Related Issues

Resolves this issue.

βœ… 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.

Copy link
Member

@marioevz marioevz 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 to me :) Just a few comments, which might need a rebase.

It might be worth it to run tox to highlight any other issues related to linting if any (Just run tox in the execution-spec-tests folder with venv enabled).

@winsvega winsvega force-pushed the tests branch 6 times, most recently from 19ae098 to 14ad8e5 Compare January 8, 2024 10:35
@spencer-tb
Copy link
Collaborator

LGTM!

I'm nit-picking but it would be good to follow these 2 additional items that we started a few months ago. They don't need to be perfect but should help us in the future for finding issues or PRs in the future.

  1. Using a similar naming conventions in the PRs (and issues as well), to make it super easy to find again in the future if something related pops up. A good example for this PR is feat(tests): update kzg point evaulation test vectors to official setupΒ #336. So renaming it to something like:
    feat(tests): add double suicide revert test

  2. Using the checklist in the PR draft: https://github.com/ethereum/execution-spec-tests/blob/main/.github/PULL_REQUEST_TEMPLATE.md.

  • Tagging the related issues.
  • Adding the appropriate labels, personally I'd add type:feat and scope:tests.
  • Adding a line to the changelog.

@winsvega
Copy link
Contributor Author

winsvega commented Jan 8, 2024

Can this also be scripted in github CI checks?

@spencer-tb
Copy link
Collaborator

Great idea, for the labels most likely and similarly with the Changelog!

Maybe after a PR is created, depending on the labels assigned by the CI, the relavent PR name is given.

Can discuss on this weeks call :)

@winsvega winsvega changed the title double suicide revert test feat(tests): add reentrancy suicide revert test Jan 8, 2024
@winsvega
Copy link
Contributor Author

winsvega commented Jan 8, 2024

git commit also should have feat(tests): add double suicide revert test message?

@winsvega winsvega added scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature labels Jan 8, 2024
@winsvega
Copy link
Contributor Author

winsvega commented Jan 9, 2024

Also CI should try to fill the test and see if it actually fills without errors

@spencer-tb
Copy link
Collaborator

git commit also should have feat(tests): add double suicide revert test message?

I'd say the ideal scenario is to have a single squashed commit message with the above yeah - purely for improved commit history on main :)

Also CI should try to fill the test and see if it actually fills without errors

Tox should already be doing this, in the order specified within the tox.ini file

  • First for forks <=Shanghai -> here
  • Then for development forks =Cancun -> here

@spencer-tb spencer-tb merged commit 51d30bd into main Jan 9, 2024
@marioevz marioevz deleted the tests branch August 5, 2025 19:13
kclowes pushed a commit to kclowes/execution-spec-tests that referenced this pull request Oct 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants