From fa9b31c2fa299b4e795a1063bfd7dc5f044f97ba Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Sun, 16 Aug 2026 14:34:52 +0200 Subject: [PATCH] test(e2e): checks.go as the reviewed vocabulary, SCENARIO.md codifies the contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Checks are the shared vocabulary between scenarios: move them to a dedicated checks.go so the whole vocabulary is read — and reviewed — as one file, independently of the tests using it. SCENARIO.md codifies the rules that so far only lived in the PR description: one intent = one invariant, step names as behavior sentences, inline compose model, state-based checks first with OutputContains as an explicit last resort, new checks generic and named after their observable — plus how to exploit the failure artifacts and E2E_KEEP_FAILED. AGENTS.md points new e2e work at it. Co-Authored-By: Claude Fable 5 Signed-off-by: Nicolas De Loof --- AGENTS.md | 9 ++ pkg/e2e/SCENARIO.md | 91 +++++++++++++++ pkg/e2e/checks.go | 268 ++++++++++++++++++++++++++++++++++++++++++++ pkg/e2e/scenario.go | 238 --------------------------------------- 4 files changed, 368 insertions(+), 238 deletions(-) create mode 100644 pkg/e2e/SCENARIO.md create mode 100644 pkg/e2e/checks.go diff --git a/AGENTS.md b/AGENTS.md index a0f73f682..e91c95f52 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -8,6 +8,15 @@ - Test single: `go test ./pkg/compose/ -run TestFunctionName` - E2E tests: `go test -tags e2e ./pkg/e2e/ -run TestName` +## E2E tests + +- **New e2e tests use the declarative `Scenario` DSL** (`NewScenario` in + `pkg/e2e/scenario.go`): intent, inline compose model, steps as + `(command → expected observables)`. Read `pkg/e2e/SCENARIO.md` before + writing or debugging one — it codifies the rules (state-based checks first, + `OutputContains` as last resort, new checks go in `pkg/e2e/checks.go`) and + how to exploit failure artifacts and `E2E_KEEP_FAILED=1`. + ## Lint - Linter: golangci-lint v2 (config in `.golangci.yml`) diff --git a/pkg/e2e/SCENARIO.md b/pkg/e2e/SCENARIO.md new file mode 100644 index 000000000..96a8d5f6c --- /dev/null +++ b/pkg/e2e/SCENARIO.md @@ -0,0 +1,91 @@ +# E2E scenarios: the contract + +With coding agents writing most of the production code, e2e tests are the +document humans actually read and review. A scenario must state an intent, a +compose model, and a sequence of steps `(command → expected observables)` — +and nothing else. Everything operational (project naming, cleanup, failure +diagnostics) belongs to the framework, not to the test. + +The DSL lives in [`scenario.go`](scenario.go) (execution, actions, +requirements) and [`checks.go`](checks.go) (the vocabulary of observables). + +## Writing a scenario + +```go +func TestRestart(t *testing.T) { + NewScenario(t, "restart must bring an exited service back up, restarting the same container"). + Compose(` +services: + app: + image: alpine + init: true + command: ash -c "if [[ -f /tmp/restart.lock ]] ; then sleep infinity; else touch /tmp/restart.lock; fi" +`). + Step("up starts the service, whose first run exits at once", + ComposeCmd("up", "-d"), + Eventually(ServiceState("app", "exited"), 10*time.Second)). + Step("restart brings the service back up, reusing the container", + ComposeCmd("restart"), + Eventually(ServiceState("app", "running"), 10*time.Second), + NotRecreated("app")) +} +``` + +Rules: + +- **New e2e tests use `NewScenario`.** The legacy `NewCLI` style remains for + existing tests, converted opportunistically; don't add to it. +- **One intent = one invariant.** The intent is a one-line statement of the + behavior being locked, phrased as an obligation ("X must Y"). If you need + two intents, write two scenarios. +- **Step names are behavior sentences**, not command echoes: "an unchanged + create is a no-op", not "run create again". The transcript of step names + should read as the specification. +- **The compose model is inline.** A scenario is self-contained in the test + source; no fixture directories. Interpolate runtime values via `Env`. +- **Regression tests link the issue** in a comment above the test, with a + sentence on the failure mode being locked. + +## Checks: observe real state + +Checks are the shared vocabulary between scenarios; their discipline is what +keeps the contract meaningful. + +- **Prefer state-based checks** (`ServiceState`, `NotRecreated`, `LabelSet`, + `RunsOnPlatform`, …): they observe containers, labels and image manifests — + what the user actually gets — not what the CLI printed. +- **`OutputContains` is a last resort**, legitimate only when the CLI's + reported decision is itself the observable (e.g. "Skipped" vs "Pulled"). +- **Never poll by hand**: wrap a state check in `Eventually(check, timeout)`. + No `time.Sleep` in scenarios. +- **A new check must be generic** — no test-specific logic — **and named + after the observable it asserts**, not after the test that needed it. It + goes in `checks.go`, where the whole vocabulary is reviewed as one file. + Before adding one, verify the observable isn't already expressible. +- A check should also fail loudly on a broken precondition (e.g. + `NotRecreated` errors if the service had no container before the step) + rather than pass vacuously. + +## When a scenario fails + +The report opens with everything needed to diagnose without re-running: + +- **`artifacts: `** — a stable per-project directory holding the + untruncated material: `compose.yaml`, `failure.txt`, each step's full + command and output (`step-NN-*.txt`), `containers.txt`, `events.txt`, full + container logs (`logs-*.txt`) and the per-step state snapshots + (`snapshots.json`). Read these before re-running anything. +- **`E2E_KEEP_FAILED=1`** — rerun with this set to skip teardown of failed + scenarios: containers, volumes and networks stay alive for `docker + inspect`/`exec`. Clean up afterwards with + `docker compose --project-name down -v --remove-orphans`. +- The inline report shows the transcript (every step, exit code, duration), + the failing step's output, project containers, engine events since the + scenario started, and container log tails — truncated for readability; the + artifacts have the full versions. + +Run a single scenario with: + +```sh +go test -tags e2e ./pkg/e2e/ -run TestRestart -v +``` diff --git a/pkg/e2e/checks.go b/pkg/e2e/checks.go new file mode 100644 index 000000000..88a6bb0f8 --- /dev/null +++ b/pkg/e2e/checks.go @@ -0,0 +1,268 @@ +/* + Copyright 2026 Docker Compose CLI authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package e2e + +// This file is the scenario vocabulary: the complete set of observables a +// step can expect. It is meant to be read — and reviewed — as a whole; see +// SCENARIO.md for the rules governing its growth. In short: checks observe +// real state (containers, labels, image manifests), are generic (no +// test-specific logic), and are named after the observable they assert. + +import ( + "fmt" + "runtime" + "slices" + "strings" + "time" + + "gotest.tools/v3/icmd" + "gotest.tools/v3/poll" +) + +// CheckContext gives a check access to the step's result and to the project +// state captured before and after the step. +type CheckContext struct { + scenario *Scenario + result *icmd.Result + prev snapshot + curr snapshot +} + +// refresh re-observes the project state, so subsequent checks of the same +// step see the latest state rather than the one captured right after the +// command returned. +func (ctx *CheckContext) refresh() { + s := ctx.scenario + ctx.curr = s.snapshot() + s.snaps[len(s.snaps)-1] = ctx.curr +} + +// Check is a named observable expected to hold after a step. +type Check struct { + name string + fn func(*CheckContext) error +} + +// OutputContains expects the command output to contain a string. Prefer +// state-based checks; use this when the CLI's reported decision is itself +// the observable. +func OutputContains(sub string) Check { + return Check{ + name: fmt.Sprintf("output contains %q", sub), + fn: func(ctx *CheckContext) error { + if !strings.Contains(ctx.result.Combined(), sub) { + return fmt.Errorf("not found in output") + } + return nil + }, + } +} + +// OutputNotContains expects the command output not to contain a string. +func OutputNotContains(sub string) Check { + return Check{ + name: fmt.Sprintf("output does not contain %q", sub), + fn: func(ctx *CheckContext) error { + if strings.Contains(ctx.result.Combined(), sub) { + return fmt.Errorf("found in output") + } + return nil + }, + } +} + +// Eventually retries a state-based check until it holds or the timeout +// expires, re-observing the project state between attempts. Output-based +// checks are not meaningful here: the step's output never changes. +// Polling is delegated to gotest.tools/v3/poll, the same engine the rest of +// the e2e framework uses. +func Eventually(check Check, timeout time.Duration) Check { + return Check{ + name: fmt.Sprintf("%s within %s", check.name, timeout), + fn: func(ctx *CheckContext) error { + first := true + capture := &pollCapture{} + done := make(chan struct{}) + // poll.WaitOn reports timeout through TestingT.Fatalf and relies + // on it halting execution; run it in a goroutine so pollCapture + // can Goexit and hand the error back to the scenario report + // instead of failing the test on the spot. + go func() { + defer close(done) + poll.WaitOn(capture, func(poll.LogT) poll.Result { + if !first { + ctx.refresh() + } + first = false + if err := check.fn(ctx); err != nil { + return poll.Continue("%v", err) + } + return poll.Success() + }, poll.WithDelay(500*time.Millisecond), poll.WithTimeout(timeout)) + }() + <-done + return capture.err + }, + } +} + +// pollCapture is a poll.TestingT that records the failure instead of failing +// the test, so Eventually can feed it to the scenario failure report. +type pollCapture struct { + err error +} + +func (c *pollCapture) Log(args ...any) {} +func (c *pollCapture) Logf(format string, args ...any) {} + +func (c *pollCapture) Fatalf(format string, args ...any) { + c.err = fmt.Errorf(format, args...) + runtime.Goexit() +} + +// ServiceState expects every container of the service to be in the given +// state (running, exited, restarting, …). +func ServiceState(service, state string) Check { + return Check{ + name: fmt.Sprintf("service %q is %s", service, state), + fn: func(ctx *CheckContext) error { + containers := ctx.curr[service] + if len(containers) == 0 { + return fmt.Errorf("service has no container") + } + for _, c := range containers { + if c.State != state { + return fmt.Errorf("container %s is %s", c.Name, c.State) + } + } + return nil + }, + } +} + +// NotRecreated expects the services' containers to be exactly the ones that +// existed before the step (same container IDs). +func NotRecreated(services ...string) Check { + return Check{ + name: fmt.Sprintf("services %s not recreated", strings.Join(services, ", ")), + fn: func(ctx *CheckContext) error { + for _, service := range services { + before, after := containerIDs(ctx.prev[service]), containerIDs(ctx.curr[service]) + if len(before) == 0 { + return fmt.Errorf("service %q had no container before the step", service) + } + if !slices.Equal(before, after) { + return fmt.Errorf("service %q containers changed: %v -> %v", service, before, after) + } + } + return nil + }, + } +} + +func containerIDs(containers []containerState) []string { + ids := make([]string, 0, len(containers)) + for _, c := range containers { + ids = append(ids, c.ID) + } + slices.Sort(ids) + return ids +} + +// LabelSet expects every container of the service to carry a non-empty label. +func LabelSet(service, key string) Check { + return Check{ + name: fmt.Sprintf("service %q has label %q set", service, key), + fn: func(ctx *CheckContext) error { + containers := ctx.curr[service] + if len(containers) == 0 { + return fmt.Errorf("service has no container") + } + for _, c := range containers { + if c.Labels[key] == "" { + return fmt.Errorf("label empty on container %s", c.Name) + } + } + return nil + }, + } +} + +// LabelsDistinct expects the given services to carry pairwise-distinct values +// for a label. +func LabelsDistinct(key string, services ...string) Check { + return Check{ + name: fmt.Sprintf("services %s have distinct %q labels", strings.Join(services, ", "), key), + fn: func(ctx *CheckContext) error { + seen := map[string]string{} + for _, service := range services { + containers := ctx.curr[service] + if len(containers) == 0 { + return fmt.Errorf("service %q has no container", service) + } + value := containers[0].Labels[key] + if other, dup := seen[value]; dup { + return fmt.Errorf("services %q and %q share label value %q", other, service, value) + } + seen[value] = service + } + return nil + }, + } +} + +// LabelUnchanged expects a service's label to have the same value as before +// the step. +func LabelUnchanged(service, key string) Check { + return Check{ + name: fmt.Sprintf("service %q label %q unchanged", service, key), + fn: func(ctx *CheckContext) error { + before, after := ctx.prev[service], ctx.curr[service] + if len(before) == 0 || len(after) == 0 { + return fmt.Errorf("service has no container to compare") + } + if before[0].Labels[key] != after[0].Labels[key] { + return fmt.Errorf("label changed: %q -> %q", before[0].Labels[key], after[0].Labels[key]) + } + return nil + }, + } +} + +// RunsOnPlatform expects the service's container to have been created for +// the given platform (from its image manifest descriptor). +func RunsOnPlatform(service, platform string) Check { + return Check{ + name: fmt.Sprintf("service %q container created for platform %s", service, platform), + fn: func(ctx *CheckContext) error { + containers := ctx.curr[service] + if len(containers) == 0 { + return fmt.Errorf("service has no container") + } + res := icmd.RunCmd(ctx.scenario.cli.NewDockerCmd(ctx.scenario.t, "inspect", "--format", + "{{.ImageManifestDescriptor.Platform.OS}}/{{.ImageManifestDescriptor.Platform.Architecture}}", + containers[0].ID)) + if res.ExitCode != 0 { + return fmt.Errorf("inspect failed: %s", res.Combined()) + } + if actual := strings.TrimSpace(res.Stdout()); actual != platform { + return fmt.Errorf("container platform is %s", actual) + } + return nil + }, + } +} diff --git a/pkg/e2e/scenario.go b/pkg/e2e/scenario.go index 96b877da0..b38e6bf3c 100644 --- a/pkg/e2e/scenario.go +++ b/pkg/e2e/scenario.go @@ -21,7 +21,6 @@ import ( "fmt" "os" "path/filepath" - "runtime" "slices" "strconv" "strings" @@ -30,7 +29,6 @@ import ( "unicode" "gotest.tools/v3/icmd" - "gotest.tools/v3/poll" ) // Scenario is a thin declarative layer over the e2e CLI helpers: a test reads @@ -490,239 +488,3 @@ func EngineVersionAtLeast(major int) Requirement { return "" } } - -// ---------- Checks ---------- - -// CheckContext gives a check access to the step's result and to the project -// state captured before and after the step. -type CheckContext struct { - scenario *Scenario - result *icmd.Result - prev snapshot - curr snapshot -} - -// refresh re-observes the project state, so subsequent checks of the same -// step see the latest state rather than the one captured right after the -// command returned. -func (ctx *CheckContext) refresh() { - s := ctx.scenario - ctx.curr = s.snapshot() - s.snaps[len(s.snaps)-1] = ctx.curr -} - -// Check is a named observable expected to hold after a step. -type Check struct { - name string - fn func(*CheckContext) error -} - -// OutputContains expects the command output to contain a string. Prefer -// state-based checks; use this when the CLI's reported decision is itself -// the observable. -func OutputContains(sub string) Check { - return Check{ - name: fmt.Sprintf("output contains %q", sub), - fn: func(ctx *CheckContext) error { - if !strings.Contains(ctx.result.Combined(), sub) { - return fmt.Errorf("not found in output") - } - return nil - }, - } -} - -// OutputNotContains expects the command output not to contain a string. -func OutputNotContains(sub string) Check { - return Check{ - name: fmt.Sprintf("output does not contain %q", sub), - fn: func(ctx *CheckContext) error { - if strings.Contains(ctx.result.Combined(), sub) { - return fmt.Errorf("found in output") - } - return nil - }, - } -} - -// Eventually retries a state-based check until it holds or the timeout -// expires, re-observing the project state between attempts. Output-based -// checks are not meaningful here: the step's output never changes. -// Polling is delegated to gotest.tools/v3/poll, the same engine the rest of -// the e2e framework uses. -func Eventually(check Check, timeout time.Duration) Check { - return Check{ - name: fmt.Sprintf("%s within %s", check.name, timeout), - fn: func(ctx *CheckContext) error { - first := true - capture := &pollCapture{} - done := make(chan struct{}) - // poll.WaitOn reports timeout through TestingT.Fatalf and relies - // on it halting execution; run it in a goroutine so pollCapture - // can Goexit and hand the error back to the scenario report - // instead of failing the test on the spot. - go func() { - defer close(done) - poll.WaitOn(capture, func(poll.LogT) poll.Result { - if !first { - ctx.refresh() - } - first = false - if err := check.fn(ctx); err != nil { - return poll.Continue("%v", err) - } - return poll.Success() - }, poll.WithDelay(500*time.Millisecond), poll.WithTimeout(timeout)) - }() - <-done - return capture.err - }, - } -} - -// pollCapture is a poll.TestingT that records the failure instead of failing -// the test, so Eventually can feed it to the scenario failure report. -type pollCapture struct { - err error -} - -func (c *pollCapture) Log(args ...any) {} -func (c *pollCapture) Logf(format string, args ...any) {} - -func (c *pollCapture) Fatalf(format string, args ...any) { - c.err = fmt.Errorf(format, args...) - runtime.Goexit() -} - -// ServiceState expects every container of the service to be in the given -// state (running, exited, restarting, …). -func ServiceState(service, state string) Check { - return Check{ - name: fmt.Sprintf("service %q is %s", service, state), - fn: func(ctx *CheckContext) error { - containers := ctx.curr[service] - if len(containers) == 0 { - return fmt.Errorf("service has no container") - } - for _, c := range containers { - if c.State != state { - return fmt.Errorf("container %s is %s", c.Name, c.State) - } - } - return nil - }, - } -} - -// NotRecreated expects the services' containers to be exactly the ones that -// existed before the step (same container IDs). -func NotRecreated(services ...string) Check { - return Check{ - name: fmt.Sprintf("services %s not recreated", strings.Join(services, ", ")), - fn: func(ctx *CheckContext) error { - for _, service := range services { - before, after := containerIDs(ctx.prev[service]), containerIDs(ctx.curr[service]) - if len(before) == 0 { - return fmt.Errorf("service %q had no container before the step", service) - } - if !slices.Equal(before, after) { - return fmt.Errorf("service %q containers changed: %v -> %v", service, before, after) - } - } - return nil - }, - } -} - -func containerIDs(containers []containerState) []string { - ids := make([]string, 0, len(containers)) - for _, c := range containers { - ids = append(ids, c.ID) - } - slices.Sort(ids) - return ids -} - -// LabelSet expects every container of the service to carry a non-empty label. -func LabelSet(service, key string) Check { - return Check{ - name: fmt.Sprintf("service %q has label %q set", service, key), - fn: func(ctx *CheckContext) error { - containers := ctx.curr[service] - if len(containers) == 0 { - return fmt.Errorf("service has no container") - } - for _, c := range containers { - if c.Labels[key] == "" { - return fmt.Errorf("label empty on container %s", c.Name) - } - } - return nil - }, - } -} - -// LabelsDistinct expects the given services to carry pairwise-distinct values -// for a label. -func LabelsDistinct(key string, services ...string) Check { - return Check{ - name: fmt.Sprintf("services %s have distinct %q labels", strings.Join(services, ", "), key), - fn: func(ctx *CheckContext) error { - seen := map[string]string{} - for _, service := range services { - containers := ctx.curr[service] - if len(containers) == 0 { - return fmt.Errorf("service %q has no container", service) - } - value := containers[0].Labels[key] - if other, dup := seen[value]; dup { - return fmt.Errorf("services %q and %q share label value %q", other, service, value) - } - seen[value] = service - } - return nil - }, - } -} - -// LabelUnchanged expects a service's label to have the same value as before -// the step. -func LabelUnchanged(service, key string) Check { - return Check{ - name: fmt.Sprintf("service %q label %q unchanged", service, key), - fn: func(ctx *CheckContext) error { - before, after := ctx.prev[service], ctx.curr[service] - if len(before) == 0 || len(after) == 0 { - return fmt.Errorf("service has no container to compare") - } - if before[0].Labels[key] != after[0].Labels[key] { - return fmt.Errorf("label changed: %q -> %q", before[0].Labels[key], after[0].Labels[key]) - } - return nil - }, - } -} - -// RunsOnPlatform expects the service's container to have been created for -// the given platform (from its image manifest descriptor). -func RunsOnPlatform(service, platform string) Check { - return Check{ - name: fmt.Sprintf("service %q container created for platform %s", service, platform), - fn: func(ctx *CheckContext) error { - containers := ctx.curr[service] - if len(containers) == 0 { - return fmt.Errorf("service has no container") - } - res := icmd.RunCmd(ctx.scenario.cli.NewDockerCmd(ctx.scenario.t, "inspect", "--format", - "{{.ImageManifestDescriptor.Platform.OS}}/{{.ImageManifestDescriptor.Platform.Architecture}}", - containers[0].ID)) - if res.ExitCode != 0 { - return fmt.Errorf("inspect failed: %s", res.Combined()) - } - if actual := strings.TrimSpace(res.Stdout()); actual != platform { - return fmt.Errorf("container platform is %s", actual) - } - return nil - }, - } -}