Skip to content

Commit f624362

Browse files
lubronzhanclaude
andcommitted
fix: make TestWaitExecHandleHooks deterministic for 2-container hook ordering
Fixes a flaky test in TestWaitExecHandleHooks: "should return no error with 2 spec hooks in 2 different containers, 1st container starts running after 10ms, 2nd container after 20ms, both succeed" Observed failure (CI run https://github.com/velero-io/velero/actions/runs/28119573625/job/83267758421): mock: Unexpected Method Call ExecutePodCommand called with resourceVersion:"3" (both containers running), but mock was registered expecting resourceVersion:"2" (container1 running, container2 still waiting). Root cause: the two source.Modify calls were 10ms apart. The informer's DeltaFIFO queue can coalesce rapid updates, delivering only the latest pod state (resourceVersion:3) to the handler before the hook for container1 fires. The comment "each of these states will be seen by the UpdateFunc handler" was incorrect — intermediate states can be silently skipped under load. Fix: add waitForSignal chan struct{} to the change struct and onCalled func() to the expectedExecution struct. The goroutine now blocks after sending the first change until the mock signals that the hook has fired (via close), then sends the second change. This guarantees the handler observes the intermediate pod state (resourceVersion:2) when executing container1's hook. Signed-off-by: Lubron Zhan <lubronzhan@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 5844ce8 commit f624362

2 files changed

Lines changed: 30 additions & 3 deletions

File tree

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fix flaky TestWaitExecHandleHooks test for 2-container hook ordering by synchronizing pod state changes with hook execution using channels

internal/hook/wait_exec_hook_handler_test.go

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,13 +53,25 @@ func TestWaitExecHandleHooks(t *testing.T) {
5353
// delta to wait since last change applied or pod added
5454
wait time.Duration
5555
updated *corev1api.Pod
56+
// waitForSignal, if set, blocks the goroutine after applying this change
57+
// until the channel is closed. Use this to ensure the handler processes
58+
// an intermediate pod state before the next change is applied.
59+
waitForSignal chan struct{}
5660
}
5761
type expectedExecution struct {
5862
hook *velerov1api.ExecHook
5963
name string
6064
error error
6165
pod *corev1api.Pod
66+
// onCalled, if set, is invoked by the mock when ExecutePodCommand is called.
67+
// Use this together with change.waitForSignal to synchronize state transitions.
68+
onCalled func()
6269
}
70+
// hookFired is used by the test case that has two containers with hooks in
71+
// different containers. It ensures the second pod state change is only sent
72+
// after the first hook has fired, preventing the informer from coalescing
73+
// both updates and skipping the intermediate state.
74+
hookFired := make(chan struct{})
6375
tests := []struct {
6476
name string
6577
// Used as argument to HandleHooks and first state added to ListerWatcher
@@ -622,6 +634,8 @@ func TestWaitExecHandleHooks(t *testing.T) {
622634
},
623635
}).
624636
Result(),
637+
// Signal after this hook fires so the goroutine can apply the next change.
638+
onCalled: func() { close(hookFired) },
625639
},
626640
{
627641
name: "my-hook-1",
@@ -678,6 +692,10 @@ func TestWaitExecHandleHooks(t *testing.T) {
678692
},
679693
}).
680694
Result(),
695+
// Block until the hook for container1 has fired before sending the
696+
// next change. Without this, the informer may coalesce both updates
697+
// and deliver only resourceVersion:3, skipping the intermediate state.
698+
waitForSignal: hookFired,
681699
},
682700
// 2nd modification: container2 starts running, resourceVersion 3
683701
{
@@ -838,11 +856,15 @@ func TestWaitExecHandleHooks(t *testing.T) {
838856
go func() {
839857
// This is the state of the pod that will be seen by the AddFunc handler.
840858
source.Add(test.initialPod)
841-
// Changes holds the versions of the pod over time. Each of these states
842-
// will be seen by the UpdateFunc handler.
859+
// Changes holds the versions of the pod over time. The informer may
860+
// coalesce rapid updates, so use waitForSignal when a test requires the
861+
// handler to observe a specific intermediate state before the next change.
843862
for _, change := range test.changes {
844863
time.Sleep(change.wait)
845864
source.Modify(change.updated)
865+
if change.waitForSignal != nil {
866+
<-change.waitForSignal
867+
}
846868
}
847869
}()
848870

@@ -857,7 +879,11 @@ func TestWaitExecHandleHooks(t *testing.T) {
857879
for _, e := range test.expectedExecutions {
858880
obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(e.pod)
859881
require.NoError(t, err)
860-
podCommandExecutor.On("ExecutePodCommand", mock.Anything, obj, e.pod.Namespace, e.pod.Name, e.name, e.hook).Return(e.error)
882+
call := podCommandExecutor.On("ExecutePodCommand", mock.Anything, obj, e.pod.Namespace, e.pod.Name, e.name, e.hook).Return(e.error)
883+
if e.onCalled != nil {
884+
onCalled := e.onCalled
885+
call.Run(func(mock.Arguments) { onCalled() })
886+
}
861887
}
862888

863889
ctx := t.Context()

0 commit comments

Comments
 (0)