Block lookup tests with SyncTester#5530
Conversation
|
This is a great initiative! Writing the tests as a sequence of events allows us to change the internals safely without breaking tests. I would love to change some parts of sync, and this would help a lot. if you can inject any event you want into sync, why do you need to have traits for RangeSync, BackFillSync and BlockLookups? At least IMO, the whole point of this re-factor is to be able to change the APIs without breaking tests. Now, we should not have to couple testing to the specific API of its internal components. See this example of a test re-written in event-only form: 624dc8a // The peer provides the correct block, should not be penalized. Now the block should be sent
// for processing.
rig.send_sync_message(SyncMessage::RpcBlock {
request_id: SyncRequestId::SingleBlock { id },
peer_id,
beacon_block: Some(block.into()),
seen_timestamp: D,
});
rig.expect_empty_network();
rig.expect_block_process(response_type);
// The request should still be active.
assert_eq!(rig.active_lookup_count(), 1);
// Send the stream termination. Peer should have not been penalized, and the request removed
// after processing.
rig.send_sync_message(SyncMessage::RpcBlock {
request_id: SyncRequestId::SingleBlock { id },
peer_id,
beacon_block: None,
seen_timestamp: D,
});
rig.send_sync_message(SyncMessage::BlockComponentProcessed {
process_type: BlockProcessType::SingleBlock { id: id.id },
result: BlockProcessingResult::Ok(AvailabilityProcessingStatus::Imported(block_root)),
});
rig.expect_empty_network();
assert_eq!(rig.active_lookup_count(), 0); |
|
Nice, yeah I agree and prefer the simplicity too! I've removed the traits. |
…e extra beacon chain harness instance created in tests.
|
@dapplion @realbigsean |
|
@jimmygchen I really love this diagram and I think it should live in the repo somewhere as a Mermaid diagram. Thoughts? |
|
Thanks @jmcph4! lighthouse/beacon_node/network/src/sync/testing/mod.rs Lines 43 to 60 in 6bfeaba I initially wanted to do Mermaid diagram too but struggled a bit as the initial diagram was a bit more complicated, so I ended up doing it in ascii. (I'm also a noob with mermaid 😅) |
|
Superseded by #5534 |
Issue Addressed
This PR adds a new test utility for Lighthouse sync/lookup and a regression test that uses it.
Currently we're lacking integration tests around sync components and we're finding it difficult to reproduce production issues (especially ones that are related to race conditions). Hopefully this test utility would help increase coverage in this area and make reproducing sync/lookup related issues easier.
Proposed Changes
SyncTestertest utility. This takes a lot of inspiration from the existing syncTestRig, andDenebTester, with the intention of making it more flexible and event-driven - perhaps we can eventually consolidate them.SyncTester.BeaconChainHarnessinNetworkBeaconProcessor::null_for_testing, and this shaved off ~300ms from the above tests (~50%). I assume this would speed up existingDenebTestertests too.Extracted public methods of(not required, thanks @dapplion!)RangeSync,BackFillSyncandBlockLookupsinto traits (no logic change there), so we can swap out some components out during testing.Additional Info
The
SyncTesterenables sending messages toSyncManager, and then making assertions on outgoing messages from it.It covers the following:
SyncMessagetoSyncManagerto triggerRangeSync,BackFillSyncandBlockLookupsbehaviours.WorkEvents received from syncNetworkMessagereceived from sync (e.g. outgoing RPC requests).With these tests we can trigger behaviour and verify outcome, i.e. testing of all the arrows in the below diagram: