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

new(tests): EIP-5656/7692 - use new marker to EOF-ize MCOPY test (2) #754

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

pdobacz
Copy link
Contributor

@pdobacz pdobacz commented Aug 19, 2024

This is #726 rebased after the EVM code type marker got merged to main and cleaned up. I also completed the set of EOF-MCOPY tests by applying the parameter in the remainder of existing 5656 tests.

I ran it through evmone intentionally breaking the MCOPY implementation in various ways, and the failures appear to be "in-sync" with ones which manifested in the old version of the test, as well as make sense in general.

@pdobacz
Copy link
Contributor Author

pdobacz commented Aug 22, 2024

@marioevz I have a question pertaining more to the evm code type marker in general, but following this MCOPY case as an example - how would such EOF-ized fixtures be organized and released? Right now they just land in the legacy MCOPY fixtures.

@marioevz
Copy link
Member

@marioevz I have a question pertaining more to the evm code type marker in general, but following this MCOPY case as an example - how would such EOF-ized fixtures be organized and released? Right now they just land in the legacy MCOPY fixtures.

As of right now, they should go into the Cancun fork folder so fixtures/state_tests/cancun/eip5656_mcopy/mcopy/mcopy_on_empty_memory.json for example, but I get what you mean that now we have tests for EOF outside of fixtures/state_tests/prague/eip7692_eof_v1/*, and there is not much that we can do regarding the output file path, but what I've been thinking we could do is dump the test markers to the resulting fixture.

E.g. JSON fixture file fixtures/state_tests/cancun/eip5656_mcopy/mcopy/mcopy_on_empty_memory.json could contain the following fixtures:

{
    "tests/cancun/eip5656_mcopy/test_mcopy.py::test_mcopy_on_empty_memory[fork_CancunEIP7692-evm_code_type_LEGACY-state_test-empty_memory-length_0-src_32-dest_32]": {
        ...
        "_info": {
            ...
            "markers": "evm_code_type=LEGACY",
            ...
        }
    },
    "tests/cancun/eip5656_mcopy/test_mcopy.py::test_mcopy_on_empty_memory[fork_CancunEIP7692-evm_code_type_EOF_V1-state_test-empty_memory-length_0-src_32-dest_32]": {
        ...
        "_info": {
            ...
            "markers": "evm_code_type=EOF_V1",
            ...
        }
    },

And this way test consumers can filter-out tests based on this information.

cc @danceratopz @spencer-tb @winsvega

@winsvega
Copy link
Contributor

Yes, another approach could be to separate

fixtures/state_tests/*
fixtures/state_tests_eof/*

So each test is filled in both modes. But stored to separate folders.

@spencer-tb
Copy link
Contributor

spencer-tb commented Aug 23, 2024

I think I like the folder approach better, until we have an EOF_V2!

Once we have V2+ I'd opt for the info section.

@marioevz
Copy link
Member

@spencer-tb @winsvega But this is not a different fixture format, so it kinda breaks the consistency that each of the subfolders at the first level are all fixture formats, and I feel that it's not justified.

@spencer-tb
Copy link
Contributor

Ah I understand now, we now have state/blockchain/eof test fixture formats. Yeah I'd like to keep the fixture format consistency!

We could even just add it as a root field in the fixtures. So instead of placing it in the _info section we add another evm_code root field.

@chfast
Copy link
Member

chfast commented Aug 26, 2024

The separate directory is not strictly needed but maybe good addition. However, you can dump everything into the same dir and wait if anyone complains...

@pdobacz
Copy link
Contributor Author

pdobacz commented Aug 27, 2024

@marioevz any steps needed from me to move forward with this?

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!

@marioevz
Copy link
Member

I created an issue to discuss how to handle automatically-generated EOF tests from other EIPs here #767

@marioevz marioevz merged commit d3d3219 into ethereum:main Aug 27, 2024
4 checks passed
@marioevz marioevz deleted the eofize-mcopy-mem-expansion branch August 27, 2024 19:11
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.

5 participants