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(tests): EIP-1153: Convert tests from ethereum/tests #440

Merged
merged 9 commits into from
Jul 2, 2024
Merged

Conversation

winsvega
Copy link
Contributor

@winsvega winsvega commented Feb 15, 2024

🗒️ Description

convert vectors
1,2,3,4,5,6,7,8,9 11,12, 16,18, ,20
add a little parametrisation around calls
need review

EVMONE calculated coverage diff. on all mentioned converted files vs pyspec generated files
http://retesteth.ethdevops.io/temp/evmone/DIFF/

  • CBC - Covered Baseline Code.
  • G/L/UBC - Gained/Lost/Uncovered Baseline Coverage.
  • GBC means new lines and functions covered.
  • Absense of LBC red color means no coverage lost (with test_yul_coverage.json that executes a few evm instructions not invoked by Op.

🔗 Related Issues

#437
Tests remove PR: ethereum/tests#1383

✅ 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.
  • Tests: All converted JSON/YML tests from ethereum/tests have been added to converted-ethereum-tests.txt.

@winsvega winsvega added scope:pytest Scope: Pytest plugins scope:tests Scope: Test cases type:test Type: Test labels Feb 15, 2024
@winsvega winsvega force-pushed the eip1153 branch 4 times, most recently from c41f3bd to b59b40f Compare February 26, 2024 13:23
@winsvega winsvega force-pushed the eip1153 branch 2 times, most recently from 0add0ff to df6bc6b Compare March 11, 2024 13:59
@winsvega
Copy link
Contributor Author

winsvega commented Mar 18, 2024

This PR depends on OOG implementation #457

EVMONE calculated coverage diff. on all mentioned converted files vs pyspec generated files
http://retesteth.ethdevops.io/temp/evmone/DIFF/

hit 20 new lines and 18 new functions/paths
with introduction of yul_coverage.json no coverage is lost. this is because existing json tets vere generated using yul and different version of evm compared to plain Op. bytes. some opcodes were never used in converted pyspec tests

@winsvega winsvega force-pushed the eip1153 branch 7 times, most recently from 4e41d53 to df23f8d Compare March 26, 2024 11:10
@winsvega winsvega changed the base branch from main to op_oog March 26, 2024 11:12
@winsvega winsvega requested a review from marioevz March 26, 2024 11:25
@winsvega
Copy link
Contributor Author

I pointed it to op_oog so that git diff is clean.

@winsvega winsvega changed the base branch from op_oog to main March 26, 2024 12:47
@winsvega winsvega force-pushed the eip1153 branch 3 times, most recently from 590ef74 to be2cb3a Compare March 26, 2024 12:59
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 really like how these tests read!

I think we should merge these into the existing 1153 tests in due course as we now have essentially two separate bodies of tests (it's quite confusing in the online doc).

tests/cancun/eip1153_tstore/test_basic_tload.py Outdated Show resolved Hide resolved
tests/cancun/eip1153_tstore/test_basic_tload.py Outdated Show resolved Hide resolved
tests/cancun/eip1153_tstore/test_basic_tload.py Outdated Show resolved Hide resolved
tests/cancun/eip1153_tstore/test_basic_tload.py Outdated Show resolved Hide resolved
tests/cancun/eip1153_tstore/test_basic_tload.py Outdated Show resolved Hide resolved
converted-ethereum-tests.txt Outdated Show resolved Hide resolved
tests/cancun/eip1153_tstore/test_tload_calls.py Outdated Show resolved Hide resolved
tests/cancun/eip1153_tstore/test_tload_calls.py Outdated Show resolved Hide resolved
tests/cancun/eip1153_tstore/test_basic_tload.py Outdated Show resolved Hide resolved
@winsvega winsvega force-pushed the eip1153 branch 2 times, most recently from d52674f to 39ba4c2 Compare April 12, 2024 12:37
@winsvega
Copy link
Contributor Author

winsvega commented Jun 12, 2024

tests removed from repo:
ethereum/tests#1383

also introduced fixes to the coverage script

@winsvega winsvega force-pushed the eip1153 branch 2 times, most recently from 37868f1 to aa0a9f5 Compare June 12, 2024 11:51
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.

Thanks for this, some comments mainly because of the recently merged #584.

tests/cancun/eip1153_tstore/test_basic_tload.py Outdated Show resolved Hide resolved
tests/cancun/eip1153_tstore/test_basic_tload.py Outdated Show resolved Hide resolved
tests/cancun/eip1153_tstore/test_basic_tload.py Outdated Show resolved Hide resolved
tests/cancun/eip1153_tstore/test_basic_tload.py Outdated Show resolved Hide resolved
tests/cancun/eip1153_tstore/test_tload_calls.py Outdated Show resolved Hide resolved
tests/cancun/eip1153_tstore/test_tstore_reentrancy.py Outdated Show resolved Hide resolved
tests/cancun/eip1153_tstore/test_tstore_reentrancy.py Outdated Show resolved Hide resolved
tests/cancun/eip1153_tstore/test_tstore_reentrancy.py Outdated Show resolved Hide resolved
tests/cancun/eip1153_tstore/test_yul_coverage.py Outdated Show resolved Hide resolved
@winsvega
Copy link
Contributor Author

"some" comments ? )

I just updated it on recent main branch and it has to be all reworked again..

@winsvega winsvega force-pushed the eip1153 branch 6 times, most recently from cfa542a to bf7da58 Compare June 25, 2024 11:02
@winsvega winsvega requested a review from marioevz June 27, 2024 21:19
@winsvega
Copy link
Contributor Author

winsvega commented Jul 1, 2024

@marioevz
lets remove yul code like you suggested and untouched account.
this must produce 1 line missed coverage in report we just ignore it
and in the future in same scenario we remove untouched accounts from json tests to avoid this issue

@winsvega winsvega force-pushed the eip1153 branch 2 times, most recently from 3a6736b to 4e92159 Compare July 1, 2024 20:30
@marioevz marioevz changed the title feat(tests): convert a few eip1153 json tests feat(tests): EIP-1153: Convert tests from ethereum/tests Jul 2, 2024
@marioevz marioevz merged commit 4a2ad1b into main Jul 2, 2024
7 of 9 checks passed
@marioevz marioevz deleted the eip1153 branch July 2, 2024 17:54
@winsvega
Copy link
Contributor Author

winsvega commented Jul 4, 2024

the yul coverage had type3 tx.
it loses 16 lines wihtout it

  tx = Transaction(
        nonce=0,
        gas_limit=100000,
        to=address_to,
        data=b"",
        value=0,
        access_list=[],
        max_fee_per_gas=10,
        max_priority_fee_per_gas=5,
    )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:pytest Scope: Pytest plugins scope:tests Scope: Test cases type:test Type: Test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants