From eac83574c1712602dc016b7651e70142e3690612 Mon Sep 17 00:00:00 2001 From: Laura Brehm Date: Fri, 26 Jul 2024 14:46:01 +0100 Subject: [PATCH] tests/run: fix flaky `RunAttachTermination` test This test was just incorrect (and testing incorrect behavior): it was checking that `docker run` exited with a `context canceled` error after signalling the CLI/cancelling the command's context, but this was incorrect (and was fixed in 991b1303daec833d662ec54002d1b34c8805afab - which was when this test started failing). However, since this test assertion was happening inside of a goroutine, it would sometimes pass if this assertion didn't get to run before the test suite terminated. It was flaky because sometimes this assertion inside the goroutine did get to execute, but after the test finished execution, which is a big no-no. As an aside, assertions inside goroutines are generally bad, and `govet` even has a linter for this (but it only catches `t.Fatal` and `t.FailNow` calls and not `assert.Xx`. Signed-off-by: Laura Brehm --- cli/command/container/run_test.go | 117 ++++++++++++++++++++++++++---- 1 file changed, 103 insertions(+), 14 deletions(-) diff --git a/cli/command/container/run_test.go b/cli/command/container/run_test.go index 661cf8bdea32..390fc19d38d5 100644 --- a/cli/command/container/run_test.go +++ b/cli/command/container/run_test.go @@ -5,7 +5,6 @@ import ( "errors" "io" "net" - "os/signal" "syscall" "testing" "time" @@ -38,15 +37,85 @@ func TestRunLabel(t *testing.T) { assert.NilError(t, cmd.Execute()) } -func TestRunAttachTermination(t *testing.T) { +func TestRunAttach(t *testing.T) { p, tty, err := pty.Open() assert.NilError(t, err) + defer func() { + _ = tty.Close() + _ = p.Close() + }() + + var conn net.Conn + attachCh := make(chan struct{}) + fakeCLI := test.NewFakeCli(&fakeClient{ + createContainerFunc: func(_ *container.Config, _ *container.HostConfig, _ *network.NetworkingConfig, _ *specs.Platform, _ string) (container.CreateResponse, error) { + return container.CreateResponse{ + ID: "id", + }, nil + }, + containerAttachFunc: func(ctx context.Context, containerID string, options container.AttachOptions) (types.HijackedResponse, error) { + server, client := net.Pipe() + conn = server + t.Cleanup(func() { + _ = server.Close() + }) + attachCh <- struct{}{} + return types.NewHijackedResponse(client, types.MediaTypeRawStream), nil + }, + waitFunc: func(_ string) (<-chan container.WaitResponse, <-chan error) { + responseChan := make(chan container.WaitResponse, 1) + errChan := make(chan error) + + responseChan <- container.WaitResponse{ + StatusCode: 33, + } + return responseChan, errChan + }, + // use new (non-legacy) wait API + // see: 38591f20d07795aaef45d400df89ca12f29c603b + Version: "1.30", + }, func(fc *test.FakeCli) { + fc.SetOut(streams.NewOut(tty)) + fc.SetIn(streams.NewIn(tty)) + }) + + cmd := NewRunCommand(fakeCLI) + cmd.SetArgs([]string{"-it", "busybox"}) + cmd.SilenceUsage = true + cmdErrC := make(chan error, 1) + go func() { + cmdErrC <- cmd.Execute() + }() + + // run command should attempt to attach to the container + select { + case <-time.After(5 * time.Second): + t.Fatal("containerAttachFunc was not called before the 5 second timeout") + case <-attachCh: + } + + // end stream from "container" so that we'll detach + conn.Close() + + select { + case cmdErr := <-cmdErrC: + assert.Equal(t, cmdErr, cli.StatusError{ + StatusCode: 33, + }) + case <-time.After(2 * time.Second): + t.Fatal("cmd did not return within timeout") + } +} +func TestRunAttachTermination(t *testing.T) { + p, tty, err := pty.Open() + assert.NilError(t, err) defer func() { _ = tty.Close() _ = p.Close() }() + var conn net.Conn killCh := make(chan struct{}) attachCh := make(chan struct{}) fakeCLI := test.NewFakeCli(&fakeClient{ @@ -61,42 +130,62 @@ func TestRunAttachTermination(t *testing.T) { }, containerAttachFunc: func(ctx context.Context, containerID string, options container.AttachOptions) (types.HijackedResponse, error) { server, client := net.Pipe() + conn = server t.Cleanup(func() { _ = server.Close() }) attachCh <- struct{}{} return types.NewHijackedResponse(client, types.MediaTypeRawStream), nil }, - Version: "1.36", + waitFunc: func(_ string) (<-chan container.WaitResponse, <-chan error) { + responseChan := make(chan container.WaitResponse, 1) + errChan := make(chan error) + + responseChan <- container.WaitResponse{ + StatusCode: 130, + } + return responseChan, errChan + }, + // use new (non-legacy) wait API + // see: 38591f20d07795aaef45d400df89ca12f29c603b + Version: "1.30", }, func(fc *test.FakeCli) { fc.SetOut(streams.NewOut(tty)) fc.SetIn(streams.NewIn(tty)) }) - ctx, cancel := signal.NotifyContext(context.Background(), syscall.SIGTERM) - defer cancel() - - assert.Equal(t, fakeCLI.In().IsTerminal(), true) - assert.Equal(t, fakeCLI.Out().IsTerminal(), true) cmd := NewRunCommand(fakeCLI) cmd.SetArgs([]string{"-it", "busybox"}) cmd.SilenceUsage = true + cmdErrC := make(chan error, 1) go func() { - assert.ErrorIs(t, cmd.ExecuteContext(ctx), context.Canceled) + cmdErrC <- cmd.Execute() }() + // run command should attempt to attach to the container select { case <-time.After(5 * time.Second): - t.Fatal("containerAttachFunc was not called before the 5 second timeout") + t.Fatal("containerAttachFunc was not called before the timeout") case <-attachCh: } - assert.NilError(t, syscall.Kill(syscall.Getpid(), syscall.SIGTERM)) + assert.NilError(t, syscall.Kill(syscall.Getpid(), syscall.SIGINT)) + // end stream from "container" so that we'll detach + conn.Close() + select { - case <-time.After(5 * time.Second): - cancel() - t.Fatal("containerKillFunc was not called before the 5 second timeout") case <-killCh: + case <-time.After(5 * time.Second): + t.Fatal("containerKillFunc was not called before the timeout") + } + + select { + case cmdErr := <-cmdErrC: + assert.Equal(t, cmdErr, cli.StatusError{ + StatusCode: 130, + }) + case <-time.After(2 * time.Second): + t.Fatal("cmd did not return before the timeout") } }