Copilot commented on code in PR #3187:
URL: https://github.com/apache/dubbo-go/pull/3187#discussion_r2749397734
##########
cluster/cluster/failback/cluster_test.go:
##########
@@ -145,27 +145,28 @@ func TestFailbackRetryFailed(t *testing.T) {
mockFailedResult := &result.RPCResult{Err: perrors.New("error")}
invoker.EXPECT().Invoke(gomock.Any(),
gomock.Any()).Return(mockFailedResult)
- //
- var wg sync.WaitGroup
- retries := 2
- wg.Add(retries)
-
- // add retry call that eventually failed.
- for i := 0; i < retries; i++ {
- invoker.EXPECT().Invoke(gomock.Any(),
gomock.Any()).DoAndReturn(func(context.Context, base.Invocation) result.Result {
- // with exponential backoff, retries happen with
increasing intervals starting from ~1s
- wg.Done()
- return mockFailedResult
- })
- }
+ // Use atomic counter to safely track retries across goroutines.
+ // With exponential backoff and randomization factor, timing is
non-deterministic.
+ var retryCount int64
+ targetRetries := int64(2)
+
+ // add retry calls that eventually failed.
+ invoker.EXPECT().Invoke(gomock.Any(),
gomock.Any()).DoAndReturn(func(context.Context, base.Invocation) result.Result {
+ atomic.AddInt64(&retryCount, 1)
+ return mockFailedResult
+ }).MinTimes(int(targetRetries))
// first call should failed.
result := clusterInvoker.Invoke(context.Background(),
&invocation.RPCInvocation{})
require.NoError(t, result.Error())
assert.Nil(t, result.Result())
assert.Empty(t, result.Attachments())
- wg.Wait()
+ // Wait for at least targetRetries to complete
+ for atomic.LoadInt64(&retryCount) < targetRetries {
+ time.Sleep(100 * time.Millisecond)
+ }
+
time.Sleep(time.Second)
Review Comment:
Using a fixed sleep here reintroduces timing sensitivity and can still be
flaky on slow/loaded CI runners. Prefer waiting on the condition you actually
need (e.g., require.Eventually until taskList.Len() >= 1, with a timeout) and
remove the unconditional sleep.
```suggestion
// Wait until we've seen at least targetRetries and the task has been
re-queued.
require.Eventually(t, func() bool {
return atomic.LoadInt64(&retryCount) >= targetRetries &&
clusterInvoker.taskList.Len() >= int64(1)
}, 5*time.Second, 50*time.Millisecond)
```
##########
cluster/cluster/failback/cluster_test.go:
##########
@@ -145,27 +145,28 @@ func TestFailbackRetryFailed(t *testing.T) {
mockFailedResult := &result.RPCResult{Err: perrors.New("error")}
invoker.EXPECT().Invoke(gomock.Any(),
gomock.Any()).Return(mockFailedResult)
- //
- var wg sync.WaitGroup
- retries := 2
- wg.Add(retries)
-
- // add retry call that eventually failed.
- for i := 0; i < retries; i++ {
- invoker.EXPECT().Invoke(gomock.Any(),
gomock.Any()).DoAndReturn(func(context.Context, base.Invocation) result.Result {
- // with exponential backoff, retries happen with
increasing intervals starting from ~1s
- wg.Done()
- return mockFailedResult
- })
- }
+ // Use atomic counter to safely track retries across goroutines.
+ // With exponential backoff and randomization factor, timing is
non-deterministic.
+ var retryCount int64
+ targetRetries := int64(2)
+
+ // add retry calls that eventually failed.
+ invoker.EXPECT().Invoke(gomock.Any(),
gomock.Any()).DoAndReturn(func(context.Context, base.Invocation) result.Result {
+ atomic.AddInt64(&retryCount, 1)
+ return mockFailedResult
+ }).MinTimes(int(targetRetries))
// first call should failed.
result := clusterInvoker.Invoke(context.Background(),
&invocation.RPCInvocation{})
require.NoError(t, result.Error())
assert.Nil(t, result.Result())
assert.Empty(t, result.Attachments())
- wg.Wait()
+ // Wait for at least targetRetries to complete
+ for atomic.LoadInt64(&retryCount) < targetRetries {
+ time.Sleep(100 * time.Millisecond)
+ }
Review Comment:
The polling loop can hang indefinitely if retries never reach targetRetries
(e.g., scheduler stalls, logic regression, or gomock expectation mismatch).
Please add a bounded wait (e.g., require.Eventually with a reasonable timeout
and tick interval, or a context deadline) so the test fails fast instead of
blocking CI.
```suggestion
// Wait for at least targetRetries to complete, but bound the wait to
avoid hanging tests.
require.Eventually(t, func() bool {
return atomic.LoadInt64(&retryCount) >= targetRetries
}, 5*time.Second, 100*time.Millisecond)
```
--
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]
