Merge pull request #3573 from apostasie/test-ensure

Fix ensurecontainerstarted logic and better debug

docs/testing/tools.md

@@ -48,7 +48,9 @@ You already saw two (`test.Expects` and `test.Contains`):
 First, `test.Expects(exitCode int, errors []error, outputCompare Comparator)`, which is
 convenient to quickly describe what you expect overall.
 
-`exitCode` is obvious (note that passing -1 as an exit code will just verify the commands does fail without comparing the code).
+`exitCode` is obvious (note that passing -1 as an exit code will just
+verify the commands does fail without comparing the code, and -2 will not verify the exit
+code at all).
 
 `errors` is a slice of go `error`, that allows you to compare what is seen on stderr
 with existing errors (for example: `errdefs.ErrNotFound`), or more generally

pkg/testutil/nerdtest/registry/cesanta.go

@@ -82,34 +82,38 @@ func (cc *CesantaConfig) Save(path string) error {
 	return err
 }
 
+// FIXME: this is a copy of the utility method EnsureContainerStarted
+// We cannot reference it (circular dep), so the copy.
+// To be fixed later when we will be done migrating test helpers to the new framework and we can split them
+// in meaningful subpackages.
+
 func ensureContainerStarted(helpers test.Helpers, con string) {
-	const maxRetry = 5
-	const sleep = time.Second
-	success := false
-	for i := 0; i < maxRetry && !success; i++ {
-		time.Sleep(sleep)
-		count := i
-		cmd := helpers.Command("container", "inspect", con)
-		cmd.Run(&test.Expected{
-			Output: func(stdout string, info string, t *testing.T) {
-				var dc []dockercompat.Container
-				err := json.Unmarshal([]byte(stdout), &dc)
-				assert.NilError(t, err, "Unable to unmarshal output\n"+info)
-				assert.Equal(t, 1, len(dc), "Unexpectedly got multiple results\n"+info)
-				if dc[0].State.Running {
-					success = true
-					return
-				}
-				if count == maxRetry-1 {
-					// FIXME: there is currently no simple way to capture stderr
-					// Sometimes, it is convenient for debugging, like here
-					// Here we cheat with unbuffer which will bundle stderr and stdout together
-					// This is just bad
-					t.Error(helpers.Err("logs", con))
-					t.Fatalf("container %s still not running after %d retries", con, count)
-				}
-			},
-		})
+	started := false
+	for i := 0; i < 5 && !started; i++ {
+		helpers.Command("container", "inspect", con).
+			Run(&test.Expected{
+				ExitCode: test.ExitCodeNoCheck,
+				Output: func(stdout string, info string, t *testing.T) {
+					var dc []dockercompat.Container
+					err := json.Unmarshal([]byte(stdout), &dc)
+					if err != nil || len(dc) == 0 {
+						return
+					}
+					assert.Equal(t, len(dc), 1, "Unexpectedly got multiple results\n"+info)
+					started = dc[0].State.Running
+				},
+			})
+		time.Sleep(time.Second)
+	}
+
+	if !started {
+		ins := helpers.Capture("container", "inspect", con)
+		lgs := helpers.Capture("logs", con)
+		ps := helpers.Capture("ps", "-a")
+		helpers.T().Log(ins)
+		helpers.T().Log(lgs)
+		helpers.T().Log(ps)
+		helpers.T().Fatalf("container %s still not running after %d retries", con, 5)
 	}
 }
 

pkg/testutil/nerdtest/utilities.go

@@ -87,28 +87,36 @@ func InspectImage(helpers test.Helpers, name string) dockercompat.Image {
 }
 
 const (
-	maxRetry = 5
+	maxRetry = 10
 	sleep    = time.Second
 )
 
 func EnsureContainerStarted(helpers test.Helpers, con string) {
-	for i := 0; i < maxRetry; i++ {
-		count := i
-		cmd := helpers.Command("container", "inspect", con)
-		cmd.Run(&test.Expected{
-			Output: func(stdout string, info string, t *testing.T) {
-				var dc []dockercompat.Container
-				err := json.Unmarshal([]byte(stdout), &dc)
-				assert.NilError(t, err, "Unable to unmarshal output\n"+info)
-				assert.Equal(t, 1, len(dc), "Unexpectedly got multiple results\n"+info)
-				if dc[0].State.Running {
-					return
-				}
-				if count == maxRetry-1 {
-					t.Fatalf("container %s still not running after %d retries", con, count)
-				}
-				time.Sleep(sleep)
-			},
-		})
+	started := false
+	for i := 0; i < maxRetry && !started; i++ {
+		helpers.Command("container", "inspect", con).
+			Run(&test.Expected{
+				ExitCode: test.ExitCodeNoCheck,
+				Output: func(stdout string, info string, t *testing.T) {
+					var dc []dockercompat.Container
+					err := json.Unmarshal([]byte(stdout), &dc)
+					if err != nil || len(dc) == 0 {
+						return
+					}
+					assert.Equal(t, len(dc), 1, "Unexpectedly got multiple results\n"+info)
+					started = dc[0].State.Running
+				},
+			})
+		time.Sleep(sleep)
+	}
+
+	if !started {
+		ins := helpers.Capture("container", "inspect", con)
+		lgs := helpers.Capture("logs", con)
+		ps := helpers.Capture("ps", "-a")
+		helpers.T().Log(ins)
+		helpers.T().Log(lgs)
+		helpers.T().Log(ps)
+		helpers.T().Fatalf("container %s still not running after %d retries", con, maxRetry)
 	}
 }

pkg/testutil/test/command.go

@@ -28,6 +28,9 @@ import (
 	"gotest.tools/v3/icmd"
 )
 
+const ExitCodeGenericFail = -1
+const ExitCodeNoCheck = -2
+
 // GenericCommand is a concrete Command implementation
 type GenericCommand struct {
 	Config  Config
@@ -117,7 +120,10 @@ func (gc *GenericCommand) Run(expect *Expected) {
 		// Build the debug string - additionally attach the env (which iCmd does not do)
 		debug := result.String() + "Env:\n" + strings.Join(env, "\n")
 		// ExitCode goes first
-		if expect.ExitCode == -1 {
+		if expect.ExitCode == ExitCodeNoCheck { //nolint:revive
+			// -2 means we do not care at all about exit code
+		} else if expect.ExitCode == ExitCodeGenericFail {
+			// -1 means any error
 			assert.Assert(gc.t, result.ExitCode != 0,
 				"Expected exit code to be different than 0\n"+debug)
 		} else {

pkg/testutil/test/helpers.go

@@ -70,7 +70,7 @@ func (help *helpersInternal) Anyhow(args ...string) {
 // Fail will run a command and make sure it does fail
 func (help *helpersInternal) Fail(args ...string) {
 	help.Command(args...).Run(&Expected{
-		ExitCode: -1,
+		ExitCode: ExitCodeGenericFail,
 	})
 }