Skip to content

Conversation

@dapplion
Copy link
Collaborator

@dapplion dapplion commented Oct 16, 2024

Issue Addressed

As noted in

The existing range sync tests should adopt the event-driven style of block lookup tests.

Proposed Changes

This PR just moves the existing tests to a new directory structure so git doesn't trip and retains all the sweet blame in the 3000 lines of code of lookup tests. A future PR will migrate the range sync tests.

beacon_node/network/src/sync/tests/
├── lookups.rs
├── mod.rs
└── range.rs

The idea is that lookup and range will each impl TestRig for what they need to keep code separate. The common functions can either be made pub(crate) (if already in lookups.rs) or moved to mod.rs.

@dapplion dapplion added ready-for-review The code is ready for review syncing labels Oct 16, 2024
Copy link
Member

@realbigsean realbigsean left a comment

Choose a reason for hiding this comment

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

Looks straightforward

@jimmygchen jimmygchen self-assigned this Oct 24, 2024
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Make sense!

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Oct 25, 2024
@jimmygchen
Copy link
Member

@mergify queue

@mergify
Copy link

mergify bot commented Oct 25, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 8188e03

mergify bot added a commit that referenced this pull request Oct 25, 2024
@mergify mergify bot merged commit 8188e03 into sigp:unstable Oct 25, 2024
29 checks passed
@dapplion dapplion deleted the sync-tests-mod branch October 25, 2024 20:35
chong-he pushed a commit to chong-he/lighthouse that referenced this pull request Nov 26, 2024
* Generalize sync block lookup tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge This PR is ready to merge. syncing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants