Skip to content

feat(pytest,tests): tests added by fork introduced #423

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

spencer-tb
Copy link
Contributor

@spencer-tb spencer-tb commented Feb 1, 2024

🗒️ Description

In the past we have had confusion on where tests should be added regarding the folder structure.

This PR aims to:

  1. Resolve this by forcing tests to be added to the folder in which the feature being tested was introduced.
  2. Determine what we should do for the cases where tests are marked valid_from a fork < the fork folder a test lies within.
  3. Help start the discussion: What structure should we following moving forward? Is the best way?

Primary additions currently

  • Added a base description to the docs, to help aid new contributors.
  • Added a check to the forks pytest plugin, asserting that the valid_from pytest marker is utilized correctly.
    • Where the valid_from fork must be greater than or equal to the fork folder the test is added.

Please note comments added below for further discussion.

🔗 Related Issues

Requires #418 for the yul test to compile from Frontier.

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

@spencer-tb spencer-tb added scope:pytest Scope: Changes EEST's pytest plugins scope:tests Scope: Changes EL client test cases in `./tests` type:feat type: Feature labels Feb 1, 2024
Comment on lines +358 to +363
fork_folder = metafunc.module.__name__.split(".")[1].capitalize()
if globals()[fork_name] < globals()[fork_folder]:
pytest.fail(
f"Incorrect usage of '{validity_marker_name}'. The validity marker fork "
f"'{fork_name}' must be greater than or equal to the test folder fork '{fork_folder}'."
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Validity marker check. Note this required us to move some existing tests or change the fork name.

@@ -39,7 +39,7 @@ def env(): # noqa: D103
)


@pytest.mark.valid_from("Paris")
@pytest.mark.valid_from("Cancun")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note this is still in Draft. I changed these to Cancun for now such that these tests pass the validity marker check.

What do we do in cases like this? To me if a test is valid before the fork where the eip or feature is introduced, it should not exist within that fork folder.

Are they specific to eip6780 or selfdestruct? If its the latter, I think we should move them to Homestead (where selfdestruct was introduced, well renamed from SUICIDE -> SELFDESTRUCT). Then mark them valid from the forks as is currently without the change.

@spencer-tb spencer-tb force-pushed the docs/specify-adding-tests-to-fork-introduced branch from c827ddb to d22bdf1 Compare February 1, 2024 12:18
@spencer-tb spencer-tb force-pushed the docs/specify-adding-tests-to-fork-introduced branch from d22bdf1 to f2ca3ad Compare February 1, 2024 12:19
Comment on lines +13 to +17
from ethereum_test_forks import ( # noqa: F401
Berlin,
Byzantium,
Cancun,
Constantinople,
Copy link
Contributor Author

@spencer-tb spencer-tb Feb 1, 2024

Choose a reason for hiding this comment

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

I would like to find a better way to this, but its needed forglobals()[fork]

@winsvega
Copy link
Contributor

winsvega commented Apr 7, 2025

up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:pytest Scope: Changes EEST's pytest plugins 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.

2 participants