[ndeloof]

What this PR does, in one sentence

Migrates the legacy icmd-style e2e tests of events_test.go, env_file_test.go and start_stop_test.go to the declarative Scenario DSL, without changing what they verify.

Context

About 200 e2e tests already use NewScenario, but 16 files in pkg/e2e still drive the CLI through icmd helpers directly. In those files the intent is buried in setup and cleanup code, and many assertions are brittle: they match progress lines such as Container e2e-start-stop-svc-multiple-files-simple-1 Stopped (including the hard-coded project name) instead of observing what the user gets, namely the state of the containers. The two styles side by side make the suite harder to read and to review.

Migrating everything at once would not be reviewable, so this is a first batch of the files that convert most mechanically. The others are left for follow-ups.

What the PR brings

Migrated (one commit per file):

• events_test.go: TestEventsUnknownService.
• env_file_test.go: TestUnusedMissingEnvFile (a 12-command test becomes a readable sequence of steps).
• start_stop_test.go: TestStartAlreadyRunning, TestStopAlreadyStopped, TestStartStopMultipleServices, TestStartStopMultipleFiles.

What the Scenario form gives:

• Each test states its intent in one sentence and its steps as (command -> expected observables).
• State-based checks (ServiceState, ServiceScale, NotRecreated, ServiceNotCreated) replace most output matching. OutputContains/StderrContains remain only where the CLI's message is the observable (an error naming the unknown service or the missing env file).
• Isolated project names, automatic cleanup, and failure artifacts (transcript, container state, events, logs) for free; the hand-written down cleanups and the hard-coded project names go away.
• Fixtures move to testdata/<TestName>/; fixtures/env_file and fixtures/start-stop had no other user and are removed. fixtures/simple-composefile is still used by another test and stays.

Two small, generic additions to the DSL, each in its own commit:

• StdoutEmpty() check (checks.go): a no-op command must stay silent on stdout.
• CompleteCmd(...) action (scenario.go): reaches shell completion in both plugin (docker __complete compose) and standalone (docker-compose __complete) forms, so completion stays covered in both modes as before.

Guardrails:

• No behavior change and no production code touched: tests and test data only.
• Same intent preserved. Deliberate differences, all listed here:
  • TestStartStopMultipleServices and TestStartStopMultipleFiles no longer match the Container ... Stopped/Started progress lines; they assert the resulting container states (and, for multiple files, that the services of the other file keep running), which is what those messages stood for.
  • TestUnusedMissingEnvFile also asserts states (service running, scaled to 2, serviceB never created) and a few outputs (ps lists serviceA, exec prints hello) that were only exit-code checks before. Its fixture drops the unrelated serviceC (and test.env).
  • TestStopAlreadyStopped keeps the quirk it already pinned (a Stopped line on stderr) with its TODO, and additionally checks the service stays stopped and is not recreated.
  • TestStartAlreadyRunning additionally checks the container is neither recreated nor stopped.
• Tests that cannot be expressed without weakening them are not touched: they need a background process, a signal, or a concurrent command, which the DSL does not model (for example TestWaitOnInfinity, TestComposeCancel, TestCascadeIgnoresOneOffContainer, TestLocalComposeLogsFollow).
• Remaining files are left for follow-ups.

AI disclosure: prepared with GitHub Copilot CLI, orchestrated by Claude Code, on behalf of and under the direction of the submitter.

[docker-agent]

Assessment: 🟢 APPROVE

This PR cleanly migrates six icmd-style e2e tests to the Scenario DSL and adds two small, generic DSL extensions. The migration is straightforward and preserves test intent; the new StdoutEmpty() check and CompleteCmd action are well-structured. Two minor issues were found in the new code.

[docker-agent]

[medium] StdoutEmpty() compares raw Stdout() to "" without trimming — may produce false failures

ctx.result.Stdout() returns the raw captured bytes from the process's stdout buffer; it does not strip trailing whitespace. Many CLI tools (including docker compose start) write a bare trailing newline to stdout even when they produce no meaningful output, so Stdout() returns "\n" rather than "". As written, StdoutEmpty() would report a spurious failure for TestStartAlreadyRunning's "start leaves the running container alone and writes nothing to stdout" step.

The rest of checks.go uses strings.TrimSpace(res.Stdout()) in every empty-sensitive comparison (e.g. lines 382, 589, 628, 730). StdoutContains does not need trimming because strings.Contains tolerates a trailing newline, but StdoutEmpty is specifically asserting complete absence of content, where a trailing newline causes a false failure.

Suggested change

	if out := ctx.result.Stdout(); out != "" {
	if out := strings.TrimSpace(ctx.result.Stdout()); out != "" {

Confidence	Score
🟡 moderate	57/100

[ndeloof]

Not changed on purpose. StdoutEmpty is meant to pin the "no-op stays silent on stdout" contract exactly, so trimming would let a stray newline slip through. The premise also does not hold here: docker compose start on an already running service writes nothing to stdout (progress goes to stderr), and TestStartAlreadyRunning passes in the plugin and standalone CI jobs with the strict comparison. The TrimSpace calls elsewhere in checks.go compare values extracted from a command output, which is a different use.

[docker-agent]

Thank you for the clarification — I understand now. The distinction is important: StdoutEmpty is enforcing an exact contract that stdout is completely silent (zero bytes), not "appears empty after normalizing whitespace." The fact that TestStartAlreadyRunning passes in CI confirms that docker compose start on an already-running service does indeed write nothing (not even a newline) to stdout, so the strict comparison is both intentional and correct.

I misread the pattern. The TrimSpace calls I cited are for extracting values from output (where trailing whitespace is incidental formatting), not for asserting complete silence. This is a false positive on my part.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[docker-agent]

Assessment: 🟢 APPROVE