test(pingpong): add synctest to ping test #5264
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the pingpong test to use Go's testing/synctest package for deterministic virtual time testing. The main changes include removing platform-specific workarounds for Windows timing issues, updating the RTT validation logic to handle virtual time, and temporarily limiting CI test runs to the pingpong package only.
- Replaced manual time delays and parallel testing with
synctest.Test()for deterministic virtual time - Updated RTT validation to accept zero values in synctest's virtual time environment
- Removed unused imports (
runtime,time,p2p)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/pingpong/pingpong_test.go | Refactored to use testing/synctest with virtual time, removed Windows-specific workarounds |
| Makefile | Temporarily restricted CI test targets to only run ./pkg/pingpong tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/pingpong/pingpong_test.go
Outdated
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| // check that RTT is a same value (rtt can be 0 in synctest virtual time) |
There was a problem hiding this comment.
Corrected spelling of 'same' to 'sane' in comment.
| // check that RTT is a same value (rtt can be 0 in synctest virtual time) | |
| // check that RTT is a sane value (rtt can be 0 in synctest virtual time) |
| } | ||
|
|
||
| // check that RTT is a sane value | ||
| if rtt <= 0 { |
There was a problem hiding this comment.
This test must evaluate that the rtt is not 0 as well, as zero value can also signal that the protocol does not work.
It could be now made that the protocol waits when responding to messages with the middleware for any os (to remove the goos check), that and to check if the rtt value is measured correctly.
There was a problem hiding this comment.
I tried to check the rtt is not zero as well, but the tests are failing. I think as I already mentioned since time is virtual in synctest so the the rtt can be zero.
https://github.com/ethersphere/bee/actions/runs/19616730484/job/56172634154
There was a problem hiding this comment.
Correct, we have to make the time to advance in order to measure rtt in the bubble, by blocking something. I thought thought that it is good to use the middleware that you removed that sleeps, but for any os. Test is working with that approach, but it actually does not make the test correct as it measures the middleware, and we want to measure messages reading and writing latencies. So, I have added the option to recorder to introduce the message latency so that we can measure rtt, now even exactly, which is much better than before.
There was a problem hiding this comment.
Ok, good.
Here it might be better to compare rtt as 2 * len(greetings) * messageLatency ?
…ate-to-synctest-pingpong
…nstead of a specific one
Checklist
Description
Add synctest in ping test.
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
#5232
Screenshots (if appropriate):