From d963545ad6516c4be2535c29ad8661e06cc28ff1 Mon Sep 17 00:00:00 2001 From: apostasie Date: Fri, 18 Oct 2024 13:06:45 -0700 Subject: [PATCH] Fix ensurecontainerstarted logic and better debug Signed-off-by: apostasie --- docs/testing/tools.md | 4 +- pkg/testutil/nerdtest/registry/cesanta.go | 58 ++++++++++++----------- pkg/testutil/nerdtest/utilities.go | 46 ++++++++++-------- pkg/testutil/test/command.go | 8 +++- pkg/testutil/test/helpers.go | 2 +- 5 files changed, 69 insertions(+), 49 deletions(-) diff --git a/docs/testing/tools.md b/docs/testing/tools.md index d1f64cd4c80..2f53441b30c 100644 --- a/docs/testing/tools.md +++ b/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 diff --git a/pkg/testutil/nerdtest/registry/cesanta.go b/pkg/testutil/nerdtest/registry/cesanta.go index b8924e8c369..cf074542c35 100644 --- a/pkg/testutil/nerdtest/registry/cesanta.go +++ b/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) } } diff --git a/pkg/testutil/nerdtest/utilities.go b/pkg/testutil/nerdtest/utilities.go index 384f5132110..f23ce2b2c9a 100644 --- a/pkg/testutil/nerdtest/utilities.go +++ b/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) } } diff --git a/pkg/testutil/test/command.go b/pkg/testutil/test/command.go index a5b1de747d0..5f072e647a8 100644 --- a/pkg/testutil/test/command.go +++ b/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 { diff --git a/pkg/testutil/test/helpers.go b/pkg/testutil/test/helpers.go index 06411df8e99..9247f4098e8 100644 --- a/pkg/testutil/test/helpers.go +++ b/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, }) }