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
991b130 - 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 <[email protected]>
(cherry picked from commit eac8357)
Signed-off-by: Paweł Gronowski <[email protected]>

Author: laurazard

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")
 	}
 }