Skip to content

feat(fill,tests): add ported_from pytest marker#1590

Merged
marioevz merged 5 commits intomainfrom
converted_marker
May 20, 2025
Merged

feat(fill,tests): add ported_from pytest marker#1590
marioevz merged 5 commits intomainfrom
converted_marker

Conversation

@winsvega
Copy link

@winsvega winsvega commented May 12, 2025

add converted test marker to pytest

with a link to original legacy test source
https://github.com/ethereum/tests/blob/v13.3/src/GeneralStateTestsFiller/...
v13.3 contains all the test vectors at the moment of Cancun snapshot in ethereum/tests repo.
releases >13.3 just remove converted tests, converted eof tests, and limit the test vectors to >=cancun only and partially support prague evm revision

so each test now can have a marker that tells which tests it is converted from.
this will allow us to program ci scripts for converted tests if we want to.

🗒️ Description

🔗 Related Issues

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

@winsvega winsvega requested a review from danceratopz May 12, 2025 13:33
@winsvega winsvega self-assigned this May 12, 2025
@winsvega winsvega added scope:ci Scope: Continuous Integration type:feat type: Feature port Related to porting ethereum/tests to EEST labels May 12, 2025
@marioevz
Copy link
Member

I like the idea in general, and we could then potentially get rid of converted-ethereum-tests.txt.

I'm wondering though if the path specified inside the marker should better be the path where the static test lives in this repository, because I feel it's a bit confusing as it is right now.

cc @danceratopz @spencer-tb @felix314159

@danceratopz danceratopz changed the title add converted test marker to pytest feat(fill): add converted test marker to pytest May 13, 2025
@danceratopz danceratopz added the scope:fill Scope: fill command label May 13, 2025
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.

I like the idea in general, and we could then potentially get rid of converted-ethereum-tests.txt.

Yes, I think we should aim to get rid of this file within the scope of this PR.

I'm wondering though if the path specified inside the marker should better be the path where the static test lives in this repository, because I feel it's a bit confusing as it is right now.

Agree, I would lean towards a permalink to the test in ethereum/tests to easily access its history. See example below.

Also, it'd be good if we used consistent language. I think we typically talk about "the port" (not "the coversion")? If you agree, I would rename the marker to ported_from (also in suggestion below).

The file converted-ethereum-tests.txt also includes the EEST PR in which the tests were ported, should we add this to the marker, too? This could be more relevant, when ./tests get moved to execution-specs?

Also we need to register the marker (to avoid warnings) the -m flag in pytest.ini and pytest-execute.ini, e.g.,

diff --git a/pytest.ini b/pytest.ini
index bf100a7345..793fb09183 100644
--- a/pytest.ini
+++ b/pytest.ini
@@ -6,6 +6,7 @@ testpaths = tests/
 markers =
     slow
     pre_alloc_modify
+    ported_from
 addopts = 
     -p pytest_plugins.concurrency
     -p pytest_plugins.filler.pre_alloc

@winsvega
Copy link
Author

winsvega commented May 13, 2025

I'm wondering though if the path specified inside the marker should better be the path where the static test lives in this repository, because I feel it's a bit confusing as it is right now.

no it is no go. we gonna remove static tests from .py repo.

@winsvega winsvega force-pushed the converted_marker branch from 1e0f82f to 363d294 Compare May 19, 2025 10:18
@winsvega winsvega requested a review from danceratopz May 19, 2025 10:19
@marioevz
Copy link
Member

I'm wondering though if the path specified inside the marker should better be the path where the static test lives in this repository, because I feel it's a bit confusing as it is right now.

no it is no go. we gonna remove static tests from .py repo.

They are going to be removed from the ethereum/tests repository too, so it feels to me we are choosing between two eventually-stale paths, it doesn't sound ideal to me either way.

@danceratopz
Copy link
Member

They are going to be removed from the ethereum/tests repository too, so it feels to me we are choosing between two eventually-stale paths, it doesn't sound ideal to me either way.

As suggested, why don't we add a permalink to the filler? Then it's not stale and you get access to the history of the file. The plugin we'll add for the coverage can extract the relevant sub-path from the link.

@winsvega
Copy link
Author

winsvega commented May 20, 2025

that link to the file is going to be to some old release in ethereum/tests ?

https://github.com/ethereum/legacytests/blob/master/src/LegacyTests/Cancun/GeneralStateTestsFiller/Cancun/stEIP5656-MCOPY/MCOPY_memory_expansion_costFiller.yml

vs

https://github.com/ethereum/tests/blob/v13.3/src/GeneralStateTestsFiller/Cancun/stEIP5656-MCOPY/MCOPY_memory_expansion_costFiller.yml

v13.3 is identical to legacy cancun. alright and you can trace the history in ethereum/tests mostly.

@winsvega winsvega force-pushed the converted_marker branch from 363d294 to 776091e Compare May 20, 2025 12:35
@winsvega
Copy link
Author

updated

@winsvega winsvega requested a review from marioevz May 20, 2025 12:35
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.

LGTM, my only remaining question is whether this affects the coverage workflow in anyway and whether we should aim to deprecate the converted-ethereum-tests.txt ?

@winsvega
Copy link
Author

Now, when we port the test, we need to remember to put this markers.

Later, I can improve the coverage script to read the data from markers to trigger the coverage run.

@danceratopz danceratopz added the scope:tests Scope: Changes EL client test cases in `./tests` label May 20, 2025
@danceratopz danceratopz changed the title feat(fill): add converted test marker to pytest feat(fill,tests): add ported_from pytest marker May 20, 2025
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.

Really nice! This is a much nicer solution.

There are 2 EOF PRs in converted-ethereum-tests.txt that haven't been added to a ported_from marker:

Would you like to add them for completeness - then we can safely remove converted-ethereum-tests.txt?

Quick Q: Are we keeping converted-ethereum-tests.txt until the new pytest plugin arrives so we don't break coverage?

Also: I pushed a fix to a typo in converted-ethereum-tests.txt.

Preemptively approving!

@marioevz
Copy link
Member

Also: I pushed a fix to a typo in converted-ethereum-tests.txt.

This broke the coverage script lol 😅

Should be ok since the script does not expect changes like typo fixes, we can merge anyway.

@winsvega
Copy link
Author

winsvega commented May 20, 2025

Eof tests were almost completely rewritten.
And original fillers just plain bytecode.
Besides, it weren't a production ready test so i skipped that.

Need to track
#1615

And test scenarios

@marioevz marioevz merged commit b5f1d95 into main May 20, 2025
25 of 26 checks passed
@marioevz marioevz deleted the converted_marker branch May 20, 2025 20:03
kclowes pushed a commit to kclowes/execution-spec-tests that referenced this pull request Oct 20, 2025
* add ported_from test marker to converted from json tests

* chore: fix test in md link in `converted-ethereum-tests.txt`

* docs: update changelog

* Update docs/CHANGELOG.md

---------

Co-authored-by: danceratopz <danceratopz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

port Related to porting ethereum/tests to EEST scope:ci Scope: Continuous Integration scope:fill Scope: fill command 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