gemini-code-assist[bot] commented on code in PR #38736:
URL: https://github.com/apache/beam/pull/38736#discussion_r3321826019
##########
sdks/go/pkg/beam/core/runtime/exec/datasampler_test.go:
##########
@@ -104,16 +104,9 @@ func TestDataSampler(t *testing.T) {
for _, sample := range test.samples {
dataSampler.SendSample(sample.PCollectionID,
sample.Element, sample.Timestamp)
}
- var samplesCount = -1
- var samples map[string][]*DataSample
- for i := 0; i < 5; i++ {
- samples = dataSampler.GetSamples(test.pids)
- if len(samples) == len(test.want) {
- samplesCount = len(samples)
- break
- }
- time.Sleep(time.Second)
- }
+ time.Sleep(1 * time.Second)
+ samples := dataSampler.GetSamples(test.pids)
+ samplesCount := len(samples)
Review Comment:
Using a hardcoded `time.Sleep(1 * time.Second)` in unit tests is an
anti-pattern. It introduces unnecessary delays when tests run quickly, and can
still cause flakiness under heavy CI load if the asynchronous operations take
longer than 1 second.
Since `GetSamples` is a destructive operation that clears the internal
buffer, we can resolve this by polling and accumulating the samples in a loop
with a small sleep interval (e.g., 10ms) until all expected samples are
received. This makes the test both faster and highly resilient.
```go
samples := make(map[string][]*DataSample)
for i := 0; i < 100; i++ {
for pid, s := range
dataSampler.GetSamples(test.pids) {
samples[pid] = append(samples[pid],
s...)
}
match := true
for pid, wantSamples := range test.want {
if len(samples[pid]) < len(wantSamples)
{
match = false
break
}
}
if match {
break
}
time.Sleep(10 * time.Millisecond)
}
samplesCount := len(samples)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
