Skip to content

Conversation

@marioevz
Copy link
Member

@marioevz marioevz commented Jan 28, 2025

Refactor of #1138.

  • Adds the only missing cases from tests/prague/eip7685_general_purpose_el_requests/test_request_types.py to tests/prague/eip7685_general_purpose_el_requests/test_deposits_withdrawals_consolidations.py
  • Removes tests/prague/eip7685_general_purpose_el_requests/test_request_types.py and the now unused fixtures it depended on.

@marioevz marioevz requested a review from danceratopz January 28, 2025 15:43
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.

Thanks. Agree, nicer to have the invalid test cases grouped together.

I verified the fixtures locally.

LGTM. One very minor nit regarding id naming 😆

correct_order_transactions,
correct_order + [bytes([fork.max_request_type() + 1])],
BlockException.INVALID_REQUESTS,
id="extra_invalid_type_request",
Copy link
Member

@danceratopz danceratopz Jan 29, 2025

Choose a reason for hiding this comment

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

I would just modify this test id to be more explicit and to better match the test cases removed from tests/prague/eip7685_general_purpose_el_requests/test_request_types.py

Suggested change
id="extra_invalid_type_request",
id="extra_invalid_type_request_with_no_data",

In particular, the "no data" case triggers an Engine API error, whereas the with_data_* test cases don't. Makes it easier to filter/target the no data cases if nothing else.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes total sense, thanks!

…_withdrawals_consolidations.py

Co-authored-by: danceratopz <[email protected]>
@marioevz marioevz merged commit e37a2fe into ethereum:7685-no-data Jan 29, 2025
11 checks passed
marioevz added a commit that referenced this pull request Jan 29, 2025
…n 1 byte (#1138)

* fix(tests): raise `InvalidParams` for requests shorter than 1 byte

* Refactor to #1138 (#1141)

* refactor(tests): EIP-7685: Add invalid cases

* refactor(tests): EIP-7685: Remove redundant `test_request_types.py`, remove unused fixtures

* small comment

* Update tests/prague/eip7685_general_purpose_el_requests/test_deposits_withdrawals_consolidations.py

Co-authored-by: danceratopz <[email protected]>

---------

Co-authored-by: danceratopz <[email protected]>

---------

Co-authored-by: Mario Vega <[email protected]>
marioevz added a commit to marioevz/execution-spec-tests that referenced this pull request Jan 30, 2025
…n 1 byte (ethereum#1138)

* fix(tests): raise `InvalidParams` for requests shorter than 1 byte

* Refactor to ethereum#1138 (ethereum#1141)

* refactor(tests): EIP-7685: Add invalid cases

* refactor(tests): EIP-7685: Remove redundant `test_request_types.py`, remove unused fixtures

* small comment

* Update tests/prague/eip7685_general_purpose_el_requests/test_deposits_withdrawals_consolidations.py

Co-authored-by: danceratopz <[email protected]>

---------

Co-authored-by: danceratopz <[email protected]>

---------

Co-authored-by: Mario Vega <[email protected]>
kclowes pushed a commit to kclowes/execution-spec-tests that referenced this pull request Oct 20, 2025
…n 1 byte (ethereum#1138)

* fix(tests): raise `InvalidParams` for requests shorter than 1 byte

* Refactor to ethereum#1138 (ethereum#1141)

* refactor(tests): EIP-7685: Add invalid cases

* refactor(tests): EIP-7685: Remove redundant `test_request_types.py`, remove unused fixtures

* small comment

* Update tests/prague/eip7685_general_purpose_el_requests/test_deposits_withdrawals_consolidations.py

Co-authored-by: danceratopz <[email protected]>

---------

Co-authored-by: danceratopz <[email protected]>

---------

Co-authored-by: Mario Vega <[email protected]>
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.

2 participants