diff --git a/changelogs/unreleased/9944-lubronzhan b/changelogs/unreleased/9944-lubronzhan new file mode 100644 index 000000000..25fb1f376 --- /dev/null +++ b/changelogs/unreleased/9944-lubronzhan @@ -0,0 +1 @@ +Fix flaky TestWaitExecHandleHooks test for 2-container hook ordering by synchronizing pod state changes with hook execution using channels \ No newline at end of file diff --git a/internal/hook/wait_exec_hook_handler_test.go b/internal/hook/wait_exec_hook_handler_test.go index bb0a7c8b1..0bcfafb61 100644 --- a/internal/hook/wait_exec_hook_handler_test.go +++ b/internal/hook/wait_exec_hook_handler_test.go @@ -53,13 +53,25 @@ func TestWaitExecHandleHooks(t *testing.T) { // delta to wait since last change applied or pod added wait time.Duration updated *corev1api.Pod + // waitForSignal, if set, blocks the goroutine after applying this change + // until the channel is closed. Use this to ensure the handler processes + // an intermediate pod state before the next change is applied. + waitForSignal chan struct{} } type expectedExecution struct { hook *velerov1api.ExecHook name string error error pod *corev1api.Pod + // onCalled, if set, is invoked by the mock when ExecutePodCommand is called. + // Use this together with change.waitForSignal to synchronize state transitions. + onCalled func() } + // hookFired is used by the test case that has two containers with hooks in + // different containers. It ensures the second pod state change is only sent + // after the first hook has fired, preventing the informer from coalescing + // both updates and skipping the intermediate state. + hookFired := make(chan struct{}) tests := []struct { name string // Used as argument to HandleHooks and first state added to ListerWatcher @@ -622,6 +634,8 @@ func TestWaitExecHandleHooks(t *testing.T) { }, }). Result(), + // Signal after this hook fires so the goroutine can apply the next change. + onCalled: func() { close(hookFired) }, }, { name: "my-hook-1", @@ -678,6 +692,10 @@ func TestWaitExecHandleHooks(t *testing.T) { }, }). Result(), + // Block until the hook for container1 has fired before sending the + // next change. Without this, the informer may coalesce both updates + // and deliver only resourceVersion:3, skipping the intermediate state. + waitForSignal: hookFired, }, // 2nd modification: container2 starts running, resourceVersion 3 { @@ -838,11 +856,15 @@ func TestWaitExecHandleHooks(t *testing.T) { go func() { // This is the state of the pod that will be seen by the AddFunc handler. source.Add(test.initialPod) - // Changes holds the versions of the pod over time. Each of these states - // will be seen by the UpdateFunc handler. + // Changes holds the versions of the pod over time. The informer may + // coalesce rapid updates, so use waitForSignal when a test requires the + // handler to observe a specific intermediate state before the next change. for _, change := range test.changes { time.Sleep(change.wait) source.Modify(change.updated) + if change.waitForSignal != nil { + <-change.waitForSignal + } } }() @@ -857,7 +879,11 @@ func TestWaitExecHandleHooks(t *testing.T) { for _, e := range test.expectedExecutions { obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(e.pod) require.NoError(t, err) - podCommandExecutor.On("ExecutePodCommand", mock.Anything, obj, e.pod.Namespace, e.pod.Name, e.name, e.hook).Return(e.error) + call := podCommandExecutor.On("ExecutePodCommand", mock.Anything, obj, e.pod.Namespace, e.pod.Name, e.name, e.hook).Return(e.error) + if e.onCalled != nil { + onCalled := e.onCalled + call.Run(func(mock.Arguments) { onCalled() }) + } } ctx := t.Context()