From b910390f713c3e13bbff8ff2384ceeafc7351068 Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Fri, 1 May 2026 10:30:09 -0500 Subject: [PATCH 01/18] Default kiln test to docker-virtual Artifactory path Remove --broadcom-proxy. The embedded Dockerfile pins FROM lines on\ntas-rel-eng-docker-virtual and sets GOPROXY/GOSUMDB from ARTIFACTORY\nbuild args. Kiln always requires ARTIFACTORY_USERNAME and\nARTIFACTORY_PASSWORD, passes them as the only ImageBuild build args,\nand sends AuthConfigs for DockerVirtualRegistryHost so pulls work\nwithout docker login. Retains quiet image build output and the\nContainerWait(next-exit) fix for AutoRemove. ai-assisted=yes Co-authored-by: Cursor --- README.md | 43 +++++--- internal/commands/test_tile.go | 14 ++- .../commands/test_tile_artifactory_test.go | 58 ++++++++++ internal/commands/test_tile_test.go | 60 ++++++++--- internal/test/Dockerfile | 30 +++--- internal/test/container.go | 101 +++++++++++++++--- internal/test/container_test.go | 81 ++++++++++++++ 7 files changed, 325 insertions(+), 62 deletions(-) create mode 100644 internal/commands/test_tile_artifactory_test.go diff --git a/README.md b/README.md index 77a4a2af6..31960c6f1 100644 --- a/README.md +++ b/README.md @@ -520,18 +520,18 @@ Any variables that Kilnfile needs for the kiln re-bake command should be set in ### `test` -The `test` command exercises to ginkgo tests under the `//test/manifest` and `//migrations` paths of the `pivotal/tas` repos (where `` is tas, ist, or tasw). +The `test` command exercises the Ginkgo tests under the `//test/manifest` and `//migrations` paths of the `pivotal/tas` repos (where `` is tas, ist, or tasw). -Running these tests requires a docker daemon. It also requires the user to -provide Artifactory credentials via the ARTIFACTORY_USERNAME and -ARTIFACTORY_PASSWORD environment variables to allow the ops-manifest gem to -be installed. The credentials must have access to the `tas-rel-eng-gem-dev-local` -repository within Broadcom's Artifactory. +Running these tests requires a Docker daemon (or Podman API-compatible socket). You must provide **ARTIFACTORY_USERNAME** and **ARTIFACTORY_PASSWORD** using **`-e`** and/or **exported** environment variables. They are used for the **ops-manifest** gem (`tas-rel-eng-gem-dev-local`), for **Go module** downloads during **`go install ginkgo`** (via **`GOPROXY`** / **`GOSUMDB=off`** in the embedded Dockerfile), and Kiln sends the same credentials to the daemon as **registry auth** so base images can be pulled from **docker-virtual** (`tas-rel-eng-docker-virtual.usw1.packages.broadcom.com`) **without a separate `docker login`** for `kiln test`. + +The embedded Dockerfile pins **`FROM`** paths on that registry. The registry hostname in Kiln’s **`AuthConfigs`** must stay aligned with those **`FROM`** lines (see **`DockerVirtualRegistryHost`** in `internal/test/container.go`). Passwords with characters that are special in URLs may not behave the same as URL-encoded credentials when interpolated into **`GOPROXY`** inside the Dockerfile. + +If either credential is missing, `kiln test` exits with an error before talking to Docker. If you run into this docker error `could not execute "test": failed to connect to Docker daemon: Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running`, then create a symlink `sudo ln -s ~/.docker/run/docker.sock /var/run/docker.sock` -Here are command line examples: +Examples: ``` $ cd ~/workspace/tas/ist @@ -539,37 +539,46 @@ $ kiln test -e ARTIFACTORY_USERNAME=myuser -e ARTIFACTORY_PASSWORD=secretpasswor ``` ``` -cd ~ -$ kiln test --verbose -tp ~/workspace/tas/ist --ginkgo-manifest-flags "-p -nodes 8 -v" +$ export ARTIFACTORY_USERNAME=myuser +$ export ARTIFACTORY_PASSWORD=secretpassword +$ kiln test --verbose -tp ~/workspace/tas/ist --ginkgo-flags "-p -nodes 8 -v" ```
Additional test options -##### `--ginkgo-manifest-flags` +##### `--ginkgo-flags` + +The `--ginkgo-flags` flag can be used to pass through Ginkgo test flags. The defaults being passed through are `-r -p -slowSpecThreshold 15`. Pass `help` as a flag to retrieve the available options for the embedded version of ginkgo. -The `--ginkgo-manifest-flags` flag can be used to pass through Ginkgo test flags. The defaults being passed through are `-r -p -slowSpecThreshold 15`. Pass `help` as a flag to retrieve the available options for the embeded version of ginkgo. +#### `--manifest` -#### `--manifest-only` +The `--manifest` flag can be used to run only Manifest tests. -The `--manifest-only` flag can be used to run only Manifest tests. If not passed, `kiln test` will run both Manifest and Migration tests by default. +#### `--migrations` -#### `--migrations-only` +The `--migrations` flag can be used to run only Migration tests. -The `--migrations-only` flag can be used to run only Migration tests. If not passed, `kiln test` will run both Manifest and Migration tests by default. +#### `--stability` + +The `--stability` flag can be used to run only Stability tests. ##### `--tile-path` -The `--tile-path` (`-tp`) flag can be set the path the directory you wish to test. It defaults to the current working directory. For example +The `--tile-path` (`-tp`) flag can be set to the directory you wish to test. It defaults to the current working directory. For example: ``` -$ kiln test -tp ~/workspace/tas/ist +$ kiln test -e ARTIFACTORY_USERNAME=myuser -e ARTIFACTORY_PASSWORD=secret -tp ~/workspace/tas/ist ``` ##### `--verbose` The `--verbose` (`-v`) flag will log additional debugging info. +##### `--silent` + +The `--silent` (`-s`) flag hides Kiln info lines (not Ginkgo output). +
### `fetch` diff --git a/internal/commands/test_tile.go b/internal/commands/test_tile.go index 2acbe3289..8b9063c05 100644 --- a/internal/commands/test_tile.go +++ b/internal/commands/test_tile.go @@ -17,14 +17,14 @@ type TileTestFunction func(ctx context.Context, w io.Writer, configuration test. type TileTest struct { Options struct { - TilePath string ` long:"tile-path" default:"." description:"Path to the Tile directory (e.g., ~/workspace/tas/ist)."` + TilePath string ` long:"tile-path" default:"." description:"Path to the Tile directory (e.g. ~/workspace/tas/ist)."` Verbose bool `short:"v" long:"verbose" default:"true" description:"Print info lines. This doesn't affect Ginkgo output."` Silent bool `short:"s" long:"silent" default:"false" description:"Hide info lines. This doesn't affect Ginkgo output."` Manifest bool ` long:"manifest" default:"false" description:"Focus the Manifest tests."` Migrations bool ` long:"migrations" default:"false" description:"Focus the Migration tests."` Stability bool ` long:"stability" default:"false" description:"Focus the Stability tests."` - EnvironmentVars []string `short:"e" long:"environment-variable" description:"Pass environment variable to the test suites. For example --stability -e 'PRODUCT=srt'."` + EnvironmentVars []string `short:"e" long:"environment-variable" description:"Pass environment variables to the test suites (e.g. -e 'PRODUCT=srt'). Include -e ARTIFACTORY_USERNAME=... and -e ARTIFACTORY_PASSWORD=... unless they are exported."` GingkoFlags string ` long:"ginkgo-flags" default:"-r -p -slowSpecThreshold 15" description:"Flags to pass to the Ginkgo Manifest and Stability test suites."` } function TileTestFunction @@ -64,7 +64,10 @@ func (cmd TileTest) configuration() (test.Configuration, error) { if _, err := os.Stat(absPath); err != nil { return test.Configuration{}, fmt.Errorf("failed to get information about --tile-path: %w", err) } - return test.Configuration{ + if _, _, err := test.RequiredArtifactoryCredentials(cmd.Options.EnvironmentVars); err != nil { + return test.Configuration{}, err + } + cfg := test.Configuration{ AbsoluteTileDirectory: absPath, RunAll: !cmd.Options.Migrations && !cmd.Options.Manifest && !cmd.Options.Stability, @@ -74,12 +77,13 @@ func (cmd TileTest) configuration() (test.Configuration, error) { GinkgoFlags: cmd.Options.GingkoFlags, Environment: cmd.Options.EnvironmentVars, - }, absErr + } + return cfg, absErr } func (cmd TileTest) Usage() jhanda.Usage { return jhanda.Usage{ - Description: "Run the Manifest, Migrations, and Stability tests for a Tile in a Docker container. Requires a Docker daemon to be running and Artifactory credentials to be provided via the ARTIFACTORY_USERNAME and ARTIFACTORY_PASSWORD environment variables to install the ops-manifest gem.", + Description: "Run the Manifest, Migrations, and Stability tests for a Tile in a Docker container. Requires a Docker daemon. Requires ARTIFACTORY_USERNAME and ARTIFACTORY_PASSWORD (via -e or your environment) for the test image build and ops-manifest gem. Kiln passes the same credentials to the Docker daemon for pulling base images from docker-virtual (no separate docker login needed for kiln test).", ShortDescription: "Runs unit tests for a Tile.", Flags: cmd.Options, } diff --git a/internal/commands/test_tile_artifactory_test.go b/internal/commands/test_tile_artifactory_test.go new file mode 100644 index 000000000..d763be1fb --- /dev/null +++ b/internal/commands/test_tile_artifactory_test.go @@ -0,0 +1,58 @@ +package commands + +import ( + "context" + "io" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/pivotal-cf/kiln/internal/test" +) + +func TestTileTest_RequiresArtifactoryCredentials(t *testing.T) { + t.Setenv("ARTIFACTORY_USERNAME", "") + t.Setenv("ARTIFACTORY_PASSWORD", "") + + err := NewTileTest().Execute([]string{}) + require.Error(t, err) + require.ErrorContains(t, err, "ARTIFACTORY_USERNAME") + require.ErrorContains(t, err, "kiln test") +} + +func TestTileTest_RequiresArtifactoryPassword(t *testing.T) { + t.Setenv("ARTIFACTORY_PASSWORD", "") + + err := NewTileTest().Execute([]string{"-e", "ARTIFACTORY_USERNAME=onlyuser"}) + require.Error(t, err) + require.ErrorContains(t, err, "ARTIFACTORY_PASSWORD") +} + +func TestTileTest_PassesArtifactoryViaEnvironmentToConfiguration(t *testing.T) { + var captured test.Configuration + stub := func(_ context.Context, _ io.Writer, c test.Configuration) error { + captured = c + return nil + } + err := NewTileTestWithCollaborators(io.Discard, stub).Execute([]string{ + "-e", "ARTIFACTORY_USERNAME=u", + "-e", "ARTIFACTORY_PASSWORD=p", + }) + require.NoError(t, err) + require.Contains(t, captured.Environment, "ARTIFACTORY_USERNAME=u") + require.Contains(t, captured.Environment, "ARTIFACTORY_PASSWORD=p") +} + +func TestTileTest_UsesProcessEnvArtifactoryCredentials(t *testing.T) { + t.Setenv("ARTIFACTORY_USERNAME", "fromenv") + t.Setenv("ARTIFACTORY_PASSWORD", "frompass") + + var captured test.Configuration + stub := func(_ context.Context, _ io.Writer, c test.Configuration) error { + captured = c + return nil + } + err := NewTileTestWithCollaborators(io.Discard, stub).Execute([]string{}) + require.NoError(t, err) + require.Empty(t, captured.Environment) +} diff --git a/internal/commands/test_tile_test.go b/internal/commands/test_tile_test.go index f04a93472..c456afd52 100644 --- a/internal/commands/test_tile_test.go +++ b/internal/commands/test_tile_test.go @@ -34,6 +34,12 @@ func init() { var _ = Describe("kiln test", func() { var output bytes.Buffer + BeforeEach(func() { + t := GinkgoT() + t.Setenv("ARTIFACTORY_USERNAME", "ginkgo-test-user") + t.Setenv("ARTIFACTORY_PASSWORD", "ginkgo-test-pass") + }) + AfterEach(func() { output.Reset() }) @@ -210,9 +216,9 @@ var _ = Describe("kiln test", func() { }) }) - When("when the stability test is enabled", func() { - It("it sets the RunMetadata configuration flag", func() { - args := []string{"--stability"} + When("when ginkgo/v2 flag arguments are passed", func() { + It("it sets the GinkgoFlags configuration", func() { + args := []string{"--ginkgo-flags=peach pair"} fakeTestFunc := fakes.TestTileFunction{} fakeTestFunc.Returns(nil) @@ -226,15 +232,16 @@ var _ = Describe("kiln test", func() { Expect(ctx).NotTo(BeNil()) Expect(w).NotTo(BeNil()) - Expect(configuration.RunManifest).To(BeFalse()) - Expect(configuration.RunMetadata).To(BeTrue()) - Expect(configuration.RunMigrations).To(BeFalse()) + Expect(configuration.GinkgoFlags).To(Equal("peach pair")) }) }) - When("when ginkgo/v2 flag arguments are passed", func() { - It("it sets the GinkgoFlags configuration", func() { - args := []string{"--ginkgo-flags=peach pair"} + When("when Artifactory credentials are provided via -e", func() { + It("invokes the test function with those variables in Environment", func() { + args := []string{ + "-e", "ARTIFACTORY_USERNAME=u", + "-e", "ARTIFACTORY_PASSWORD=p", + } fakeTestFunc := fakes.TestTileFunction{} fakeTestFunc.Returns(nil) @@ -242,13 +249,38 @@ var _ = Describe("kiln test", func() { err := commands.NewTileTestWithCollaborators(&output, fakeTestFunc.Spy).Execute(args) Expect(err).NotTo(HaveOccurred()) - Expect(fakeTestFunc.CallCount()).To(Equal(1)) + _, _, configuration := fakeTestFunc.ArgsForCall(0) + Expect(configuration.Environment).To(ContainElement("ARTIFACTORY_USERNAME=u")) + Expect(configuration.Environment).To(ContainElement("ARTIFACTORY_PASSWORD=p")) + }) + }) - ctx, w, configuration := fakeTestFunc.ArgsForCall(0) - Expect(ctx).NotTo(BeNil()) - Expect(w).NotTo(BeNil()) + When("when Artifactory credentials are missing", func() { + It("returns an error before invoking the test function", func() { + savedU, hasU := os.LookupEnv("ARTIFACTORY_USERNAME") + savedP, hasP := os.LookupEnv("ARTIFACTORY_PASSWORD") + DeferCleanup(func() { + if hasU { + Expect(os.Setenv("ARTIFACTORY_USERNAME", savedU)).To(Succeed()) + } else { + Expect(os.Unsetenv("ARTIFACTORY_USERNAME")).To(Succeed()) + } + if hasP { + Expect(os.Setenv("ARTIFACTORY_PASSWORD", savedP)).To(Succeed()) + } else { + Expect(os.Unsetenv("ARTIFACTORY_PASSWORD")).To(Succeed()) + } + }) + Expect(os.Unsetenv("ARTIFACTORY_USERNAME")).To(Succeed()) + Expect(os.Unsetenv("ARTIFACTORY_PASSWORD")).To(Succeed()) - Expect(configuration.GinkgoFlags).To(Equal("peach pair")) + fakeTestFunc := fakes.TestTileFunction{} + fakeTestFunc.Returns(nil) + + err := commands.NewTileTestWithCollaborators(&output, fakeTestFunc.Spy).Execute([]string{}) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("ARTIFACTORY_USERNAME")) + Expect(fakeTestFunc.CallCount()).To(Equal(0)) }) }) diff --git a/internal/test/Dockerfile b/internal/test/Dockerfile index 42987b73c..bc022594f 100644 --- a/internal/test/Dockerfile +++ b/internal/test/Dockerfile @@ -1,10 +1,11 @@ -FROM golang AS go-image -FROM docker.io/pivotalcfreleng/kiln:v0.110.0-rc2 AS kiln +# Base images: host must match DockerVirtualRegistryHost in internal/test/container.go (AuthConfigs). +FROM tas-rel-eng-docker-virtual.usw1.packages.broadcom.com/golang AS go-image +FROM tas-rel-eng-docker-virtual.usw1.packages.broadcom.com/pivotalcfreleng/kiln:v0.110.0-rc2 AS kiln -FROM ruby:3.4.8 AS builder +FROM tas-rel-eng-docker-virtual.usw1.packages.broadcom.com/ruby:3.4.8 AS builder RUN mkdir -p -m 0700 ~/.ssh && ssh-keyscan github.com >> ~/.ssh/known_hosts -FROM ruby:3.4.8 +FROM tas-rel-eng-docker-virtual.usw1.packages.broadcom.com/ruby:3.4.8 # Install Go COPY --from=go-image /usr/local/go/ /usr/local/go/ @@ -17,19 +18,24 @@ COPY --from=kiln /kiln /usr/local/bin/kiln # Install JQ RUN apt-get update && apt-get install jq -y -# Install Ginkgo -RUN go install github.com/onsi/ginkgo/ginkgo@latest - -# Install NodeJS -RUN apt-get update && apt-get install nodejs npm -y - -# Install OpsManifest from Artifactory ARG ARTIFACTORY_USERNAME ARG ARTIFACTORY_PASSWORD - ENV ARTIFACTORY_USERNAME=${ARTIFACTORY_USERNAME} ENV ARTIFACTORY_PASSWORD=${ARTIFACTORY_PASSWORD} +# Install Ginkgo (module download via Artifactory Go virtual; credentials from build args). +ARG GOTOOLCHAIN=local +ENV GOTOOLCHAIN=${GOTOOLCHAIN} +ENV GOPROXY=https://${ARTIFACTORY_USERNAME}:${ARTIFACTORY_PASSWORD}@usw1.packages.broadcom.com/artifactory/api/go/tas-rel-eng-go-virtual +ENV GOSUMDB=off +# Pure-Go install; avoid invoking gcc. On linux/arm64 (e.g. Podman on Apple Silicon), +# cgo can fail with: gcc: error: unrecognized command-line option '-m64'. +ENV CGO_ENABLED=0 +RUN go install github.com/onsi/ginkgo/ginkgo@latest + +# Install NodeJS +RUN apt-get update && apt-get install nodejs npm -y + RUN gem source -a https://${ARTIFACTORY_USERNAME}:${ARTIFACTORY_PASSWORD}@usw1.packages.broadcom.com/artifactory/api/gems/tas-rel-eng-gem-dev-local/ RUN gem install --verbose ops-manifest -v 0.0.4.pre diff --git a/internal/test/container.go b/internal/test/container.go index 5b3120ed1..f21e6b392 100644 --- a/internal/test/container.go +++ b/internal/test/container.go @@ -22,6 +22,7 @@ import ( "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/mount" "github.com/docker/docker/api/types/network" + "github.com/docker/docker/api/types/registry" "github.com/docker/docker/client" specV1 "github.com/opencontainers/image-spec/specs-go/v1" "golang.org/x/sync/errgroup" @@ -32,6 +33,10 @@ const ( // If the integration tests pass on your machine with an older version, feel free to PR a less conservative value. MinimumDockerServerVersion = "> 24.0.0" MinimumPodmanServerVersion = "> 5.3.0" + + // DockerVirtualRegistryHost is the docker-virtual registry used in Dockerfile FROM lines + // and in ImageBuild AuthConfigs. Keep in sync with internal/test/Dockerfile. + DockerVirtualRegistryHost = "tas-rel-eng-docker-virtual.usw1.packages.broadcom.com" ) func Run(ctx context.Context, w io.Writer, configuration Configuration) error { @@ -119,24 +124,36 @@ func runTest(ctx context.Context, logger *log.Logger, w io.Writer, dockerDaemon return fmt.Errorf("failed to parse environment: %w", err) } - artifactoryUsername := envMap["ARTIFACTORY_USERNAME"] - artifactoryPassword := envMap["ARTIFACTORY_PASSWORD"] + username, password, err := RequiredArtifactoryCredentials(configuration.Environment) + if err != nil { + return err + } + envMap["ARTIFACTORY_USERNAME"] = username + envMap["ARTIFACTORY_PASSWORD"] = password + + artifactoryUsername := username + artifactoryPassword := password + + authConfigs := registryAuthForDockerVirtual(envMap) logger.Println("creating test image") + buildArgs := map[string]*string{ + "ARTIFACTORY_USERNAME": &artifactoryUsername, + "ARTIFACTORY_PASSWORD": &artifactoryPassword, + } + resp, err := dockerDaemon.ImageBuild(ctx, &dockerfileTarball, build.ImageBuildOptions{ - Tags: []string{"kiln_test_dependencies:vmware"}, - BuildArgs: map[string]*string{ - "ARTIFACTORY_USERNAME": &artifactoryUsername, - "ARTIFACTORY_PASSWORD": &artifactoryPassword, - }, + Tags: []string{"kiln_test_dependencies:vmware"}, + BuildArgs: buildArgs, + AuthConfigs: authConfigs, + SuppressOutput: true, }) if err != nil { return fmt.Errorf("failed to build image: %w", err) } - logger.Println("reading image build response") - if err := checkImageBuildResponse(resp.Body); err != nil { + if err := checkImageBuildResponse(resp.Body, nil); err != nil { return fmt.Errorf("image build failed: %w", err) } @@ -197,6 +214,12 @@ func runTest(ctx context.Context, logger *log.Logger, w io.Writer, dockerDaemon return fmt.Errorf("failed to start test container: %w", err) } + // Subscribe for exit before draining logs. With AutoRemove, the engine may delete the + // container as soon as it stops; waiting for removal after io.Copy can race and + // return "no such container" (often under Podman). next-exit records the exit while + // the ID still exists. + statusCh, containerWaitError := dockerDaemon.ContainerWait(ctx, testContainer.ID, container.WaitConditionNextExit) + out, err := dockerDaemon.ContainerLogs(ctx, testContainer.ID, container.LogsOptions{ShowStdout: true, ShowStderr: true, Follow: true}) if err != nil { return fmt.Errorf("container log request failure: %w", err) @@ -205,10 +228,7 @@ func runTest(ctx context.Context, logger *log.Logger, w io.Writer, dockerDaemon return err } - //Although the fan-in loop pattern seems like the right solution here, ContainerWait - //does not properly close channels, so it won't work. var resultErr error - statusCh, containerWaitError := dockerDaemon.ContainerWait(ctx, testContainer.ID, container.WaitConditionRemoved) select { case err := <-containerWaitError: resultErr = err @@ -237,6 +257,48 @@ func encodeEnvironment(m environmentVars) []string { return result } +// RequiredArtifactoryCredentials resolves ARTIFACTORY_USERNAME and ARTIFACTORY_PASSWORD +// from -e flags and os.Getenv for kiln test. +func RequiredArtifactoryCredentials(envVarArgs []string) (username, password string, err error) { + m, err := decodeEnvironment(envVarArgs) + if err != nil { + return "", "", err + } + user := strings.TrimSpace(m["ARTIFACTORY_USERNAME"]) + if user == "" { + user = strings.TrimSpace(os.Getenv("ARTIFACTORY_USERNAME")) + } + pass := strings.TrimSpace(m["ARTIFACTORY_PASSWORD"]) + if pass == "" { + pass = strings.TrimSpace(os.Getenv("ARTIFACTORY_PASSWORD")) + } + if user == "" { + return "", "", fmt.Errorf("kiln test requires ARTIFACTORY_USERNAME: set it using -e or export it in your environment") + } + if pass == "" { + return "", "", fmt.Errorf("kiln test requires ARTIFACTORY_PASSWORD: set it using -e or export it in your environment") + } + return user, pass, nil +} + +// registryAuthForDockerVirtual supplies credentials for pulling FROM images on +// DockerVirtualRegistryHost during docker build (X-Registry-Config). +func registryAuthForDockerVirtual(env environmentVars) map[string]registry.AuthConfig { + user := strings.TrimSpace(env["ARTIFACTORY_USERNAME"]) + pass := strings.TrimSpace(env["ARTIFACTORY_PASSWORD"]) + if user == "" || pass == "" { + return nil + } + host := DockerVirtualRegistryHost + return map[string]registry.AuthConfig{ + host: { + Username: user, + Password: pass, + ServerAddress: host, + }, + } +} + func decodeEnvironment(environmentVarArgs []string) (environmentVars, error) { envMap := make(environmentVars) for _, envVar := range environmentVarArgs { @@ -296,13 +358,17 @@ type tarWriter interface { } type imageBuildMessage struct { + Stream string `json:"stream"` Error string `json:"error"` ErrorDetail struct { Message string `json:"message"` } `json:"errorDetail"` } -func checkImageBuildResponse(body io.ReadCloser) error { +// checkImageBuildResponse reads the Docker/Podman image-build JSON stream. If +// logOutput is non-nil, "stream" lines are copied there; otherwise they are +// discarded. Build failures are still returned from daemon "error" messages. +func checkImageBuildResponse(body io.ReadCloser, logOutput io.Writer) error { defer func() { _ = body.Close() }() @@ -315,8 +381,15 @@ func checkImageBuildResponse(body io.ReadCloser) error { } return fmt.Errorf("failed to read image build response: %w", err) } + if logOutput != nil && msg.Stream != "" { + _, _ = io.WriteString(logOutput, msg.Stream) + } if msg.Error != "" { - return fmt.Errorf("%s", msg.Error) + detail := msg.Error + if msg.ErrorDetail.Message != "" { + detail = msg.ErrorDetail.Message + } + return fmt.Errorf("%s", detail) } } return nil diff --git a/internal/test/container_test.go b/internal/test/container_test.go index 39172e7b6..d26aa3d5e 100644 --- a/internal/test/container_test.go +++ b/internal/test/container_test.go @@ -1,8 +1,11 @@ package test import ( + "bytes" + "io" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/require" @@ -79,6 +82,84 @@ func TestConfiguration_commands(t *testing.T) { } } +func Test_checkImageBuildResponse(t *testing.T) { + t.Run("streams build log then error", func(t *testing.T) { + body := io.NopCloser(strings.NewReader( + `{"stream":"Step 1\n"}` + "\n" + + `{"stream":"go: downloading\n"}` + "\n" + + `{"error":"failed","errorDetail":{"message":"go install: nope"}}` + "\n", + )) + var buf bytes.Buffer + err := checkImageBuildResponse(body, &buf) + require.ErrorContains(t, err, "go install: nope") + require.Contains(t, buf.String(), "Step 1") + require.Contains(t, buf.String(), "go: downloading") + }) +} + +func TestEmbeddedDockerfile_usesDockerVirtualRegistry(t *testing.T) { + require.Contains(t, dockerfile, "FROM "+DockerVirtualRegistryHost+"/golang") + require.Contains(t, dockerfile, "FROM "+DockerVirtualRegistryHost+"/ruby:3.4.8") + require.NotContains(t, dockerfile, "REGISTRY_PREFIX") + require.Contains(t, dockerfile, "ENV GOPROXY=https://${ARTIFACTORY_USERNAME}:${ARTIFACTORY_PASSWORD}@usw1.packages.broadcom.com/artifactory/api/go/tas-rel-eng-go-virtual") + require.Contains(t, dockerfile, "ENV GOSUMDB=off") +} + +func Test_registryAuthForDockerVirtual(t *testing.T) { + t.Run("nil when username missing", func(t *testing.T) { + require.Nil(t, registryAuthForDockerVirtual(environmentVars{"ARTIFACTORY_PASSWORD": "p"})) + }) + t.Run("nil when password missing", func(t *testing.T) { + require.Nil(t, registryAuthForDockerVirtual(environmentVars{"ARTIFACTORY_USERNAME": "u"})) + }) + t.Run("returns auth for docker virtual host", func(t *testing.T) { + got := registryAuthForDockerVirtual(environmentVars{ + "ARTIFACTORY_USERNAME": "alice", + "ARTIFACTORY_PASSWORD": "secret", + }) + require.Len(t, got, 1) + cfg := got[DockerVirtualRegistryHost] + require.Equal(t, "alice", cfg.Username) + require.Equal(t, "secret", cfg.Password) + require.Equal(t, DockerVirtualRegistryHost, cfg.ServerAddress) + }) +} + +func Test_RequiredArtifactoryCredentials(t *testing.T) { + t.Run("from -e only", func(t *testing.T) { + t.Setenv("ARTIFACTORY_USERNAME", "") + t.Setenv("ARTIFACTORY_PASSWORD", "") + u, p, err := RequiredArtifactoryCredentials([]string{"ARTIFACTORY_USERNAME=a", "ARTIFACTORY_PASSWORD=b"}) + require.NoError(t, err) + require.Equal(t, "a", u) + require.Equal(t, "b", p) + }) + t.Run("-e overrides process env", func(t *testing.T) { + t.Setenv("ARTIFACTORY_USERNAME", "envuser") + t.Setenv("ARTIFACTORY_PASSWORD", "envpass") + u, p, err := RequiredArtifactoryCredentials([]string{"ARTIFACTORY_USERNAME=fromflag", "ARTIFACTORY_PASSWORD=frompass"}) + require.NoError(t, err) + require.Equal(t, "fromflag", u) + require.Equal(t, "frompass", p) + }) + t.Run("missing username", func(t *testing.T) { + t.Setenv("ARTIFACTORY_USERNAME", "") + _, _, err := RequiredArtifactoryCredentials([]string{"ARTIFACTORY_PASSWORD=only"}) + require.ErrorContains(t, err, "ARTIFACTORY_USERNAME") + require.ErrorContains(t, err, "kiln test") + }) + t.Run("missing password", func(t *testing.T) { + t.Setenv("ARTIFACTORY_PASSWORD", "") + _, _, err := RequiredArtifactoryCredentials([]string{"ARTIFACTORY_USERNAME=only"}) + require.ErrorContains(t, err, "ARTIFACTORY_PASSWORD") + require.ErrorContains(t, err, "kiln test") + }) + t.Run("invalid env pair", func(t *testing.T) { + _, _, err := RequiredArtifactoryCredentials([]string{"notakeyval"}) + require.Error(t, err) + }) +} + func Test_decodeEnvironment(t *testing.T) { for _, tt := range []struct { Name string From 9d9fac241c907b55128515ec06e09be06b653fd1 Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Tue, 5 May 2026 12:15:44 -0500 Subject: [PATCH 02/18] Improve kiln test UX: Dockerfile caching, suite headers, GOMAXPROCS Restructure Dockerfile so stable tool layers (jq, node, npm) precede credential-dependent steps; scopes GOPROXY to the ginkgo RUN command to avoid cache-busting on credential rotation; pins ginkgo to v1.16.5 for reproducibility; sets CGO_ENABLED=0 to avoid gcc issues on arm64. Run each test suite (migration, stability, manifest) as a separate shell invocation so output is never interleaved. Add printed suite headers for migration and stability suites that do not emit their own Ginkgo header. Prefer npm ci when package-lock.json exists, and suppress audit/fund noise with --no-audit --no-fund otherwise. Expose GOMAXPROCS in the container to utilise all available cores. Remove log.Logger in favour of plain fmt.Fprintln progress lines, and show the container ID only when --verbose is set. ai-assisted=yes Co-authored-by: Cursor --- internal/commands/test_tile.go | 4 +- internal/test/Dockerfile | 40 ++++++++------ internal/test/container.go | 62 ++++++++++++++------- internal/test/container_test.go | 95 ++++++++++++++++++++++++++++----- 4 files changed, 151 insertions(+), 50 deletions(-) diff --git a/internal/commands/test_tile.go b/internal/commands/test_tile.go index 8b9063c05..ea9410d87 100644 --- a/internal/commands/test_tile.go +++ b/internal/commands/test_tile.go @@ -2,7 +2,6 @@ package commands import ( "context" - _ "embed" "fmt" "io" "os" @@ -18,7 +17,7 @@ type TileTestFunction func(ctx context.Context, w io.Writer, configuration test. type TileTest struct { Options struct { TilePath string ` long:"tile-path" default:"." description:"Path to the Tile directory (e.g. ~/workspace/tas/ist)."` - Verbose bool `short:"v" long:"verbose" default:"true" description:"Print info lines. This doesn't affect Ginkgo output."` + Verbose bool `short:"v" long:"verbose" default:"true" description:"Print extra details such as the container ID. This doesn't affect Ginkgo output."` Silent bool `short:"s" long:"silent" default:"false" description:"Hide info lines. This doesn't affect Ginkgo output."` Manifest bool ` long:"manifest" default:"false" description:"Focus the Manifest tests."` Migrations bool ` long:"migrations" default:"false" description:"Focus the Migration tests."` @@ -77,6 +76,7 @@ func (cmd TileTest) configuration() (test.Configuration, error) { GinkgoFlags: cmd.Options.GingkoFlags, Environment: cmd.Options.EnvironmentVars, + Verbose: cmd.Options.Verbose, } return cfg, absErr } diff --git a/internal/test/Dockerfile b/internal/test/Dockerfile index bc022594f..cf1f57ecd 100644 --- a/internal/test/Dockerfile +++ b/internal/test/Dockerfile @@ -7,34 +7,40 @@ RUN mkdir -p -m 0700 ~/.ssh && ssh-keyscan github.com >> ~/.ssh/known_hosts FROM tas-rel-eng-docker-virtual.usw1.packages.broadcom.com/ruby:3.4.8 -# Install Go +# ── Stable tools — no credentials; these layers cache across credential rotation ── + +# Go runtime COPY --from=go-image /usr/local/go/ /usr/local/go/ ENV GOROOT=/usr/local/go/ ENV PATH="$GOROOT/bin:/root/go/bin:$PATH" -# Install Kiln +# Kiln binary (used by ops-manifest during manifest tests) COPY --from=kiln /kiln /usr/local/bin/kiln -# Install JQ -RUN apt-get update && apt-get install jq -y +# System packages (consolidated to one layer; before credentials so they stay cached) +RUN apt-get update \ + && apt-get install --no-install-recommends -y jq nodejs npm \ + && rm -rf /var/lib/apt/lists/* + +# Go toolchain settings — must appear before any `go install` +# CGO_ENABLED=0: pure-Go build avoids gcc issues on arm64 (e.g. Podman on Apple Silicon). +ENV GOTOOLCHAIN=local +ENV CGO_ENABLED=0 + +# ── Ginkgo — credentials scoped to this RUN only so layers above stay cached ── +# Pinned to v1.16.5 for reproducibility (last stable v1; tiles using ginkgo v2 use their own binary). ARG ARTIFACTORY_USERNAME ARG ARTIFACTORY_PASSWORD -ENV ARTIFACTORY_USERNAME=${ARTIFACTORY_USERNAME} -ENV ARTIFACTORY_PASSWORD=${ARTIFACTORY_PASSWORD} -# Install Ginkgo (module download via Artifactory Go virtual; credentials from build args). -ARG GOTOOLCHAIN=local -ENV GOTOOLCHAIN=${GOTOOLCHAIN} -ENV GOPROXY=https://${ARTIFACTORY_USERNAME}:${ARTIFACTORY_PASSWORD}@usw1.packages.broadcom.com/artifactory/api/go/tas-rel-eng-go-virtual -ENV GOSUMDB=off -# Pure-Go install; avoid invoking gcc. On linux/arm64 (e.g. Podman on Apple Silicon), -# cgo can fail with: gcc: error: unrecognized command-line option '-m64'. -ENV CGO_ENABLED=0 -RUN go install github.com/onsi/ginkgo/ginkgo@latest +RUN GOPROXY=https://${ARTIFACTORY_USERNAME}:${ARTIFACTORY_PASSWORD}@usw1.packages.broadcom.com/artifactory/api/go/tas-rel-eng-go-virtual \ + GOSUMDB=off \ + go install github.com/onsi/ginkgo/ginkgo@v1.16.5 + +# ── ops-manifest gem — credentials exported for gem source registration at build time ── -# Install NodeJS -RUN apt-get update && apt-get install nodejs npm -y +ENV ARTIFACTORY_USERNAME=${ARTIFACTORY_USERNAME} +ENV ARTIFACTORY_PASSWORD=${ARTIFACTORY_PASSWORD} RUN gem source -a https://${ARTIFACTORY_USERNAME}:${ARTIFACTORY_PASSWORD}@usw1.packages.broadcom.com/artifactory/api/gems/tas-rel-eng-gem-dev-local/ RUN gem install --verbose ops-manifest -v 0.0.4.pre diff --git a/internal/test/container.go b/internal/test/container.go index f21e6b392..8616d3ab5 100644 --- a/internal/test/container.go +++ b/internal/test/container.go @@ -9,11 +9,12 @@ import ( "errors" "fmt" "io" - "log" "os" "os/signal" "path" "path/filepath" + "runtime" + "strconv" "strings" cerrdefs "github.com/containerd/errdefs" @@ -40,14 +41,12 @@ const ( ) func Run(ctx context.Context, w io.Writer, configuration Configuration) error { - logger := log.New(w, "kiln test: ", log.Default().Flags()) - dockerDaemon, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation()) if err != nil { return err } - return runTest(ctx, logger, w, dockerDaemon, configuration) + return runTest(ctx, w, dockerDaemon, configuration) } type Configuration struct { @@ -62,6 +61,13 @@ type Configuration struct { GinkgoFlags string Environment []string + Verbose bool +} + +// suiteHeader builds the "Running Suite: …\n…\n" printf command for a suite. +func suiteHeader(title string) string { + label := "Running Suite: " + title + return fmt.Sprintf(`printf '\n%s\n%s\n'`, label, strings.Repeat("=", len(label))) } func (configuration Configuration) commands() ([]string, error) { @@ -73,21 +79,28 @@ func (configuration Configuration) commands() ([]string, error) { commands := []string{"git config --global --add safe.directory '*'"} if configuration.RunMigrations || configuration.RunAll { commands = append(commands, fmt.Sprintf("cd /tas/%s/migrations", tileDirName)) - commands = append(commands, "npm install") + commands = append(commands, npmInstallCommand(configuration.AbsoluteTileDirectory)) + commands = append(commands, suiteHeader("Migration Tests")) commands = append(commands, "npm test") } - var ginkgo []string + + // Each suite gets its own invocation so output is never interleaved. + // Stability tests use Go's standard testing package (no ginkgo bootstrap), so + // ginkgo does not print "Running Suite: ..."; we add the header ourselves. + // Manifest suites use ginkgo specs and print their own header — we only add a + // blank line. Note: not compatible with tiles that use ginkgo v2. if configuration.RunMetadata || configuration.RunAll { - ginkgo = append(ginkgo, fmt.Sprintf("/tas/%s/test/stability", tileDirName)) + stabilityPath := fmt.Sprintf("/tas/%s/test/stability", tileDirName) + commands = append(commands, suiteHeader("Stability Tests")) + commands = append(commands, fmt.Sprintf("cd /tas/%s && ginkgo %s %s", tileDirName, configuration.GinkgoFlags, stabilityPath)) } + if configuration.RunManifest || configuration.RunAll { - ginkgo = append(ginkgo, fmt.Sprintf("/tas/%s/test/manifest", tileDirName)) - } - // Note: this isn't compatible with tiles that use ginkgo v2 for their manifest tests - if configuration.RunMetadata || configuration.RunManifest || configuration.RunAll { - ginkgoCommand := fmt.Sprintf("cd /tas/%s && ginkgo %s %s", tileDirName, configuration.GinkgoFlags, strings.Join(ginkgo, " ")) - commands = append(commands, ginkgoCommand) + manifestPath := fmt.Sprintf("/tas/%s/test/manifest", tileDirName) + commands = append(commands, `printf '\n'`) + commands = append(commands, fmt.Sprintf("cd /tas/%s && ginkgo %s %s", tileDirName, configuration.GinkgoFlags, manifestPath)) } + return commands, nil } @@ -102,8 +115,7 @@ type mobyClient interface { ContainerStop(ctx context.Context, containerID string, options container.StopOptions) error } -func runTest(ctx context.Context, logger *log.Logger, w io.Writer, dockerDaemon mobyClient, configuration Configuration) error { - logger.Printf("pinging docker daemon") +func runTest(ctx context.Context, w io.Writer, dockerDaemon mobyClient, configuration Configuration) error { _, err := dockerDaemon.Ping(ctx) if err != nil { return fmt.Errorf("failed to connect to Docker daemon: %w", err) @@ -136,7 +148,7 @@ func runTest(ctx context.Context, logger *log.Logger, w io.Writer, dockerDaemon authConfigs := registryAuthForDockerVirtual(envMap) - logger.Println("creating test image") + fmt.Fprintln(w, "Preparing test image...") buildArgs := map[string]*string{ "ARTIFACTORY_USERNAME": &artifactoryUsername, "ARTIFACTORY_PASSWORD": &artifactoryPassword, @@ -163,7 +175,7 @@ func runTest(ctx context.Context, logger *log.Logger, w io.Writer, dockerDaemon dockerCmd := strings.Join(commands, " && ") envVars := getTileTestEnvVars(configuration.AbsoluteTileDirectory, tileDir, envMap) - logger.Println("creating test container") + fmt.Fprintln(w, "Starting tests...") testContainer, err := dockerDaemon.ContainerCreate(ctx, &container.Config{ Image: "kiln_test_dependencies:vmware", Cmd: []string{"/bin/bash", "-c", dockerCmd}, @@ -187,7 +199,9 @@ func runTest(ctx context.Context, logger *log.Logger, w io.Writer, dockerDaemon if err != nil { return fmt.Errorf("failed to create container: %w", err) } - logger.Printf("created test container with id %s", testContainer.ID) + if configuration.Verbose { + fmt.Fprintf(w, "Container: %s\n", testContainer.ID) + } errG := errgroup.Group{} @@ -224,6 +238,7 @@ func runTest(ctx context.Context, logger *log.Logger, w io.Writer, dockerDaemon if err != nil { return fmt.Errorf("container log request failure: %w", err) } + fmt.Fprintln(w, "") if _, err := io.Copy(w, out); err != nil { return err } @@ -322,6 +337,16 @@ func toProduct(dir string) string { } } +// npmInstallCommand returns "npm ci" when a package-lock.json is present in the +// tile's migrations directory (faster, strict), otherwise "npm install --no-audit --no-fund". +func npmInstallCommand(absoluteTileDir string) string { + lockFile := filepath.Join(absoluteTileDir, "migrations", "package-lock.json") + if _, err := os.Stat(lockFile); err == nil { + return "npm ci" + } + return "npm install --no-audit --no-fund" +} + func getTileTestEnvVars(dir, productDir string, envMap environmentVars) environmentVars { const fixturesFormat = "%s/test/manifest/fixtures" metadataPath := fmt.Sprintf(fixturesFormat+"/tas_metadata.yml", dir) @@ -341,6 +366,7 @@ func getTileTestEnvVars(dir, productDir string, envMap environmentVars) environm envVarsMap["PRODUCT"] = toProduct(productDir) } envVarsMap["RENDERER"] = "ops-manifest" + envVarsMap["GOMAXPROCS"] = strconv.Itoa(runtime.NumCPU()) // overwrite with / include optional env vars for k, v := range envMap { diff --git a/internal/test/container_test.go b/internal/test/container_test.go index d26aa3d5e..ffcc7593b 100644 --- a/internal/test/container_test.go +++ b/internal/test/container_test.go @@ -20,7 +20,7 @@ func TestConfiguration_commands(t *testing.T) { for _, tt := range []struct { Name string Configuration Configuration - Result []string + ExpCmds []string ExpErrSubstring string }{ { @@ -35,7 +35,7 @@ func TestConfiguration_commands(t *testing.T) { Configuration: Configuration{ AbsoluteTileDirectory: absoluteTileDirectory, }, - Result: []string{"git config --global --add safe.directory '*'"}, + ExpCmds: []string{"git config --global --add safe.directory '*'"}, }, { Name: "when running migrations tests", @@ -43,7 +43,13 @@ func TestConfiguration_commands(t *testing.T) { AbsoluteTileDirectory: absoluteTileDirectory, RunMigrations: true, }, - Result: []string{"git config --global --add safe.directory '*'", "cd /tas/test/migrations", "npm install", "npm test"}, + ExpCmds: []string{ + "git config --global --add safe.directory '*'", + "cd /tas/test/migrations", + "npm install --no-audit --no-fund", + `printf '\nRunning Suite: Migration Tests\n==============================\n'`, + "npm test", + }, }, { Name: "when running manifest tests", @@ -51,7 +57,11 @@ func TestConfiguration_commands(t *testing.T) { AbsoluteTileDirectory: absoluteTileDirectory, RunManifest: true, }, - Result: []string{"git config --global --add safe.directory '*'", "cd /tas/test && ginkgo /tas/test/test/manifest"}, + ExpCmds: []string{ + "git config --global --add safe.directory '*'", + `printf '\n'`, + "cd /tas/test && ginkgo /tas/test/test/manifest", + }, }, { Name: "when running metadata tests", @@ -59,7 +69,11 @@ func TestConfiguration_commands(t *testing.T) { AbsoluteTileDirectory: absoluteTileDirectory, RunMetadata: true, }, - Result: []string{"git config --global --add safe.directory '*'", "cd /tas/test && ginkgo /tas/test/test/stability"}, + ExpCmds: []string{ + "git config --global --add safe.directory '*'", + `printf '\nRunning Suite: Stability Tests\n==============================\n'`, + "cd /tas/test && ginkgo /tas/test/test/stability", + }, }, { Name: "when running all tests", @@ -67,17 +81,27 @@ func TestConfiguration_commands(t *testing.T) { AbsoluteTileDirectory: absoluteTileDirectory, RunAll: true, }, - Result: []string{"git config --global --add safe.directory '*'", "cd /tas/test/migrations", "npm install", "npm test", "cd /tas/test && ginkgo /tas/test/test/stability /tas/test/test/manifest"}, + ExpCmds: []string{ + "git config --global --add safe.directory '*'", + "cd /tas/test/migrations", + "npm install --no-audit --no-fund", + `printf '\nRunning Suite: Migration Tests\n==============================\n'`, + "npm test", + `printf '\nRunning Suite: Stability Tests\n==============================\n'`, + "cd /tas/test && ginkgo /tas/test/test/stability", + `printf '\n'`, + "cd /tas/test && ginkgo /tas/test/test/manifest", + }, }, } { t.Run(tt.Name, func(t *testing.T) { - result, err := tt.Configuration.commands() + cmds, err := tt.Configuration.commands() if tt.ExpErrSubstring != "" { require.ErrorContains(t, err, tt.ExpErrSubstring) - } else { - require.NoError(t, err) - require.Equal(t, tt.Result, result) + return } + require.NoError(t, err) + require.Equal(t, tt.ExpCmds, cmds) }) } } @@ -97,12 +121,57 @@ func Test_checkImageBuildResponse(t *testing.T) { }) } -func TestEmbeddedDockerfile_usesDockerVirtualRegistry(t *testing.T) { +func TestEmbeddedDockerfile_structure(t *testing.T) { + // Base image FROM lines must use the internal docker-virtual registry. require.Contains(t, dockerfile, "FROM "+DockerVirtualRegistryHost+"/golang") require.Contains(t, dockerfile, "FROM "+DockerVirtualRegistryHost+"/ruby:3.4.8") require.NotContains(t, dockerfile, "REGISTRY_PREFIX") - require.Contains(t, dockerfile, "ENV GOPROXY=https://${ARTIFACTORY_USERNAME}:${ARTIFACTORY_PASSWORD}@usw1.packages.broadcom.com/artifactory/api/go/tas-rel-eng-go-virtual") - require.Contains(t, dockerfile, "ENV GOSUMDB=off") + + // ginkgo must be pinned to a specific version (not @latest) so builds are reproducible + // and the cache layer is stable. + require.Contains(t, dockerfile, "go install github.com/onsi/ginkgo/ginkgo@v1.16.5") + require.NotContains(t, dockerfile, "ginkgo@latest") + + // GOPROXY credentials must be scoped to the ginkgo RUN step only — not exported + // as an ENV layer — so the ginkgo install layer is not busted by credential rotation. + require.NotContains(t, dockerfile, "ENV GOPROXY=https://${ARTIFACTORY_USERNAME}") + + // Credentials ARG declaration must come AFTER stable system package installs + // (jq, nodejs, npm) so those layers stay cached when credentials rotate. + argIdx := strings.Index(dockerfile, "ARG ARTIFACTORY_USERNAME") + jqIdx := strings.Index(dockerfile, "apt-get") + require.Greater(t, argIdx, jqIdx, "ARTIFACTORY_USERNAME ARG should appear after apt-get installs") + + // Credentials must be exported to ENV for ops-manifest gem at container runtime. + require.Contains(t, dockerfile, "ENV ARTIFACTORY_USERNAME=${ARTIFACTORY_USERNAME}") + require.Contains(t, dockerfile, "ENV ARTIFACTORY_PASSWORD=${ARTIFACTORY_PASSWORD}") +} + +func TestConfiguration_commands_usesNpmCiWhenLockfilePresent(t *testing.T) { + tileDir := filepath.Join(t.TempDir(), "ist") + require.NoError(t, os.MkdirAll(filepath.Join(tileDir, "migrations"), 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(tileDir, "migrations", "package-lock.json"), []byte("{}"), 0o600)) + + cmds, err := Configuration{AbsoluteTileDirectory: tileDir, RunMigrations: true}.commands() + require.NoError(t, err) + require.Contains(t, cmds, "npm ci") +} + +func TestConfiguration_commands_usesNpmInstallWithoutLockfile(t *testing.T) { + tileDir := filepath.Join(t.TempDir(), "ist") + require.NoError(t, os.MkdirAll(filepath.Join(tileDir, "migrations"), 0o700)) + + cmds, err := Configuration{AbsoluteTileDirectory: tileDir, RunMigrations: true}.commands() + require.NoError(t, err) + require.Contains(t, cmds, "npm install --no-audit --no-fund") +} + +func TestGetTileTestEnvVars_setsGOMAXPROCS(t *testing.T) { + tileDir := filepath.Join(t.TempDir(), "ist") + envVars := getTileTestEnvVars(tileDir, "ist", environmentVars{}) + gomaxprocs, ok := envVars["GOMAXPROCS"] + require.True(t, ok, "GOMAXPROCS should be set in container env") + require.NotEmpty(t, gomaxprocs) } func Test_registryAuthForDockerVirtual(t *testing.T) { From a59703db178618f181e931009c4fb8f7b0579d8f Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Tue, 5 May 2026 12:18:05 -0500 Subject: [PATCH 03/18] Refactor commands() into testPlan with per-suite exit tracking and summary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the flat command slice with a testPlan struct that holds a fail-fast setup block and an ordered list of named suiteSteps. Each suite runs in its own bash subshell so a cd in one suite cannot affect another and exit codes are captured individually. When more than one suite is selected, script() appends a pass/fail summary with ANSI green ✓ / red ✗ markers and a completion timestamp for each suite. When only one suite runs the summary is omitted. The script always exits non-zero if any individual suite failed. ai-assisted=yes Co-authored-by: Cursor --- internal/test/container.go | 110 +++++++++++++++++--- internal/test/container_test.go | 178 ++++++++++++++++++++++++++------ 2 files changed, 241 insertions(+), 47 deletions(-) diff --git a/internal/test/container.go b/internal/test/container.go index 8616d3ab5..5588f123a 100644 --- a/internal/test/container.go +++ b/internal/test/container.go @@ -64,44 +64,124 @@ type Configuration struct { Verbose bool } +// testPlan holds the complete set of shell work for a kiln test run: global +// setup that must succeed before any suite, and an ordered list of named +// suites each of which is run as an independent unit. +type testPlan struct { + setup []string // fail-fast preamble (git config, etc.) + suites []suiteStep +} + +// suiteStep is one test suite — migrations, stability, or manifest. +// cmds are chained with && inside a subshell so their exit code is captured +// as a single unit. +type suiteStep struct { + name string // human label used in header and summary + cmds []string // shell commands; first entry is typically the header printf +} + // suiteHeader builds the "Running Suite: …\n…\n" printf command for a suite. func suiteHeader(title string) string { label := "Running Suite: " + title return fmt.Sprintf(`printf '\n%s\n%s\n'`, label, strings.Repeat("=", len(label))) } -func (configuration Configuration) commands() ([]string, error) { +func (configuration Configuration) commands() (testPlan, error) { if !filepath.IsAbs(configuration.AbsoluteTileDirectory) { - return nil, fmt.Errorf("tile path must be absolute") + return testPlan{}, fmt.Errorf("tile path must be absolute") } tileDirName := filepath.Base(configuration.AbsoluteTileDirectory) - commands := []string{"git config --global --add safe.directory '*'"} + plan := testPlan{ + setup: []string{"git config --global --add safe.directory '*'"}, + } + if configuration.RunMigrations || configuration.RunAll { - commands = append(commands, fmt.Sprintf("cd /tas/%s/migrations", tileDirName)) - commands = append(commands, npmInstallCommand(configuration.AbsoluteTileDirectory)) - commands = append(commands, suiteHeader("Migration Tests")) - commands = append(commands, "npm test") + plan.suites = append(plan.suites, suiteStep{ + name: "Migration Tests", + cmds: []string{ + fmt.Sprintf("cd /tas/%s/migrations", tileDirName), + npmInstallCommand(configuration.AbsoluteTileDirectory), + suiteHeader("Migration Tests"), + "npm test", + }, + }) } - // Each suite gets its own invocation so output is never interleaved. + // Each ginkgo suite gets its own invocation so output is never interleaved. // Stability tests use Go's standard testing package (no ginkgo bootstrap), so // ginkgo does not print "Running Suite: ..."; we add the header ourselves. // Manifest suites use ginkgo specs and print their own header — we only add a // blank line. Note: not compatible with tiles that use ginkgo v2. if configuration.RunMetadata || configuration.RunAll { stabilityPath := fmt.Sprintf("/tas/%s/test/stability", tileDirName) - commands = append(commands, suiteHeader("Stability Tests")) - commands = append(commands, fmt.Sprintf("cd /tas/%s && ginkgo %s %s", tileDirName, configuration.GinkgoFlags, stabilityPath)) + plan.suites = append(plan.suites, suiteStep{ + name: "Stability Tests", + cmds: []string{ + suiteHeader("Stability Tests"), + fmt.Sprintf("cd /tas/%s && ginkgo %s %s", tileDirName, configuration.GinkgoFlags, stabilityPath), + }, + }) } if configuration.RunManifest || configuration.RunAll { manifestPath := fmt.Sprintf("/tas/%s/test/manifest", tileDirName) - commands = append(commands, `printf '\n'`) - commands = append(commands, fmt.Sprintf("cd /tas/%s && ginkgo %s %s", tileDirName, configuration.GinkgoFlags, manifestPath)) + plan.suites = append(plan.suites, suiteStep{ + name: "Manifest Tests", + cmds: []string{ + `printf '\n'`, + fmt.Sprintf("cd /tas/%s && ginkgo %s %s", tileDirName, configuration.GinkgoFlags, manifestPath), + }, + }) + } + + return plan, nil +} + +// script produces the complete bash command for the test container. +// +// Each suite runs in a subshell so cd calls don't leak between suites. Exit +// codes are captured individually. When more than one suite is selected a +// colored pass/fail summary is printed at the end. The script always exits +// non-zero if any suite failed. +func (p testPlan) script() string { + var b strings.Builder + + // Global setup — fail fast on any error. + if len(p.setup) > 0 { + b.WriteString(strings.Join(p.setup, " && ")) + b.WriteString("\n") + } + + if len(p.suites) == 0 { + return b.String() + } + + // One subshell per suite; exit code and end-time stored in _exitN / _timeN. + for i, s := range p.suites { + fmt.Fprintf(&b, "\n(%s); _exit%d=$?\n", strings.Join(s.cmds, " && "), i) + fmt.Fprintf(&b, "_time%d=$(date '+%%H:%%M:%%S')\n", i) + } + + // Summary — only when running more than one suite. + if len(p.suites) > 1 { + b.WriteString("\nprintf '\\n'\n") + for i, s := range p.suites { + fmt.Fprintf(&b, + "[ $_exit%d -eq 0 ] && printf '[%%s] \\033[32m✓\\033[0m %s Passed\\n' \"$_time%d\" || printf '[%%s] \\033[31m✗\\033[0m %s Failed\\n' \"$_time%d\"\n", + i, s.name, i, s.name, i, + ) + } + } + + // Overall exit — non-zero if any suite failed. + b.WriteString("\n_overall=0\n") + for i := range p.suites { + fmt.Fprintf(&b, "[ $_exit%d -ne 0 ] && _overall=1\n", i) } + b.WriteString("exit $_overall\n") - return commands, nil + return b.String() } //counterfeiter:generate -o ./fakes/moby_client.go --fake-name MobyClient . mobyClient @@ -121,7 +201,7 @@ func runTest(ctx context.Context, w io.Writer, dockerDaemon mobyClient, configur return fmt.Errorf("failed to connect to Docker daemon: %w", err) } - commands, err := configuration.commands() + plan, err := configuration.commands() if err != nil { return err } @@ -172,7 +252,7 @@ func runTest(ctx context.Context, w io.Writer, dockerDaemon mobyClient, configur parentDir := path.Dir(configuration.AbsoluteTileDirectory) tileDir := path.Base(configuration.AbsoluteTileDirectory) - dockerCmd := strings.Join(commands, " && ") + dockerCmd := plan.script() envVars := getTileTestEnvVars(configuration.AbsoluteTileDirectory, tileDir, envMap) fmt.Fprintln(w, "Starting tests...") diff --git a/internal/test/container_test.go b/internal/test/container_test.go index ffcc7593b..6fc8e7d13 100644 --- a/internal/test/container_test.go +++ b/internal/test/container_test.go @@ -20,7 +20,7 @@ func TestConfiguration_commands(t *testing.T) { for _, tt := range []struct { Name string Configuration Configuration - ExpCmds []string + ExpPlan testPlan ExpErrSubstring string }{ { @@ -35,7 +35,9 @@ func TestConfiguration_commands(t *testing.T) { Configuration: Configuration{ AbsoluteTileDirectory: absoluteTileDirectory, }, - ExpCmds: []string{"git config --global --add safe.directory '*'"}, + ExpPlan: testPlan{ + setup: []string{"git config --global --add safe.directory '*'"}, + }, }, { Name: "when running migrations tests", @@ -43,12 +45,19 @@ func TestConfiguration_commands(t *testing.T) { AbsoluteTileDirectory: absoluteTileDirectory, RunMigrations: true, }, - ExpCmds: []string{ - "git config --global --add safe.directory '*'", - "cd /tas/test/migrations", - "npm install --no-audit --no-fund", - `printf '\nRunning Suite: Migration Tests\n==============================\n'`, - "npm test", + ExpPlan: testPlan{ + setup: []string{"git config --global --add safe.directory '*'"}, + suites: []suiteStep{ + { + name: "Migration Tests", + cmds: []string{ + "cd /tas/test/migrations", + "npm install --no-audit --no-fund", + `printf '\nRunning Suite: Migration Tests\n==============================\n'`, + "npm test", + }, + }, + }, }, }, { @@ -57,10 +66,17 @@ func TestConfiguration_commands(t *testing.T) { AbsoluteTileDirectory: absoluteTileDirectory, RunManifest: true, }, - ExpCmds: []string{ - "git config --global --add safe.directory '*'", - `printf '\n'`, - "cd /tas/test && ginkgo /tas/test/test/manifest", + ExpPlan: testPlan{ + setup: []string{"git config --global --add safe.directory '*'"}, + suites: []suiteStep{ + { + name: "Manifest Tests", + cmds: []string{ + `printf '\n'`, + "cd /tas/test && ginkgo /tas/test/test/manifest", + }, + }, + }, }, }, { @@ -69,10 +85,17 @@ func TestConfiguration_commands(t *testing.T) { AbsoluteTileDirectory: absoluteTileDirectory, RunMetadata: true, }, - ExpCmds: []string{ - "git config --global --add safe.directory '*'", - `printf '\nRunning Suite: Stability Tests\n==============================\n'`, - "cd /tas/test && ginkgo /tas/test/test/stability", + ExpPlan: testPlan{ + setup: []string{"git config --global --add safe.directory '*'"}, + suites: []suiteStep{ + { + name: "Stability Tests", + cmds: []string{ + `printf '\nRunning Suite: Stability Tests\n==============================\n'`, + "cd /tas/test && ginkgo /tas/test/test/stability", + }, + }, + }, }, }, { @@ -81,31 +104,121 @@ func TestConfiguration_commands(t *testing.T) { AbsoluteTileDirectory: absoluteTileDirectory, RunAll: true, }, - ExpCmds: []string{ - "git config --global --add safe.directory '*'", - "cd /tas/test/migrations", - "npm install --no-audit --no-fund", - `printf '\nRunning Suite: Migration Tests\n==============================\n'`, - "npm test", - `printf '\nRunning Suite: Stability Tests\n==============================\n'`, - "cd /tas/test && ginkgo /tas/test/test/stability", - `printf '\n'`, - "cd /tas/test && ginkgo /tas/test/test/manifest", + ExpPlan: testPlan{ + setup: []string{"git config --global --add safe.directory '*'"}, + suites: []suiteStep{ + { + name: "Migration Tests", + cmds: []string{ + "cd /tas/test/migrations", + "npm install --no-audit --no-fund", + `printf '\nRunning Suite: Migration Tests\n==============================\n'`, + "npm test", + }, + }, + { + name: "Stability Tests", + cmds: []string{ + `printf '\nRunning Suite: Stability Tests\n==============================\n'`, + "cd /tas/test && ginkgo /tas/test/test/stability", + }, + }, + { + name: "Manifest Tests", + cmds: []string{ + `printf '\n'`, + "cd /tas/test && ginkgo /tas/test/test/manifest", + }, + }, + }, }, }, } { t.Run(tt.Name, func(t *testing.T) { - cmds, err := tt.Configuration.commands() + plan, err := tt.Configuration.commands() if tt.ExpErrSubstring != "" { require.ErrorContains(t, err, tt.ExpErrSubstring) return } require.NoError(t, err) - require.Equal(t, tt.ExpCmds, cmds) + require.Equal(t, tt.ExpPlan.setup, plan.setup) + require.Len(t, plan.suites, len(tt.ExpPlan.suites)) + for i, expSuite := range tt.ExpPlan.suites { + require.Equal(t, expSuite.name, plan.suites[i].name) + require.Equal(t, expSuite.cmds, plan.suites[i].cmds) + } }) } } +func TestTestPlan_script_includesSummaryForMultipleSuites(t *testing.T) { + plan := testPlan{ + setup: []string{"setup cmd"}, + suites: []suiteStep{ + {name: "Migration Tests", cmds: []string{"npm test"}}, + {name: "Stability Tests", cmds: []string{"ginkgo stability"}}, + }, + } + + script := plan.script() + + // Each suite runs in a subshell with captured exit code. + require.Contains(t, script, "); _exit0=$?") + require.Contains(t, script, "); _exit1=$?") + + // End time captured right after each suite. + require.Contains(t, script, "_time0=$(date") + require.Contains(t, script, "_time1=$(date") + + // Summary lines present for both suites with captured timestamps. + require.Contains(t, script, "Migration Tests Passed") + require.Contains(t, script, "Migration Tests Failed") + require.Contains(t, script, "Stability Tests Passed") + require.Contains(t, script, "Stability Tests Failed") + require.Contains(t, script, "$_time0") + require.Contains(t, script, "$_time1") + + // ANSI green and red codes present. + require.Contains(t, script, "\\033[32m") + require.Contains(t, script, "\\033[31m") + + // Pass/fail symbols present. + require.Contains(t, script, "✓") + require.Contains(t, script, "✗") + + // Overall exit present. + require.Contains(t, script, "_overall") + require.Contains(t, script, "exit $_overall") +} + +func TestTestPlan_script_omitsSummaryForSingleSuite(t *testing.T) { + plan := testPlan{ + setup: []string{"setup cmd"}, + suites: []suiteStep{ + {name: "Manifest Tests", cmds: []string{"ginkgo manifest"}}, + }, + } + + script := plan.script() + + // No summary text for single suite. + require.NotContains(t, script, "Passed") + require.NotContains(t, script, "Failed") + + // Still exits with the suite's exit code. + require.Contains(t, script, "exit $_overall") +} + +func TestTestPlan_script_emptyWithNoSuites(t *testing.T) { + plan := testPlan{ + setup: []string{"git config --global --add safe.directory '*'"}, + } + script := plan.script() + require.Contains(t, script, "git config") + require.NotContains(t, script, "_exit0") + require.NotContains(t, script, "_overall") +} + func Test_checkImageBuildResponse(t *testing.T) { t.Run("streams build log then error", func(t *testing.T) { body := io.NopCloser(strings.NewReader( @@ -152,18 +265,19 @@ func TestConfiguration_commands_usesNpmCiWhenLockfilePresent(t *testing.T) { require.NoError(t, os.MkdirAll(filepath.Join(tileDir, "migrations"), 0o700)) require.NoError(t, os.WriteFile(filepath.Join(tileDir, "migrations", "package-lock.json"), []byte("{}"), 0o600)) - cmds, err := Configuration{AbsoluteTileDirectory: tileDir, RunMigrations: true}.commands() + plan, err := Configuration{AbsoluteTileDirectory: tileDir, RunMigrations: true}.commands() require.NoError(t, err) - require.Contains(t, cmds, "npm ci") + require.Len(t, plan.suites, 1) + require.Contains(t, plan.suites[0].cmds, "npm ci") } func TestConfiguration_commands_usesNpmInstallWithoutLockfile(t *testing.T) { tileDir := filepath.Join(t.TempDir(), "ist") require.NoError(t, os.MkdirAll(filepath.Join(tileDir, "migrations"), 0o700)) - cmds, err := Configuration{AbsoluteTileDirectory: tileDir, RunMigrations: true}.commands() + plan, err := Configuration{AbsoluteTileDirectory: tileDir, RunMigrations: true}.commands() require.NoError(t, err) - require.Contains(t, cmds, "npm install --no-audit --no-fund") + require.Contains(t, plan.suites[0].cmds, "npm install --no-audit --no-fund") } func TestGetTileTestEnvVars_setsGOMAXPROCS(t *testing.T) { From edd3cbe92f8e38a3b187613d76879a53d9aa2532 Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Tue, 5 May 2026 12:20:43 -0500 Subject: [PATCH 04/18] Add --verbose flag: timestamps, quiet npm by default Default --verbose to false so kiln test output stays clean. When verbose is off, npm ci/install runs with --silent to suppress progress bars and deprecation warnings. When verbose is on, npm output is fully visible and the script emits a timestamped Starting/Completed line around each suite so it is easy to see how long each phase takes. The verbose flag is propagated into testPlan so script() can toggle these behaviours without threading configuration deeper into helpers. ai-assisted=yes Co-authored-by: Cursor --- internal/commands/test_tile.go | 2 +- internal/test/container.go | 34 +++++++++++++++------ internal/test/container_test.go | 52 +++++++++++++++++++++++++++++++-- 3 files changed, 75 insertions(+), 13 deletions(-) diff --git a/internal/commands/test_tile.go b/internal/commands/test_tile.go index ea9410d87..927013744 100644 --- a/internal/commands/test_tile.go +++ b/internal/commands/test_tile.go @@ -17,7 +17,7 @@ type TileTestFunction func(ctx context.Context, w io.Writer, configuration test. type TileTest struct { Options struct { TilePath string ` long:"tile-path" default:"." description:"Path to the Tile directory (e.g. ~/workspace/tas/ist)."` - Verbose bool `short:"v" long:"verbose" default:"true" description:"Print extra details such as the container ID. This doesn't affect Ginkgo output."` + Verbose bool `short:"v" long:"verbose" default:"false" description:"Print extra details such as the container ID. This doesn't affect Ginkgo output."` Silent bool `short:"s" long:"silent" default:"false" description:"Hide info lines. This doesn't affect Ginkgo output."` Manifest bool ` long:"manifest" default:"false" description:"Focus the Manifest tests."` Migrations bool ` long:"migrations" default:"false" description:"Focus the Migration tests."` diff --git a/internal/test/container.go b/internal/test/container.go index 5588f123a..48b4fd329 100644 --- a/internal/test/container.go +++ b/internal/test/container.go @@ -68,8 +68,9 @@ type Configuration struct { // setup that must succeed before any suite, and an ordered list of named // suites each of which is run as an independent unit. type testPlan struct { - setup []string // fail-fast preamble (git config, etc.) - suites []suiteStep + setup []string // fail-fast preamble (git config, etc.) + suites []suiteStep + verbose bool // when true: timestamps before each suite; npm output visible } // suiteStep is one test suite — migrations, stability, or manifest. @@ -93,7 +94,8 @@ func (configuration Configuration) commands() (testPlan, error) { tileDirName := filepath.Base(configuration.AbsoluteTileDirectory) plan := testPlan{ - setup: []string{"git config --global --add safe.directory '*'"}, + setup: []string{"git config --global --add safe.directory '*'"}, + verbose: configuration.Verbose, } if configuration.RunMigrations || configuration.RunAll { @@ -101,7 +103,7 @@ func (configuration Configuration) commands() (testPlan, error) { name: "Migration Tests", cmds: []string{ fmt.Sprintf("cd /tas/%s/migrations", tileDirName), - npmInstallCommand(configuration.AbsoluteTileDirectory), + npmInstallCommand(configuration.AbsoluteTileDirectory, configuration.Verbose), suiteHeader("Migration Tests"), "npm test", }, @@ -159,8 +161,14 @@ func (p testPlan) script() string { // One subshell per suite; exit code and end-time stored in _exitN / _timeN. for i, s := range p.suites { + if p.verbose { + fmt.Fprintf(&b, "\necho \"[$(date '+%%H:%%M:%%S')] Starting: %s\"\n", s.name) + } fmt.Fprintf(&b, "\n(%s); _exit%d=$?\n", strings.Join(s.cmds, " && "), i) fmt.Fprintf(&b, "_time%d=$(date '+%%H:%%M:%%S')\n", i) + if p.verbose { + fmt.Fprintf(&b, "echo \"[$_time%d] Completed: %s\"\n", i, s.name) + } } // Summary — only when running more than one suite. @@ -417,14 +425,22 @@ func toProduct(dir string) string { } } -// npmInstallCommand returns "npm ci" when a package-lock.json is present in the -// tile's migrations directory (faster, strict), otherwise "npm install --no-audit --no-fund". -func npmInstallCommand(absoluteTileDir string) string { +// npmInstallCommand returns the appropriate npm install command. +// When not verbose, --silent suppresses all progress output; errors still cause +// a non-zero exit. When verbose, output is unrestricted so the user can see +// what npm is doing. +func npmInstallCommand(absoluteTileDir string, verbose bool) string { lockFile := filepath.Join(absoluteTileDir, "migrations", "package-lock.json") if _, err := os.Stat(lockFile); err == nil { - return "npm ci" + if verbose { + return "npm ci" + } + return "npm ci --silent" + } + if verbose { + return "npm install --no-audit --no-fund" } - return "npm install --no-audit --no-fund" + return "npm install --no-audit --no-fund --silent" } func getTileTestEnvVars(dir, productDir string, envMap environmentVars) environmentVars { diff --git a/internal/test/container_test.go b/internal/test/container_test.go index 6fc8e7d13..e58005d8d 100644 --- a/internal/test/container_test.go +++ b/internal/test/container_test.go @@ -52,7 +52,7 @@ func TestConfiguration_commands(t *testing.T) { name: "Migration Tests", cmds: []string{ "cd /tas/test/migrations", - "npm install --no-audit --no-fund", + "npm install --no-audit --no-fund --silent", `printf '\nRunning Suite: Migration Tests\n==============================\n'`, "npm test", }, @@ -111,7 +111,7 @@ func TestConfiguration_commands(t *testing.T) { name: "Migration Tests", cmds: []string{ "cd /tas/test/migrations", - "npm install --no-audit --no-fund", + "npm install --no-audit --no-fund --silent", `printf '\nRunning Suite: Migration Tests\n==============================\n'`, "npm test", }, @@ -268,7 +268,19 @@ func TestConfiguration_commands_usesNpmCiWhenLockfilePresent(t *testing.T) { plan, err := Configuration{AbsoluteTileDirectory: tileDir, RunMigrations: true}.commands() require.NoError(t, err) require.Len(t, plan.suites, 1) + // verbose=false (default): npm output silenced + require.Contains(t, plan.suites[0].cmds, "npm ci --silent") +} + +func TestConfiguration_commands_verboseUsesNpmCiWithoutSilent(t *testing.T) { + tileDir := filepath.Join(t.TempDir(), "ist") + require.NoError(t, os.MkdirAll(filepath.Join(tileDir, "migrations"), 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(tileDir, "migrations", "package-lock.json"), []byte("{}"), 0o600)) + + plan, err := Configuration{AbsoluteTileDirectory: tileDir, RunMigrations: true, Verbose: true}.commands() + require.NoError(t, err) require.Contains(t, plan.suites[0].cmds, "npm ci") + require.NotContains(t, plan.suites[0].cmds, "npm ci --silent") } func TestConfiguration_commands_usesNpmInstallWithoutLockfile(t *testing.T) { @@ -277,7 +289,41 @@ func TestConfiguration_commands_usesNpmInstallWithoutLockfile(t *testing.T) { plan, err := Configuration{AbsoluteTileDirectory: tileDir, RunMigrations: true}.commands() require.NoError(t, err) - require.Contains(t, plan.suites[0].cmds, "npm install --no-audit --no-fund") + require.Contains(t, plan.suites[0].cmds, "npm install --no-audit --no-fund --silent") +} + +func TestTestPlan_script_verbose_addsStartAndEndTimestamps(t *testing.T) { + plan := testPlan{ + setup: []string{"setup cmd"}, + verbose: true, + suites: []suiteStep{ + {name: "Migration Tests", cmds: []string{"npm test"}}, + {name: "Stability Tests", cmds: []string{"ginkgo stability"}}, + }, + } + + script := plan.script() + + // Start and end echo lines present for each suite. + require.Contains(t, script, "Starting: Migration Tests") + require.Contains(t, script, "Completed: Migration Tests") + require.Contains(t, script, "Starting: Stability Tests") + require.Contains(t, script, "Completed: Stability Tests") +} + +func TestTestPlan_script_noStartEndEchoWhenNotVerbose(t *testing.T) { + plan := testPlan{ + setup: []string{"setup cmd"}, + verbose: false, + suites: []suiteStep{{name: "Migration Tests", cmds: []string{"npm test"}}}, + } + + script := plan.script() + + // No verbose echo lines; end-time variable is still captured for potential summary use. + require.NotContains(t, script, "Starting:") + require.NotContains(t, script, "Completed:") + require.Contains(t, script, "_time0=$(date") } func TestGetTileTestEnvVars_setsGOMAXPROCS(t *testing.T) { From ba05565c259e5ae9711d87a8c6893f3a921f9f01 Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Tue, 5 May 2026 12:30:54 -0500 Subject: [PATCH 05/18] Rename Configuration.RunMetadata to RunStability The field was set by --stability, labels the suite "Stability Tests", and is described as stability everywhere. The old name conflicted with all of that, making the code harder to read. ai-assisted=yes Co-authored-by: Cursor --- internal/commands/test_tile.go | 2 +- internal/commands/test_tile_test.go | 8 ++++---- internal/test/container.go | 4 ++-- internal/test/container_test.go | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/commands/test_tile.go b/internal/commands/test_tile.go index 927013744..08792a810 100644 --- a/internal/commands/test_tile.go +++ b/internal/commands/test_tile.go @@ -71,7 +71,7 @@ func (cmd TileTest) configuration() (test.Configuration, error) { RunAll: !cmd.Options.Migrations && !cmd.Options.Manifest && !cmd.Options.Stability, RunManifest: cmd.Options.Manifest, - RunMetadata: cmd.Options.Stability, + RunStability: cmd.Options.Stability, RunMigrations: cmd.Options.Migrations, GinkgoFlags: cmd.Options.GingkoFlags, diff --git a/internal/commands/test_tile_test.go b/internal/commands/test_tile_test.go index c456afd52..c17eceb2c 100644 --- a/internal/commands/test_tile_test.go +++ b/internal/commands/test_tile_test.go @@ -167,7 +167,7 @@ var _ = Describe("kiln test", func() { Expect(w).NotTo(BeNil()) Expect(configuration.RunManifest).To(BeTrue()) - Expect(configuration.RunMetadata).To(BeFalse()) + Expect(configuration.RunStability).To(BeFalse()) Expect(configuration.RunMigrations).To(BeFalse()) }) }) @@ -189,13 +189,13 @@ var _ = Describe("kiln test", func() { Expect(w).NotTo(BeNil()) Expect(configuration.RunManifest).To(BeFalse()) - Expect(configuration.RunMetadata).To(BeFalse()) + Expect(configuration.RunStability).To(BeFalse()) Expect(configuration.RunMigrations).To(BeTrue()) }) }) When("when the stability test is enabled", func() { - It("it sets the RunMetadata configuration flag", func() { + It("it sets the RunStability configuration flag", func() { args := []string{"--stability"} fakeTestFunc := fakes.TestTileFunction{} @@ -211,7 +211,7 @@ var _ = Describe("kiln test", func() { Expect(w).NotTo(BeNil()) Expect(configuration.RunManifest).To(BeFalse()) - Expect(configuration.RunMetadata).To(BeTrue()) + Expect(configuration.RunStability).To(BeTrue()) Expect(configuration.RunMigrations).To(BeFalse()) }) }) diff --git a/internal/test/container.go b/internal/test/container.go index 48b4fd329..9d87380db 100644 --- a/internal/test/container.go +++ b/internal/test/container.go @@ -57,7 +57,7 @@ type Configuration struct { RunAll, RunMigrations, RunManifest, - RunMetadata bool + RunStability bool GinkgoFlags string Environment []string @@ -115,7 +115,7 @@ func (configuration Configuration) commands() (testPlan, error) { // ginkgo does not print "Running Suite: ..."; we add the header ourselves. // Manifest suites use ginkgo specs and print their own header — we only add a // blank line. Note: not compatible with tiles that use ginkgo v2. - if configuration.RunMetadata || configuration.RunAll { + if configuration.RunStability || configuration.RunAll { stabilityPath := fmt.Sprintf("/tas/%s/test/stability", tileDirName) plan.suites = append(plan.suites, suiteStep{ name: "Stability Tests", diff --git a/internal/test/container_test.go b/internal/test/container_test.go index e58005d8d..bc78ba869 100644 --- a/internal/test/container_test.go +++ b/internal/test/container_test.go @@ -83,7 +83,7 @@ func TestConfiguration_commands(t *testing.T) { Name: "when running metadata tests", Configuration: Configuration{ AbsoluteTileDirectory: absoluteTileDirectory, - RunMetadata: true, + RunStability: true, }, ExpPlan: testPlan{ setup: []string{"git config --global --add safe.directory '*'"}, From 6ca9e9aab4f56298dc0841082f6352ae8dd72f1e Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Tue, 5 May 2026 12:32:21 -0500 Subject: [PATCH 06/18] Eliminate double decodeEnvironment call in runTest RequiredArtifactoryCredentials was re-parsing configuration.Environment after runTest had already decoded it into envMap. Extract an unexported requiredArtifactoryCredentialsFromMap that works on the decoded map, and call it from runTest. Drop the redundant artifactoryUsername/Password alias variables; buildArgs now holds pointers to the resolved strings directly. ai-assisted=yes Co-authored-by: Cursor --- internal/test/container.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/internal/test/container.go b/internal/test/container.go index 9d87380db..d69b0bdcb 100644 --- a/internal/test/container.go +++ b/internal/test/container.go @@ -224,22 +224,19 @@ func runTest(ctx context.Context, w io.Writer, dockerDaemon mobyClient, configur return fmt.Errorf("failed to parse environment: %w", err) } - username, password, err := RequiredArtifactoryCredentials(configuration.Environment) + username, password, err := requiredArtifactoryCredentialsFromMap(envMap) if err != nil { return err } envMap["ARTIFACTORY_USERNAME"] = username envMap["ARTIFACTORY_PASSWORD"] = password - artifactoryUsername := username - artifactoryPassword := password - authConfigs := registryAuthForDockerVirtual(envMap) fmt.Fprintln(w, "Preparing test image...") buildArgs := map[string]*string{ - "ARTIFACTORY_USERNAME": &artifactoryUsername, - "ARTIFACTORY_PASSWORD": &artifactoryPassword, + "ARTIFACTORY_USERNAME": &username, + "ARTIFACTORY_PASSWORD": &password, } resp, err := dockerDaemon.ImageBuild(ctx, &dockerfileTarball, build.ImageBuildOptions{ @@ -367,6 +364,12 @@ func RequiredArtifactoryCredentials(envVarArgs []string) (username, password str if err != nil { return "", "", err } + return requiredArtifactoryCredentialsFromMap(m) +} + +// requiredArtifactoryCredentialsFromMap resolves credentials from an already-decoded +// environment map, falling back to os.Getenv when a value is absent. +func requiredArtifactoryCredentialsFromMap(m environmentVars) (username, password string, err error) { user := strings.TrimSpace(m["ARTIFACTORY_USERNAME"]) if user == "" { user = strings.TrimSpace(os.Getenv("ARTIFACTORY_USERNAME")) From 15ae184e84fe71220ae07b68cb3fce15d85848e5 Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Tue, 5 May 2026 12:33:25 -0500 Subject: [PATCH 07/18] Decompose runTest into buildTestImage and startAndWaitContainer runTest was a 145-line function covering five concerns: daemon ping, environment parsing, image build, container lifecycle, and log streaming. Extract buildTestImage (image build and registry auth) and startAndWaitContainer (create, start, signal handling, log drain, wait) so each unit is independently readable and testable. runTest becomes a 30-line orchestrator. Also removes the now-unused "path" import; path.Dir/Base were replaced with filepath.Dir/Base during extraction. ai-assisted=yes Co-authored-by: Cursor --- internal/test/container.go | 55 +++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 25 deletions(-) diff --git a/internal/test/container.go b/internal/test/container.go index d69b0bdcb..75d95477c 100644 --- a/internal/test/container.go +++ b/internal/test/container.go @@ -11,7 +11,6 @@ import ( "io" "os" "os/signal" - "path" "path/filepath" "runtime" "strconv" @@ -214,11 +213,6 @@ func runTest(ctx context.Context, w io.Writer, dockerDaemon mobyClient, configur return err } - var dockerfileTarball bytes.Buffer - if err := createDockerfileTarball(tar.NewWriter(&dockerfileTarball), dockerfile); err != nil { - return err - } - envMap, err := decodeEnvironment(configuration.Environment) if err != nil { return fmt.Errorf("failed to parse environment: %w", err) @@ -231,39 +225,50 @@ func runTest(ctx context.Context, w io.Writer, dockerDaemon mobyClient, configur envMap["ARTIFACTORY_USERNAME"] = username envMap["ARTIFACTORY_PASSWORD"] = password - authConfigs := registryAuthForDockerVirtual(envMap) + if err := buildTestImage(ctx, w, dockerDaemon, username, password, envMap); err != nil { + return err + } - fmt.Fprintln(w, "Preparing test image...") - buildArgs := map[string]*string{ - "ARTIFACTORY_USERNAME": &username, - "ARTIFACTORY_PASSWORD": &password, + parentDir := filepath.Dir(configuration.AbsoluteTileDirectory) + tileDir := filepath.Base(configuration.AbsoluteTileDirectory) + envVars := getTileTestEnvVars(configuration.AbsoluteTileDirectory, tileDir, envMap) + + return startAndWaitContainer(ctx, w, dockerDaemon, plan.script(), envVars, parentDir, configuration.Verbose) +} + +// buildTestImage builds the kiln test Docker image, forwarding Artifactory +// credentials as build args and registry auth for pulling base images. +func buildTestImage(ctx context.Context, w io.Writer, dockerDaemon mobyClient, username, password string, envMap environmentVars) error { + var dockerfileTarball bytes.Buffer + if err := createDockerfileTarball(tar.NewWriter(&dockerfileTarball), dockerfile); err != nil { + return err } + fmt.Fprintln(w, "Preparing test image...") resp, err := dockerDaemon.ImageBuild(ctx, &dockerfileTarball, build.ImageBuildOptions{ - Tags: []string{"kiln_test_dependencies:vmware"}, - BuildArgs: buildArgs, - AuthConfigs: authConfigs, + Tags: []string{"kiln_test_dependencies:vmware"}, + BuildArgs: map[string]*string{ + "ARTIFACTORY_USERNAME": &username, + "ARTIFACTORY_PASSWORD": &password, + }, + AuthConfigs: registryAuthForDockerVirtual(envMap), SuppressOutput: true, }) - if err != nil { return fmt.Errorf("failed to build image: %w", err) } - if err := checkImageBuildResponse(resp.Body, nil); err != nil { return fmt.Errorf("image build failed: %w", err) } + return nil +} - parentDir := path.Dir(configuration.AbsoluteTileDirectory) - tileDir := path.Base(configuration.AbsoluteTileDirectory) - - dockerCmd := plan.script() - - envVars := getTileTestEnvVars(configuration.AbsoluteTileDirectory, tileDir, envMap) - fmt.Fprintln(w, "Starting tests...") +// startAndWaitContainer creates, starts, and waits for the test container to +// exit, streaming its logs to w. It stops the container on SIGINT. +func startAndWaitContainer(ctx context.Context, w io.Writer, dockerDaemon mobyClient, script string, envVars environmentVars, parentDir string, verbose bool) error { testContainer, err := dockerDaemon.ContainerCreate(ctx, &container.Config{ Image: "kiln_test_dependencies:vmware", - Cmd: []string{"/bin/bash", "-c", dockerCmd}, + Cmd: []string{"/bin/bash", "-c", script}, Env: encodeEnvironment(envVars), Tty: true, }, &container.HostConfig{ @@ -284,7 +289,7 @@ func runTest(ctx context.Context, w io.Writer, dockerDaemon mobyClient, configur if err != nil { return fmt.Errorf("failed to create container: %w", err) } - if configuration.Verbose { + if verbose { fmt.Fprintf(w, "Container: %s\n", testContainer.ID) } From b80e9da091b518c3f4db5978549142a409cd41da Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Tue, 5 May 2026 12:33:56 -0500 Subject: [PATCH 08/18] Fix GingkoFlags typo: rename to GinkgoFlags The struct field TileTest.Options.GingkoFlags was silently renamed when assigned to Configuration.GinkgoFlags. Correcting the typo removes the inconsistency. ai-assisted=yes Co-authored-by: Cursor --- internal/commands/test_tile.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/commands/test_tile.go b/internal/commands/test_tile.go index 08792a810..7c42bcf55 100644 --- a/internal/commands/test_tile.go +++ b/internal/commands/test_tile.go @@ -24,7 +24,7 @@ type TileTest struct { Stability bool ` long:"stability" default:"false" description:"Focus the Stability tests."` EnvironmentVars []string `short:"e" long:"environment-variable" description:"Pass environment variables to the test suites (e.g. -e 'PRODUCT=srt'). Include -e ARTIFACTORY_USERNAME=... and -e ARTIFACTORY_PASSWORD=... unless they are exported."` - GingkoFlags string ` long:"ginkgo-flags" default:"-r -p -slowSpecThreshold 15" description:"Flags to pass to the Ginkgo Manifest and Stability test suites."` + GinkgoFlags string ` long:"ginkgo-flags" default:"-r -p -slowSpecThreshold 15" description:"Flags to pass to the Ginkgo Manifest and Stability test suites."` } function TileTestFunction output io.Writer @@ -74,7 +74,7 @@ func (cmd TileTest) configuration() (test.Configuration, error) { RunStability: cmd.Options.Stability, RunMigrations: cmd.Options.Migrations, - GinkgoFlags: cmd.Options.GingkoFlags, + GinkgoFlags: cmd.Options.GinkgoFlags, Environment: cmd.Options.EnvironmentVars, Verbose: cmd.Options.Verbose, } From 7c36e19eedad0590e73a45a9cbf3e2e387498675 Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Tue, 5 May 2026 12:35:40 -0500 Subject: [PATCH 09/18] Move verbose out of testPlan; pass as parameter to script() testPlan is a pure data value describing which suites to run. Verbosity is a rendering concern: it only affects the generated shell script, not what tests are selected. Changing the signature to script(verbose bool) makes this explicit at the call site and removes the hidden coupling between plan construction and script rendering. ai-assisted=yes Co-authored-by: Cursor --- internal/test/container.go | 19 +++++++++---------- internal/test/container_test.go | 18 ++++++++---------- 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/internal/test/container.go b/internal/test/container.go index 75d95477c..a44306aaa 100644 --- a/internal/test/container.go +++ b/internal/test/container.go @@ -67,9 +67,8 @@ type Configuration struct { // setup that must succeed before any suite, and an ordered list of named // suites each of which is run as an independent unit. type testPlan struct { - setup []string // fail-fast preamble (git config, etc.) - suites []suiteStep - verbose bool // when true: timestamps before each suite; npm output visible + setup []string // fail-fast preamble (git config, etc.) + suites []suiteStep } // suiteStep is one test suite — migrations, stability, or manifest. @@ -93,8 +92,7 @@ func (configuration Configuration) commands() (testPlan, error) { tileDirName := filepath.Base(configuration.AbsoluteTileDirectory) plan := testPlan{ - setup: []string{"git config --global --add safe.directory '*'"}, - verbose: configuration.Verbose, + setup: []string{"git config --global --add safe.directory '*'"}, } if configuration.RunMigrations || configuration.RunAll { @@ -144,8 +142,9 @@ func (configuration Configuration) commands() (testPlan, error) { // Each suite runs in a subshell so cd calls don't leak between suites. Exit // codes are captured individually. When more than one suite is selected a // colored pass/fail summary is printed at the end. The script always exits -// non-zero if any suite failed. -func (p testPlan) script() string { +// non-zero if any suite failed. When verbose is true, start/end timestamps +// are echoed before and after each suite. +func (p testPlan) script(verbose bool) string { var b strings.Builder // Global setup — fail fast on any error. @@ -160,12 +159,12 @@ func (p testPlan) script() string { // One subshell per suite; exit code and end-time stored in _exitN / _timeN. for i, s := range p.suites { - if p.verbose { + if verbose { fmt.Fprintf(&b, "\necho \"[$(date '+%%H:%%M:%%S')] Starting: %s\"\n", s.name) } fmt.Fprintf(&b, "\n(%s); _exit%d=$?\n", strings.Join(s.cmds, " && "), i) fmt.Fprintf(&b, "_time%d=$(date '+%%H:%%M:%%S')\n", i) - if p.verbose { + if verbose { fmt.Fprintf(&b, "echo \"[$_time%d] Completed: %s\"\n", i, s.name) } } @@ -233,7 +232,7 @@ func runTest(ctx context.Context, w io.Writer, dockerDaemon mobyClient, configur tileDir := filepath.Base(configuration.AbsoluteTileDirectory) envVars := getTileTestEnvVars(configuration.AbsoluteTileDirectory, tileDir, envMap) - return startAndWaitContainer(ctx, w, dockerDaemon, plan.script(), envVars, parentDir, configuration.Verbose) + return startAndWaitContainer(ctx, w, dockerDaemon, plan.script(configuration.Verbose), envVars, parentDir, configuration.Verbose) } // buildTestImage builds the kiln test Docker image, forwarding Artifactory diff --git a/internal/test/container_test.go b/internal/test/container_test.go index bc78ba869..22ee55227 100644 --- a/internal/test/container_test.go +++ b/internal/test/container_test.go @@ -160,7 +160,7 @@ func TestTestPlan_script_includesSummaryForMultipleSuites(t *testing.T) { }, } - script := plan.script() + script := plan.script(false) // Each suite runs in a subshell with captured exit code. require.Contains(t, script, "); _exit0=$?") @@ -199,7 +199,7 @@ func TestTestPlan_script_omitsSummaryForSingleSuite(t *testing.T) { }, } - script := plan.script() + script := plan.script(false) // No summary text for single suite. require.NotContains(t, script, "Passed") @@ -213,7 +213,7 @@ func TestTestPlan_script_emptyWithNoSuites(t *testing.T) { plan := testPlan{ setup: []string{"git config --global --add safe.directory '*'"}, } - script := plan.script() + script := plan.script(false) require.Contains(t, script, "git config") require.NotContains(t, script, "_exit0") require.NotContains(t, script, "_overall") @@ -294,15 +294,14 @@ func TestConfiguration_commands_usesNpmInstallWithoutLockfile(t *testing.T) { func TestTestPlan_script_verbose_addsStartAndEndTimestamps(t *testing.T) { plan := testPlan{ - setup: []string{"setup cmd"}, - verbose: true, + setup: []string{"setup cmd"}, suites: []suiteStep{ {name: "Migration Tests", cmds: []string{"npm test"}}, {name: "Stability Tests", cmds: []string{"ginkgo stability"}}, }, } - script := plan.script() + script := plan.script(true) // Start and end echo lines present for each suite. require.Contains(t, script, "Starting: Migration Tests") @@ -313,12 +312,11 @@ func TestTestPlan_script_verbose_addsStartAndEndTimestamps(t *testing.T) { func TestTestPlan_script_noStartEndEchoWhenNotVerbose(t *testing.T) { plan := testPlan{ - setup: []string{"setup cmd"}, - verbose: false, - suites: []suiteStep{{name: "Migration Tests", cmds: []string{"npm test"}}}, + setup: []string{"setup cmd"}, + suites: []suiteStep{{name: "Migration Tests", cmds: []string{"npm test"}}}, } - script := plan.script() + script := plan.script(false) // No verbose echo lines; end-time variable is still captured for potential summary use. require.NotContains(t, script, "Starting:") From 77093c893e28444ea8387a4243cbfa292a6ae422 Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Tue, 5 May 2026 12:36:07 -0500 Subject: [PATCH 10/18] Replace fmt.Errorf(\"%s\", detail) with errors.New in checkImageBuildResponse fmt.Errorf with a plain %s verb (no wrapping) triggers go vet's printf-style warning and adds unnecessary overhead. errors.New is the correct call when there is no cause to wrap. ai-assisted=yes Co-authored-by: Cursor --- internal/test/container.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/test/container.go b/internal/test/container.go index a44306aaa..619469b26 100644 --- a/internal/test/container.go +++ b/internal/test/container.go @@ -518,7 +518,7 @@ func checkImageBuildResponse(body io.ReadCloser, logOutput io.Writer) error { if msg.ErrorDetail.Message != "" { detail = msg.ErrorDetail.Message } - return fmt.Errorf("%s", detail) + return errors.New(detail) } } return nil From ec6287ff3d6ce3d9ff66705dbcc46a69a4092687 Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Tue, 5 May 2026 12:46:19 -0500 Subject: [PATCH 11/18] Fix errcheck lint: explicitly discard progress write return values fmt.Fprintln/Fprintf writes to the status writer are informational only; failing the operation because a progress line could not be printed would be incorrect. Use _, _ = to make the intentional discard explicit and satisfy errcheck. ai-assisted=yes Co-authored-by: Cursor --- internal/test/container.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/test/container.go b/internal/test/container.go index 619469b26..176c2a081 100644 --- a/internal/test/container.go +++ b/internal/test/container.go @@ -243,7 +243,7 @@ func buildTestImage(ctx context.Context, w io.Writer, dockerDaemon mobyClient, u return err } - fmt.Fprintln(w, "Preparing test image...") + _, _ = fmt.Fprintln(w, "Preparing test image...") resp, err := dockerDaemon.ImageBuild(ctx, &dockerfileTarball, build.ImageBuildOptions{ Tags: []string{"kiln_test_dependencies:vmware"}, BuildArgs: map[string]*string{ @@ -289,7 +289,7 @@ func startAndWaitContainer(ctx context.Context, w io.Writer, dockerDaemon mobyCl return fmt.Errorf("failed to create container: %w", err) } if verbose { - fmt.Fprintf(w, "Container: %s\n", testContainer.ID) + _, _ = fmt.Fprintf(w, "Container: %s\n", testContainer.ID) } errG := errgroup.Group{} @@ -327,7 +327,7 @@ func startAndWaitContainer(ctx context.Context, w io.Writer, dockerDaemon mobyCl if err != nil { return fmt.Errorf("container log request failure: %w", err) } - fmt.Fprintln(w, "") + _, _ = fmt.Fprintln(w, "") if _, err := io.Copy(w, out); err != nil { return err } From e08695721c25efe9786d01c36f1d1b15c24a03e2 Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Tue, 5 May 2026 13:02:54 -0500 Subject: [PATCH 12/18] Fix timestamps in non-verbose summary footer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _timeN was always captured and always interpolated into the [HH:MM:SS] prefix of the pass/fail summary lines, regardless of the verbose flag. Timestamps are a verbose-only concern: gate _timeN capture on verbose and produce two summary formats — plain (✓/✗ Suite Passed/Failed) when not verbose, timestamped when verbose. ai-assisted=yes Co-authored-by: Cursor --- internal/test/container.go | 20 ++++++++++++++------ internal/test/container_test.go | 24 +++++++++++++++--------- 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/internal/test/container.go b/internal/test/container.go index 176c2a081..ada7d14d6 100644 --- a/internal/test/container.go +++ b/internal/test/container.go @@ -157,14 +157,15 @@ func (p testPlan) script(verbose bool) string { return b.String() } - // One subshell per suite; exit code and end-time stored in _exitN / _timeN. + // One subshell per suite; exit code in _exitN. End-time (_timeN) only + // captured when verbose — it is only used in the verbose summary format. for i, s := range p.suites { if verbose { fmt.Fprintf(&b, "\necho \"[$(date '+%%H:%%M:%%S')] Starting: %s\"\n", s.name) } fmt.Fprintf(&b, "\n(%s); _exit%d=$?\n", strings.Join(s.cmds, " && "), i) - fmt.Fprintf(&b, "_time%d=$(date '+%%H:%%M:%%S')\n", i) if verbose { + fmt.Fprintf(&b, "_time%d=$(date '+%%H:%%M:%%S')\n", i) fmt.Fprintf(&b, "echo \"[$_time%d] Completed: %s\"\n", i, s.name) } } @@ -173,10 +174,17 @@ func (p testPlan) script(verbose bool) string { if len(p.suites) > 1 { b.WriteString("\nprintf '\\n'\n") for i, s := range p.suites { - fmt.Fprintf(&b, - "[ $_exit%d -eq 0 ] && printf '[%%s] \\033[32m✓\\033[0m %s Passed\\n' \"$_time%d\" || printf '[%%s] \\033[31m✗\\033[0m %s Failed\\n' \"$_time%d\"\n", - i, s.name, i, s.name, i, - ) + if verbose { + fmt.Fprintf(&b, + "[ $_exit%d -eq 0 ] && printf '[%%s] \\033[32m✓\\033[0m %s Passed\\n' \"$_time%d\" || printf '[%%s] \\033[31m✗\\033[0m %s Failed\\n' \"$_time%d\"\n", + i, s.name, i, s.name, i, + ) + } else { + fmt.Fprintf(&b, + "[ $_exit%d -eq 0 ] && printf '\\033[32m✓\\033[0m %s Passed\\n' || printf '\\033[31m✗\\033[0m %s Failed\\n'\n", + i, s.name, s.name, + ) + } } } diff --git a/internal/test/container_test.go b/internal/test/container_test.go index 22ee55227..50adf39f1 100644 --- a/internal/test/container_test.go +++ b/internal/test/container_test.go @@ -166,17 +166,17 @@ func TestTestPlan_script_includesSummaryForMultipleSuites(t *testing.T) { require.Contains(t, script, "); _exit0=$?") require.Contains(t, script, "); _exit1=$?") - // End time captured right after each suite. - require.Contains(t, script, "_time0=$(date") - require.Contains(t, script, "_time1=$(date") + // End time NOT captured without verbose — not needed for plain summary. + require.NotContains(t, script, "_time0=$(date") + require.NotContains(t, script, "_time1=$(date") - // Summary lines present for both suites with captured timestamps. + // Summary lines present for both suites without timestamps. require.Contains(t, script, "Migration Tests Passed") require.Contains(t, script, "Migration Tests Failed") require.Contains(t, script, "Stability Tests Passed") require.Contains(t, script, "Stability Tests Failed") - require.Contains(t, script, "$_time0") - require.Contains(t, script, "$_time1") + require.NotContains(t, script, "$_time0") + require.NotContains(t, script, "$_time1") // ANSI green and red codes present. require.Contains(t, script, "\\033[32m") @@ -292,7 +292,7 @@ func TestConfiguration_commands_usesNpmInstallWithoutLockfile(t *testing.T) { require.Contains(t, plan.suites[0].cmds, "npm install --no-audit --no-fund --silent") } -func TestTestPlan_script_verbose_addsStartAndEndTimestamps(t *testing.T) { +func TestTestPlan_script_verbose_addsTimestampsAndUsesThemInSummary(t *testing.T) { plan := testPlan{ setup: []string{"setup cmd"}, suites: []suiteStep{ @@ -308,6 +308,12 @@ func TestTestPlan_script_verbose_addsStartAndEndTimestamps(t *testing.T) { require.Contains(t, script, "Completed: Migration Tests") require.Contains(t, script, "Starting: Stability Tests") require.Contains(t, script, "Completed: Stability Tests") + + // End time captured and used in summary with timestamp prefix. + require.Contains(t, script, "_time0=$(date") + require.Contains(t, script, "_time1=$(date") + require.Contains(t, script, "$_time0") + require.Contains(t, script, "$_time1") } func TestTestPlan_script_noStartEndEchoWhenNotVerbose(t *testing.T) { @@ -318,10 +324,10 @@ func TestTestPlan_script_noStartEndEchoWhenNotVerbose(t *testing.T) { script := plan.script(false) - // No verbose echo lines; end-time variable is still captured for potential summary use. + // No verbose echo lines and no timestamp capture without verbose. require.NotContains(t, script, "Starting:") require.NotContains(t, script, "Completed:") - require.Contains(t, script, "_time0=$(date") + require.NotContains(t, script, "_time0=$(date") } func TestGetTileTestEnvVars_setsGOMAXPROCS(t *testing.T) { From d219e718a96faf48bb37c2d61d6af5e8f42c738c Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Tue, 5 May 2026 13:04:33 -0500 Subject: [PATCH 13/18] Update --verbose flag description to reflect actual behaviour The previous description only mentioned the container ID. Verbose also controls per-suite timestamps, the timestamped summary footer, and npm install verbosity. The new text lists all four effects. ai-assisted=yes Co-authored-by: Cursor --- internal/commands/test_tile.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/commands/test_tile.go b/internal/commands/test_tile.go index 7c42bcf55..ae835bd6c 100644 --- a/internal/commands/test_tile.go +++ b/internal/commands/test_tile.go @@ -17,7 +17,7 @@ type TileTestFunction func(ctx context.Context, w io.Writer, configuration test. type TileTest struct { Options struct { TilePath string ` long:"tile-path" default:"." description:"Path to the Tile directory (e.g. ~/workspace/tas/ist)."` - Verbose bool `short:"v" long:"verbose" default:"false" description:"Print extra details such as the container ID. This doesn't affect Ginkgo output."` + Verbose bool `short:"v" long:"verbose" default:"false" description:"Print container ID, per-suite start/end timestamps, and timestamped pass/fail summary. Also enables npm install output. Does not affect Ginkgo or npm test output."` Silent bool `short:"s" long:"silent" default:"false" description:"Hide info lines. This doesn't affect Ginkgo output."` Manifest bool ` long:"manifest" default:"false" description:"Focus the Manifest tests."` Migrations bool ` long:"migrations" default:"false" description:"Focus the Migration tests."` From 97b16f11c22e569f8893d1316da5747624118766 Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Wed, 6 May 2026 09:30:38 -0500 Subject: [PATCH 14/18] Add NPM virtual proxy Co-authored-by: Nick Rohn --- internal/test/Dockerfile | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/test/Dockerfile b/internal/test/Dockerfile index cf1f57ecd..ecee42389 100644 --- a/internal/test/Dockerfile +++ b/internal/test/Dockerfile @@ -46,3 +46,7 @@ RUN gem source -a https://${ARTIFACTORY_USERNAME}:${ARTIFACTORY_PASSWORD}@usw1.p RUN gem install --verbose ops-manifest -v 0.0.4.pre RUN which ops-manifest + +RUN printf '%s\n%s\n' \ + 'registry=https://${ARTIFACTORY_USERNAME}:${ARTIFACTORY_PASSWORD}@usw1.packages.broadcom.com/artifactory/api/npm/tis-npm-virtual/' \ + 'always-auth=true' > /root/.npmrc From fe19ecb6a971acb58dd2744ab7983f8915b7cd70 Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Wed, 6 May 2026 09:50:44 -0500 Subject: [PATCH 15/18] Use t.Setenv for Artifactory credential cleanup in test Replace manual os.LookupEnv/DeferCleanup save-restore pattern with GinkgoT().Setenv, which handles teardown automatically. ai-assisted=yes Co-authored-by: Cursor --- internal/commands/test_tile_test.go | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/internal/commands/test_tile_test.go b/internal/commands/test_tile_test.go index c17eceb2c..45b16fc91 100644 --- a/internal/commands/test_tile_test.go +++ b/internal/commands/test_tile_test.go @@ -257,22 +257,9 @@ var _ = Describe("kiln test", func() { When("when Artifactory credentials are missing", func() { It("returns an error before invoking the test function", func() { - savedU, hasU := os.LookupEnv("ARTIFACTORY_USERNAME") - savedP, hasP := os.LookupEnv("ARTIFACTORY_PASSWORD") - DeferCleanup(func() { - if hasU { - Expect(os.Setenv("ARTIFACTORY_USERNAME", savedU)).To(Succeed()) - } else { - Expect(os.Unsetenv("ARTIFACTORY_USERNAME")).To(Succeed()) - } - if hasP { - Expect(os.Setenv("ARTIFACTORY_PASSWORD", savedP)).To(Succeed()) - } else { - Expect(os.Unsetenv("ARTIFACTORY_PASSWORD")).To(Succeed()) - } - }) - Expect(os.Unsetenv("ARTIFACTORY_USERNAME")).To(Succeed()) - Expect(os.Unsetenv("ARTIFACTORY_PASSWORD")).To(Succeed()) + t := GinkgoT() + t.Setenv("ARTIFACTORY_USERNAME", "") + t.Setenv("ARTIFACTORY_PASSWORD", "") fakeTestFunc := fakes.TestTileFunction{} fakeTestFunc.Returns(nil) From e4db7029c57ea12c56d7c52796352ffd00e4eeb4 Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Wed, 6 May 2026 09:51:35 -0500 Subject: [PATCH 16/18] Simplify checkImageBuildResponse to take one argument The logOutput parameter was always passed nil at the only call site, and SuppressOutput: true on the ImageBuild call means the daemon never sends stream messages anyway. Drop the parameter and remove the dead write path. ai-assisted=yes Co-authored-by: Cursor --- internal/test/container.go | 12 ++++-------- internal/test/container_test.go | 8 ++------ 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/internal/test/container.go b/internal/test/container.go index ada7d14d6..d68f25a06 100644 --- a/internal/test/container.go +++ b/internal/test/container.go @@ -264,7 +264,7 @@ func buildTestImage(ctx context.Context, w io.Writer, dockerDaemon mobyClient, u if err != nil { return fmt.Errorf("failed to build image: %w", err) } - if err := checkImageBuildResponse(resp.Body, nil); err != nil { + if err := checkImageBuildResponse(resp.Body); err != nil { return fmt.Errorf("image build failed: %w", err) } return nil @@ -502,10 +502,9 @@ type imageBuildMessage struct { } `json:"errorDetail"` } -// checkImageBuildResponse reads the Docker/Podman image-build JSON stream. If -// logOutput is non-nil, "stream" lines are copied there; otherwise they are -// discarded. Build failures are still returned from daemon "error" messages. -func checkImageBuildResponse(body io.ReadCloser, logOutput io.Writer) error { +// checkImageBuildResponse reads the Docker/Podman image-build JSON stream and +// returns any build error reported by the daemon. +func checkImageBuildResponse(body io.ReadCloser) error { defer func() { _ = body.Close() }() @@ -518,9 +517,6 @@ func checkImageBuildResponse(body io.ReadCloser, logOutput io.Writer) error { } return fmt.Errorf("failed to read image build response: %w", err) } - if logOutput != nil && msg.Stream != "" { - _, _ = io.WriteString(logOutput, msg.Stream) - } if msg.Error != "" { detail := msg.Error if msg.ErrorDetail.Message != "" { diff --git a/internal/test/container_test.go b/internal/test/container_test.go index 50adf39f1..615e3a43d 100644 --- a/internal/test/container_test.go +++ b/internal/test/container_test.go @@ -1,7 +1,6 @@ package test import ( - "bytes" "io" "os" "path/filepath" @@ -220,17 +219,14 @@ func TestTestPlan_script_emptyWithNoSuites(t *testing.T) { } func Test_checkImageBuildResponse(t *testing.T) { - t.Run("streams build log then error", func(t *testing.T) { + t.Run("returns error from daemon error message", func(t *testing.T) { body := io.NopCloser(strings.NewReader( `{"stream":"Step 1\n"}` + "\n" + `{"stream":"go: downloading\n"}` + "\n" + `{"error":"failed","errorDetail":{"message":"go install: nope"}}` + "\n", )) - var buf bytes.Buffer - err := checkImageBuildResponse(body, &buf) + err := checkImageBuildResponse(body) require.ErrorContains(t, err, "go install: nope") - require.Contains(t, buf.String(), "Step 1") - require.Contains(t, buf.String(), "go: downloading") }) } From 2e8ab04f872416190e28993a8664bf772972eeb6 Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Wed, 6 May 2026 09:53:13 -0500 Subject: [PATCH 17/18] Pass Verbose flag and clean up setupTestRepo in integration test Set Verbose: testing.Verbose() on the integration test Configuration so build and container output is shown when running with -v, making network errors visible. Also tighten setupTestRepo: close the tar archive immediately after extraction (rather than in a deferred cleanup) and drop the redundant os.RemoveAll since t.TempDir() already manages the directory lifecycle. Switch assert to require for the setup calls so test failures are fatal. ai-assisted=yes Co-authored-by: Cursor --- internal/test/integration_test.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/internal/test/integration_test.go b/internal/test/integration_test.go index 652f904dc..bc379f279 100644 --- a/internal/test/integration_test.go +++ b/internal/test/integration_test.go @@ -49,6 +49,7 @@ func TestDockerIntegration(t *testing.T) { AbsoluteTileDirectory: tmpDir, RunAll: true, Environment: []string{"ARTIFACTORY_USERNAME=" + artifactoryUsername, "ARTIFACTORY_PASSWORD=" + artifactoryPassword}, + Verbose: testing.Verbose(), } out := io.Discard if testing.Verbose() { @@ -129,14 +130,10 @@ func setupTestRepo(t *testing.T) string { tmpDir := t.TempDir() happyTilePath := filepath.Join(wd, "testdata", "happy-tile") tar, err := archive.Tar(happyTilePath, compression.None) - assert.NoError(t, err) + require.NoError(t, err) err = archive.Untar(tar, tmpDir, nil) - assert.NoError(t, err) - t.Cleanup(func() { - err = os.RemoveAll(tmpDir) - assert.NoError(t, err) - _ = tar.Close() - }) + require.NoError(t, err) + _ = tar.Close() cmds := [][]string{ {"git", "init"}, From 19eebd58404216fcd23ce9e1913aad6c266c9ca6 Mon Sep 17 00:00:00 2001 From: Rizwan Reza Date: Thu, 7 May 2026 14:38:58 -0500 Subject: [PATCH 18/18] Restore verbose image build output in kiln test SuppressOutput was hardcoded to true and the logOutput writer was dropped when buildTestImage was extracted, silencing Docker build progress even with --verbose. Thread verbose through buildTestImage and restore the stream writer so build steps are visible when the flag is set. ai-assisted=yes Co-authored-by: Cursor --- internal/test/container.go | 22 +++++++++++++++------- internal/test/container_test.go | 2 +- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/internal/test/container.go b/internal/test/container.go index d68f25a06..4af0f1a12 100644 --- a/internal/test/container.go +++ b/internal/test/container.go @@ -232,7 +232,7 @@ func runTest(ctx context.Context, w io.Writer, dockerDaemon mobyClient, configur envMap["ARTIFACTORY_USERNAME"] = username envMap["ARTIFACTORY_PASSWORD"] = password - if err := buildTestImage(ctx, w, dockerDaemon, username, password, envMap); err != nil { + if err := buildTestImage(ctx, w, dockerDaemon, username, password, envMap, configuration.Verbose); err != nil { return err } @@ -245,13 +245,17 @@ func runTest(ctx context.Context, w io.Writer, dockerDaemon mobyClient, configur // buildTestImage builds the kiln test Docker image, forwarding Artifactory // credentials as build args and registry auth for pulling base images. -func buildTestImage(ctx context.Context, w io.Writer, dockerDaemon mobyClient, username, password string, envMap environmentVars) error { +func buildTestImage(ctx context.Context, w io.Writer, dockerDaemon mobyClient, username, password string, envMap environmentVars, verbose bool) error { var dockerfileTarball bytes.Buffer if err := createDockerfileTarball(tar.NewWriter(&dockerfileTarball), dockerfile); err != nil { return err } _, _ = fmt.Fprintln(w, "Preparing test image...") + var logOutput io.Writer + if verbose { + logOutput = w + } resp, err := dockerDaemon.ImageBuild(ctx, &dockerfileTarball, build.ImageBuildOptions{ Tags: []string{"kiln_test_dependencies:vmware"}, BuildArgs: map[string]*string{ @@ -259,12 +263,12 @@ func buildTestImage(ctx context.Context, w io.Writer, dockerDaemon mobyClient, u "ARTIFACTORY_PASSWORD": &password, }, AuthConfigs: registryAuthForDockerVirtual(envMap), - SuppressOutput: true, + SuppressOutput: !verbose, }) if err != nil { return fmt.Errorf("failed to build image: %w", err) } - if err := checkImageBuildResponse(resp.Body); err != nil { + if err := checkImageBuildResponse(resp.Body, logOutput); err != nil { return fmt.Errorf("image build failed: %w", err) } return nil @@ -502,9 +506,10 @@ type imageBuildMessage struct { } `json:"errorDetail"` } -// checkImageBuildResponse reads the Docker/Podman image-build JSON stream and -// returns any build error reported by the daemon. -func checkImageBuildResponse(body io.ReadCloser) error { +// checkImageBuildResponse reads the Docker/Podman image-build JSON stream. If +// logOutput is non-nil, "stream" lines are written there. Build errors are +// always returned. +func checkImageBuildResponse(body io.ReadCloser, logOutput io.Writer) error { defer func() { _ = body.Close() }() @@ -517,6 +522,9 @@ func checkImageBuildResponse(body io.ReadCloser) error { } return fmt.Errorf("failed to read image build response: %w", err) } + if logOutput != nil && msg.Stream != "" { + _, _ = io.WriteString(logOutput, msg.Stream) + } if msg.Error != "" { detail := msg.Error if msg.ErrorDetail.Message != "" { diff --git a/internal/test/container_test.go b/internal/test/container_test.go index 3f6a7c2df..fcbbf5fa6 100644 --- a/internal/test/container_test.go +++ b/internal/test/container_test.go @@ -225,7 +225,7 @@ func Test_checkImageBuildResponse(t *testing.T) { `{"stream":"go: downloading\n"}` + "\n" + `{"error":"failed","errorDetail":{"message":"go install: nope"}}` + "\n", )) - err := checkImageBuildResponse(body) + err := checkImageBuildResponse(body, nil) require.ErrorContains(t, err, "go install: nope") }) }