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.

Sign up for GitHub

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

Jump to bottom

fix(tests): EOF - EIP-3540: Organize code_validation.py tests #668

Merged
merged 2 commits into from
Jul 9, 2024
Merged

fix(tests): EOF - EIP-3540: Organize code_validation.py tests #668

merged 2 commits into from
Jul 9, 2024

Conversation

marioevz
Copy link
Member

@marioevz marioevz commented Jul 3, 2024

🗒️ Description

Update tests contained in tests/prague/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py, and delete the file.

Implement dynamic stack properties after the data portion of the opcode has been provided.

🔗 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 force-pushed the organize_code_validation_tests branch 2 times, most recently from e3872fc to 3af06fe Compare July 3, 2024 23:14
@marioevz marioevz requested a review from chfast July 3, 2024 23:14
chfast
chfast reviewed Jul 4, 2024
View reviewed changes
tests/prague/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py Outdated Show resolved Hide resolved
tests/prague/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py Outdated Show resolved Hide resolved
tests/prague/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py Outdated Show resolved Hide resolved
tests/prague/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py Outdated Show resolved Hide resolved
tests/prague/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py Outdated Show resolved Hide resolved
tests/prague/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py Show resolved Hide resolved
tests/prague/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py Outdated Show resolved Hide resolved
@marioevz marioevz force-pushed the organize_code_validation_tests branch 2 times, most recently from 7a1cf43 to f404143 Compare July 4, 2024 13:59
@marioevz marioevz requested a review from winsvega July 8, 2024 17:00
winsvega
winsvega reviewed Jul 8, 2024
View reviewed changes
Copy link
Collaborator

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

Hm this makes my existed test too complicated.

The parametrisation gets conditional
The test logic becomes hidden somewhere in sub preparators that have even more ifs

src/ethereum_test_tools/vm/opcode.py Show resolved Hide resolved
@winsvega
Copy link
Collaborator

winsvega commented Jul 8, 2024

can remote from this file now:
https://github.com/ethereum/execution-spec-tests/blob/main/tests/prague/eip7692_eof_v1/eip3540_eof_v1/test_example_valid_invalid.py

lines 88-100

pytest.param(
            # Sections with unreachable code fail

@marioevz marioevz force-pushed the organize_code_validation_tests branch from 5304071 to 3bdfe5d Compare July 8, 2024 22:53
marioevz added 2 commits July 9, 2024 15:45
@marioevz
feat(fw): opcodes: update stack properties after parsing data portion
1784850 
fix(fw): opcode mutability issue

fix(fw): `Bytecode` and `Opcode` docstrings

feat(fw): Implement comparison methods for Opcode

feat(fw): Add data portion to opcode string representation
@marioevz
fix(tests): refactor stack tests in code_validation.py
bb9c434 
fix(tests): refactor non-terminating opcode tests in code_validation.py

fix(tests): refactor terminating opcode tests in code_validation.py

Update tests/prague/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py

Co-authored-by: Paweł Bylica <[email protected]>

Update tests/prague/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py

Co-authored-by: Paweł Bylica <[email protected]>

Update tests/prague/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py

Co-authored-by: Paweł Bylica <[email protected]>

ORIGIN -> PUSH0

fix: review comments

fix(tests): EIP-3540: Unnecessary fixtures

fix(tests): remove `Op.RETF` from `halting_opcodes`

fix(tests): remove test duplicate

fix(tests): correctly use sets, fix min_stack_height usages

Co-authored-by: Dimitry Kh <[email protected]>

fix(tests): remove unnecessary generalization
@marioevz marioevz force-pushed the organize_code_validation_tests branch from 3bdfe5d to bb9c434 Compare July 9, 2024 15:45
@marioevz marioevz requested a review from winsvega July 9, 2024 18:12
winsvega
winsvega approved these changes Jul 9, 2024
View reviewed changes
Copy link
Collaborator

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

Whoa the tests are getting complicated, huh?

tests/prague/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py Show resolved Hide resolved
tests/prague/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py Show resolved Hide resolved
@marioevz marioevz merged commit a301708 into ethereum:main Jul 9, 2024
4 checks passed
@marioevz marioevz deleted the organize_code_validation_tests branch July 9, 2024 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chfast chfast chfast left review comments

@winsvega winsvega winsvega approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@marioevz @winsvega @chfast