Skip to content

feat(consume): Add consume sync tests to sync clients after engine tests #2007

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

Merged
merged 2 commits into from
Aug 12, 2025

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Aug 7, 2025

🗒️ Description

Add a new test type that will send consume engine tests to client under test and then have a separate client sync to it. Parametrizes both client under test and sync client using hive client config file.

  • Fill e.g. uv run fill --fork=Osaka -k rlp_limit --clean (or narrow down to one or two tests)
  • Consume e.g. uv run consume sync -v -s (-s for good logs, -v for good test ids)

TODO:

  • Add documentation once we agree on the code after reviews
  • Add entry to CHANGELOG
  • Add some appropriate tests for the new command

✅ Checklist

  • All: Ran fast tox checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:
    uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • All: Considered adding an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).

@fselmo fselmo added type:feat type: Feature scope:consume Scope: Consume command suite labels Aug 7, 2025
@fselmo fselmo force-pushed the feat/consume-sync branch 2 times, most recently from aa628e9 to 2d11b62 Compare August 7, 2025 22:53
@fselmo
Copy link
Collaborator Author

fselmo commented Aug 7, 2025

The logic for checking the verify_sync flag is a little tricky. We check if the flag exists at all on the fixture here (if so, we mark it with sync marker) and then later, inside generate(), when we can actually get the value here, we either skip the test if it's set to False, as in this case with invalid payload, or we generate it.

This way if you use uv run fill --fork=Osaka -k rlp_size_limit_boundary --clean -m blockchain_test_sync_only, it will skip the negative case and only fill the other two cases. Without the -m flag it will fill all of blockchain, blockchain_engine and blockchain_engine_sync and still skip that single invalid sync case but fill all 3 cases for blockchain and blockchain_engine. I just couldn't get it to not skip and just not collect it at all.

I couldn't figure out a good way to completely skip test filling, instead of pytest.skip(), if the value is False, because by the time we are checking the value we have the fixture already. I think this is fine but lmk if you see it another way!

@fselmo
Copy link
Collaborator Author

fselmo commented Aug 8, 2025

From Discord:

I'm realizing now that the -m flag doesn't work for fill command for blockchain_test_only or blockchain_test_engine_only. It isn't registered for filling, only for consuming. This is what I was (wrongly) trying to accomplish with sync and why I jumped through those hoops. It seems filling doesn't make the -m distinction so I should not try to for the blockchain sync tests.

Changes incoming 🙂


edit: It looks like a -m blockchain_test_sync flag is built dynamically from somewhere and can be used to fill. Perhaps I'm misunderstanding the blockchain_test_*_only flag. I will do some more reading.

edit2: It seems these are different markers altogether and meant to mark tests to only fill one type of fixture. The more you know 🌈 ...

@fselmo fselmo force-pushed the feat/consume-sync branch from 8ad785b to 7041b1d Compare August 8, 2025 21:52
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.

Looks fantastic! Just a couple of comments, main one being that we should make the refactor of the verify_sync field in the BlockchainTest spec.

@fselmo
Copy link
Collaborator Author

fselmo commented Aug 9, 2025

main one being that we should make the refactor of the verify_sync field in the BlockchainTest spec.

Ok yeah, this is the best way I think as well. I'm glad you agree. I was wondering if we wanted to try to keep it this way for backward compatibility but since no tests were turned on yet for this, I think this is the way to go. Will go this route 👍🏼

fselmo added a commit to fselmo/execution-spec-tests that referenced this pull request Aug 10, 2025
- Use `pytest.mark.verify_sync` instead of the previous `verify_sync` flag on the blockchain test.
- Remove the syncing periodic checks as when the forkchoice_updated response is `VALID`, we can break.
- Update all files according to changes above.
fselmo added a commit to fselmo/execution-spec-tests that referenced this pull request Aug 10, 2025
Add a new type of test that is essentially `consume_engine` but gets another client to sync after payloads are executed.

- These tests can be filled with: `uv run fill ... -m "blockchain_test_sync"`
- Filled tests can be run against hive with `uv run consume sync`

Squashes:

- chore: fix formatting and lint errors
- chore: lower some of the timeouts and sleeps for sync tests
- chore(docs): Document consume sync and add entry to CHANGELOG.md
- Add tests for the ``verify_sync`` marker to make sure it's behaving as expected.
- chore: Don't convert state tests to sync tests
- refactor: Make test ids better

- Changes from comments on PR ethereum#2007
    - Use `pytest.mark.verify_sync` instead of the previous `verify_sync` flag on the blockchain test.
    - Remove the syncing periodic checks as when the forkchoice_updated response is `VALID`, we can break.
    - Update all files according to changes above.
@fselmo fselmo force-pushed the feat/consume-sync branch from 11ea5e8 to d1186c9 Compare August 10, 2025 23:02
@fselmo
Copy link
Collaborator Author

fselmo commented Aug 10, 2025

@marioevz Updated this to reflect your comments. I had some unideal paths to get to where we got and rebasing main with those changes didn't really add anything useful to the git history so I squashed this all as a single commit. Hopefully that didn't make it too hard to track the changes here.

Add a new type of test that is essentially `consume_engine` but gets another client to sync after payloads are executed.

- These tests can be filled with: `uv run fill ... -m "blockchain_test_sync"`
- Filled tests can be run against hive with `uv run consume sync`

Squashes:

- chore: fix formatting and lint errors
- chore: lower some of the timeouts and sleeps for sync tests
- chore(docs): Document consume sync and add entry to CHANGELOG.md
- Add tests for the ``verify_sync`` marker to make sure it's behaving as expected.
- chore: Don't convert state tests to sync tests
- refactor: Make test ids better

- Changes from comments on PR ethereum#2007
    - Use `pytest.mark.verify_sync` instead of the previous `verify_sync` flag on the blockchain test.
    - Remove the syncing periodic checks as when the forkchoice_updated response is `VALID`, we can break.
    - Update all files according to changes above.
@fselmo fselmo force-pushed the feat/consume-sync branch from d1186c9 to 680b84c Compare August 10, 2025 23:57
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.

Amazing! I feel this is pretty much ready to merge, just made two minor comments.

fselmo added a commit to fselmo/execution-spec-tests that referenced this pull request Aug 11, 2025
fselmo added a commit to fselmo/execution-spec-tests that referenced this pull request Aug 11, 2025
- Add marker to `pytest_configure` as an official marker and to silence warnings.
- Define a `BlockchainEngineSyncFixture.discard_fixture_format_by_marks` and specify
  `verify_sync` marker presence only for `BlockchainEngineSyncFixture`.
@fselmo fselmo force-pushed the feat/consume-sync branch from 2fdb77d to 9eb05d0 Compare August 11, 2025 22:26
- Add marker to `pytest_configure` as an official marker and to silence warnings.
- Define a `BlockchainEngineSyncFixture.discard_fixture_format_by_marks` and specify
  `verify_sync` marker presence only for `BlockchainEngineSyncFixture`.
@fselmo fselmo force-pushed the feat/consume-sync branch from 9eb05d0 to 00889e5 Compare August 11, 2025 22: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.

Awesome! Thanks so much

@marioevz marioevz merged commit 7b78f3c into ethereum:main Aug 12, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:consume Scope: Consume command suite type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants