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(forks,plugins): Fork transition test marker #1081

Merged
merged 15 commits into from
Jan 22, 2025

Conversation

marioevz
Copy link
Member

@marioevz marioevz commented Jan 16, 2025

🗒️ Description

Adds two keyword parameters to the valid_at_transition_to marker:

  • subsequent_forks: bool: If set to True, the test will be generated for the transition fork to the specified fork name, but also for all subsequent transition forks. Default is False.
  • until: str | None: Specify an end to the fork transitions to be filed when using subsequent_forks=True. Implies subsequent_forks=True. Default is None.

E.g.

@pytest.mark.valid_at_transition_to("Cancun", subsequent_forks=True)

produces tests on ShanghaiToCancunAtTime15k and CancunToPragueAtTime15k, and any transition fork after that.

Also added unit tests in src/pytest_plugins/forks/tests/test_markers.py.

Implementing tests for EIP-7691 in a separate PR, as required by lack of coverage pointed out in this thread: https://discord.com/channels/595666850260713488/1329093054698881087

Validity Markers Refactor

All validity markers are now defined as classes that subclass a base validity marker class, ValidityMarker.

This allows simpler validity marker definition, and easier handling of keyword arguments if it is the case that a validity marker supports them. See ValidAtTransitionTo for an example of how such a validity marker is defined.

🔗 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: 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.

@marioevz marioevz requested a review from danceratopz January 16, 2025 00:45
@marioevz marioevz force-pushed the fork-transition-test-marker branch from 420e221 to 270870b Compare January 16, 2025 12:51
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.

How about:

@pytest.mark.valid_at_transition_to("Cancun", subsequent_transitions=True, until="Osaka")

@marioevz marioevz force-pushed the fork-transition-test-marker branch from 0cf221b to cef281f Compare January 20, 2025 15:48
@marioevz
Copy link
Member Author

How about:

@pytest.mark.valid_at_transition_to("Cancun", subsequent_transitions=True, until="Osaka")

Addresed on the latest commit, which also includes a nice refactor of all validity markers into separate classes:

class ValidAtTransitionTo(ValidityMarker, mutually_exclusive=True):
"""Marker to specify that a test is valid at the transition to a specific fork."""
def process_with_args(
self, *fork_args, subsequent_forks: bool = False, until: str | None = None
) -> Set[Fork]:
"""Process the fork arguments."""
forks: Set[Fork] = self.process_fork_arguments(*fork_args)
until_forks: Set[Fork] | None = (
None if until is None else self.process_fork_arguments(until)
)
if len(forks) == 0:
pytest.fail(
f"'{self.test_name}': Missing fork argument with 'valid_at_transition_to' "
"marker."
)
if len(forks) > 1:
pytest.fail(
f"'{self.test_name}': Too many forks specified to 'valid_at_transition_to' "
"marker."
)
resulting_set: Set[Fork] = set()
for fork in forks:
resulting_set |= transition_fork_to(fork)
if subsequent_forks:
for transition_forks in (
transition_fork_to(f) for f in self.all_forks if f > fork
):
for transition_fork in transition_forks:
if transition_fork and (
until_forks is None
or any(transition_fork <= until_fork for until_fork in until_forks)
):
resulting_set.add(transition_fork)
return resulting_set

@marioevz marioevz force-pushed the fork-transition-test-marker branch from 10905cb to 53754e9 Compare January 20, 2025 21:53
Copy link
Contributor

@winsvega winsvega left a comment

Choose a reason for hiding this comment

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

have a couple of questions

* refactor(forks): make `name()` an abstractmethod

* refactor(forks): add helper methods to simplify fork comparisons
@marioevz
Copy link
Member Author

@danceratopz merged your suggestion from the other PR, this one should be ready 😄

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.

The refactoring of the validity markers is really nice! It adds much more structure to this plugin (I think this is one of the most complex parts of our pytest code). And really nice additional tests for these, btw! Few minor comments below.

docs/writing_tests/test_markers.md Show resolved Hide resolved
src/ethereum_test_forks/base_fork.py Outdated Show resolved Hide resolved
src/pytest_plugins/forks/forks.py Outdated Show resolved Hide resolved
src/pytest_plugins/forks/forks.py Show resolved Hide resolved
src/pytest_plugins/forks/forks.py Outdated Show resolved Hide resolved
src/pytest_plugins/forks/forks.py Outdated Show resolved Hide resolved
src/pytest_plugins/forks/forks.py Show resolved Hide resolved
src/pytest_plugins/forks/forks.py Show resolved Hide resolved
@marioevz marioevz merged commit 607d1b6 into main Jan 22, 2025
21 checks passed
@marioevz marioevz deleted the fork-transition-test-marker branch January 22, 2025 17:35
fselmo pushed a commit to fselmo/execution-spec-tests that referenced this pull request Jan 24, 2025
* feat(forks): Adds `supports_blobs` method

* fix(forks): Transition fork comparisons

* refactor,fix(plugins/forks): `fork_transition_test` marker

* docs: add documentation, changelog

* refactor(plugins/forks): Validity markers as classes

* fix: fork set handling

* fix(plugins): config variable usage in other plugins

* fix(plugins/forks): Self-documenting code for validity marker classes

* whitelist

* refactor(forks): suggestions for fork transition marker changes (ethereum#1104)

* refactor(forks): make `name()` an abstractmethod

* refactor(forks): add helper methods to simplify fork comparisons

* Update src/pytest_plugins/forks/forks.py

Co-authored-by: danceratopz <[email protected]>

* Update src/pytest_plugins/forks/forks.py

Co-authored-by: danceratopz <[email protected]>

* Update src/pytest_plugins/forks/forks.py

Co-authored-by: danceratopz <[email protected]>

* fixup

---------

Co-authored-by: danceratopz <[email protected]>
codeofcarson pushed a commit to codeofcarson/execution-spec-tests that referenced this pull request Jan 24, 2025
* feat(forks): Adds `supports_blobs` method

* fix(forks): Transition fork comparisons

* refactor,fix(plugins/forks): `fork_transition_test` marker

* docs: add documentation, changelog

* refactor(plugins/forks): Validity markers as classes

* fix: fork set handling

* fix(plugins): config variable usage in other plugins

* fix(plugins/forks): Self-documenting code for validity marker classes

* whitelist

* refactor(forks): suggestions for fork transition marker changes (ethereum#1104)

* refactor(forks): make `name()` an abstractmethod

* refactor(forks): add helper methods to simplify fork comparisons

* Update src/pytest_plugins/forks/forks.py

Co-authored-by: danceratopz <[email protected]>

* Update src/pytest_plugins/forks/forks.py

Co-authored-by: danceratopz <[email protected]>

* Update src/pytest_plugins/forks/forks.py

Co-authored-by: danceratopz <[email protected]>

* fixup

---------

Co-authored-by: danceratopz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants