Copilot commented on code in PR #873:
URL: https://github.com/apache/dubbo-go-pixiu/pull/873#discussion_r2726712774
##########
pkg/filter/metric/metric_test.go:
##########
@@ -523,12 +526,15 @@ func TestMetricReporterPushMode(t *testing.T) {
ctx.ClearMetrics()
}
- assert.GreaterOrEqual(t, atomic.LoadInt32(&pushHits), int32(1))
select {
case <-pushCh:
- case <-time.After(200 * time.Millisecond):
+ case <-time.After(2 * time.Second):
t.Fatalf("expected push gateway to be called")
}
+ assert.GreaterOrEqual(t, atomic.LoadInt32(&pushHits), int32(1))
+
+ // Give a grace period for the client to process the response
+ time.Sleep(200 * time.Millisecond)
Review Comment:
The grace-period Sleep is a brittle way to address concurrency/race issues
and increases test runtime without guaranteeing all push goroutines have
finished. Prefer waiting on explicit synchronization (e.g., drain the pushCh
for the expected number of pushes, or add a completion signal from the server
handler) so the test is timing-independent.
```suggestion
```
##########
pkg/filter/metric/metric_test.go:
##########
@@ -335,6 +335,9 @@ func TestFilterWithPushMode(t *testing.T) {
t.Fatalf("expected push gateway to be called")
}
assert.GreaterOrEqual(t, atomic.LoadInt32(&pushHits), int32(1))
+
+ // Give a small grace period for the client to process the response
+ time.Sleep(50 * time.Millisecond)
Review Comment:
The added sleep makes this test timing-based and still doesn’t guarantee the
push goroutine / httptest handler has fully completed. In this test the pushCh
signal is sent before the handler writes the response header (see the handler’s
pushCh send right before WriteHeader), so receiving from pushCh can happen
while the request is still in-flight. Consider signaling pushCh at the end of
the handler (e.g., via a defer) or using a WaitGroup/extra channel to
deterministically wait for completion, and then drop the Sleep.
##########
pkg/filter/metric/metric_test.go:
##########
@@ -523,12 +526,15 @@ func TestMetricReporterPushMode(t *testing.T) {
ctx.ClearMetrics()
}
- assert.GreaterOrEqual(t, atomic.LoadInt32(&pushHits), int32(1))
select {
case <-pushCh:
- case <-time.After(200 * time.Millisecond):
+ case <-time.After(2 * time.Second):
t.Fatalf("expected push gateway to be called")
}
Review Comment:
With PushInterval set to 5 and 15 iterations below, the code will start
multiple async pushes (likely 3). This test only waits for a single pushCh
signal, so additional in-flight pushes can continue past the end of the test
and still race with server teardown. To make this deterministic, either reduce
the simulated request count to just trigger one push, or wait/drain the
expected number of pushes (and assert against that) before exiting.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
