From 852eb8c6548f872b01a5a93769270e74a06fd0bb Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 14 Jul 2026 14:26:34 +0200 Subject: [PATCH 1/2] test: SwapSeam helper + advisory gremlins mutation workflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit internal/testutil.SwapSeam(t, ptr, stub) replaces the hand-rolled save/stub/restore blocks around package-level seam variables: swap, then LIFO-restore via t.Cleanup. Generic, so non-function seams (timeouts) work too. Converted the three blocks in internal/submit (watch_test.go x2, run_watch_test.go x1); internal/push has none. The ~26 blocks in internal/cli are deliberately left for the decomposition work to pick up; converting them here would collide. mutation.yml formalizes the gremlins ritual that produced #262-#264: workflow_dispatch only, one package per run, advisory (lived mutants never fail the job — no thresholds). Prints a survivors-to-triage summary, uploads the JSON report, and CONTRIBUTING.md documents how survivors get triaged into pinning-test issues. Fixes #295 Co-Authored-By: Claude Fable 5 --- .github/workflows/mutation.yml | 112 ++++++++++++++++++++++++++++++ CONTRIBUTING.md | 27 +++++++ internal/submit/run_watch_test.go | 7 +- internal/submit/watch_test.go | 9 +-- internal/testutil/seam.go | 33 +++++++++ internal/testutil/seam_test.go | 49 +++++++++++++ 6 files changed, 227 insertions(+), 10 deletions(-) create mode 100644 .github/workflows/mutation.yml create mode 100644 internal/testutil/seam.go create mode 100644 internal/testutil/seam_test.go diff --git a/.github/workflows/mutation.yml b/.github/workflows/mutation.yml new file mode 100644 index 00000000..d2760a7b --- /dev/null +++ b/.github/workflows/mutation.yml @@ -0,0 +1,112 @@ +name: Mutation (gremlins) + +# ADVISORY, manual-only mutation testing with go-gremlins (#295). +# +# Mutation testing answers the question coverage can't: "would the tests +# actually FAIL if this line's logic flipped?" Several real gaps in this repo +# were found by running gremlins by hand and pinning the survivors (#262, +# #263, #264). This workflow formalizes that ritual: +# +# - workflow_dispatch ONLY — never on PRs or pushes. Mutation runs are +# slow (each mutant recompiles + reruns the package tests) and their +# output needs human triage, so they must never gate a merge. +# - ONE package per run (the `package` input). Whole-module runs take +# hours and produce an untriageable wall of survivors. +# - Findings are NOT failures. A LIVED mutant means "no test would catch +# this logic flip" — triage it and file an issue (see CONTRIBUTING.md +# "Mutation testing" for the ritual: which survivors matter, which are +# noise, and how to write the pinning test). +# +# The job's conclusion only reflects tool/test health (a broken suite fails +# the run); lived mutants never do — no --threshold-* flags are set. + +on: + workflow_dispatch: + inputs: + package: + description: "Package to mutate, relative to the repo root (one per run), e.g. internal/push" + required: true + default: "internal/push" + type: string + timeout_coefficient: + description: "gremlins --timeout-coefficient (raise if mutants report TIMED OUT instead of KILLED/LIVED)" + required: false + default: "3" + type: string + +permissions: + contents: read + +concurrency: + group: mutation-${{ inputs.package }} + cancel-in-progress: false + +jobs: + mutate: + timeout-minutes: 45 + name: Mutation (${{ inputs.package }}) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version-file: go.mod + cache: true + + # Pinned for reproducibility, same policy as the lint tools in + # build.yml. v0.5.0 is the latest gremlins release. + - name: Install gremlins + run: go install github.com/go-gremlins/gremlins/cmd/gremlins@v0.5.0 + + # The inputs are typed through env vars and validated before use — + # never interpolate ${{ inputs.* }} straight into the script. + - name: Run mutation testing (advisory — lived mutants do not fail the job) + env: + PKG: ${{ inputs.package }} + TCOEF: ${{ inputs.timeout_coefficient }} + run: | + case "$PKG" in + (*[!a-zA-Z0-9/_.-]*|""|/*|*..*) + echo "::error::invalid package path '$PKG' (want e.g. internal/push)"; exit 1;; + esac + case "$TCOEF" in + (*[!0-9]*|"") + echo "::error::invalid timeout_coefficient '$TCOEF' (want a positive integer)"; exit 1;; + esac + if [ ! -d "$PKG" ]; then + echo "::error::no such package directory: $PKG"; exit 1 + fi + gremlins unleash "./$PKG" \ + --timeout-coefficient "$TCOEF" \ + --output gremlins-report.json \ + | tee gremlins.log + + - name: Summary — survivors to triage + if: always() + run: | + { + echo "## Mutation report: \`${PKG:-?}\`" + echo + echo '```' + grep -E "^\s+(LIVED|TIMED OUT)" gremlins.log 2>/dev/null || echo "no LIVED or TIMED OUT mutants — nothing to triage" + echo '```' + echo + tail -5 gremlins.log 2>/dev/null || true + echo + echo "Triage ritual: CONTRIBUTING.md > Mutation testing. Survivors worth killing become issues titled 'test(): pin (mutation survivor)'." + } >> "$GITHUB_STEP_SUMMARY" + env: + PKG: ${{ inputs.package }} + + - name: Upload machine-readable report + if: always() + uses: actions/upload-artifact@v4 + with: + name: gremlins-report + path: | + gremlins-report.json + gremlins.log + if-no-files-found: warn + retention-days: 30 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2bd03cbc..b1374a67 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -78,3 +78,30 @@ Every new subcommand needs at least: - Snapshot-style tests for output formats where the format is part of the contract (e.g. `--output-json`) See `internal/cli/version_test.go` for the pattern. + +### Test seams + +Packages stub their I/O boundaries through package-level function variables (`watchJobFn`, `newAPIClient`, `helm.Runner`, …). To swap one in a test, use the shared helper instead of a hand-rolled save/stub/restore block: + +```go +import "github.com/tracebloc/cli/internal/testutil" + +testutil.SwapSeam(t, &watchJobFn, func(...) (*WatchResult, error) { + return wr, nil +}) +``` + +`SwapSeam` sets the stub and restores the original via `t.Cleanup` (LIFO, so nested swaps unwind correctly). It's generic — non-function seams like timeouts work too. `internal/testutil` is test-support only: production code must never import it. + +## Mutation testing + +Coverage says a line *ran*; mutation testing says a test would *fail* if the line's logic flipped. We use [gremlins](https://github.com/go-gremlins/gremlins) as an **advisory, on-demand** check — it never gates a merge. + +**The ritual:** + +1. Trigger the [`Mutation (gremlins)` workflow](../../actions/workflows/mutation.yml) via *Run workflow*, giving it **one package per run** (e.g. `internal/push`). Whole-module runs take hours and produce an untriageable wall of survivors. Locally: `go install github.com/go-gremlins/gremlins/cmd/gremlins@v0.5.0 && gremlins unleash ./internal/push --timeout-coefficient 3`. +2. Read the run's summary: every `LIVED` mutant is a logic flip no test catches. (`TIMED OUT` usually means the timeout coefficient is too tight — re-run with a higher `timeout_coefficient` input before treating it as signal.) +3. Triage each survivor. Not every survivor matters: mutants in log strings, cosmetic branches, or defensive checks that are structurally unreachable are noise — note and skip them. Survivors in validation boundaries, error mapping, or anything a customer's data flows through are real gaps. +4. File one issue per real gap, titled `test(): pin (mutation survivor)`, quoting the gremlins line (mutant type + file:line) and what behavior the missing test must pin. That issue then flows through the kanban like any other test ticket — #262, #263, #264 are the pattern. + +Survivors are *findings to triage*, not build failures — the workflow stays green even when mutants live, on purpose. diff --git a/internal/submit/run_watch_test.go b/internal/submit/run_watch_test.go index bfd82c30..c5634635 100644 --- a/internal/submit/run_watch_test.go +++ b/internal/submit/run_watch_test.go @@ -11,6 +11,7 @@ import ( "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" + "github.com/tracebloc/cli/internal/testutil" "github.com/tracebloc/cli/internal/ui" ) @@ -22,11 +23,9 @@ import ( // themselves stay covered by watch_test.go. func withWatchJob(t *testing.T, wr *WatchResult, watchErr error) { t.Helper() - orig := watchJobFn - t.Cleanup(func() { watchJobFn = orig }) - watchJobFn = func(context.Context, kubernetes.Interface, string, string, io.Writer, *ui.Printer) (*WatchResult, error) { + testutil.SwapSeam(t, &watchJobFn, func(context.Context, kubernetes.Interface, string, string, io.Writer, *ui.Printer) (*WatchResult, error) { return wr, watchErr - } + }) } // runNonDetach drives Run down the watch path (Detach:false) with the submit POST diff --git a/internal/submit/watch_test.go b/internal/submit/watch_test.go index 5f466cbd..5ca20274 100644 --- a/internal/submit/watch_test.go +++ b/internal/submit/watch_test.go @@ -16,6 +16,7 @@ import ( "k8s.io/client-go/kubernetes/fake" k8stesting "k8s.io/client-go/testing" + "github.com/tracebloc/cli/internal/testutil" "github.com/tracebloc/cli/internal/ui" ) @@ -424,9 +425,7 @@ func TestWaitForJobPod_SameSecondTiePrefersLive(t *testing.T) { // WatchJob must fall back to the Pod phase and report Succeeded rather than // Unknown — which the orchestrator maps to a false exit 9. #219. func TestWatchJob_UnknownJobFallsBackToPodPhase(t *testing.T) { - prev := finalJobStatusTimeout - finalJobStatusTimeout = 150 * time.Millisecond - defer func() { finalJobStatusTimeout = prev }() + testutil.SwapSeam(t, &finalJobStatusTimeout, 150*time.Millisecond) cs := fake.NewClientset( jobPod("ingestor-fast", "ingestor", corev1.PodSucceeded), @@ -483,9 +482,7 @@ func TestMostRecentUsefulPod(t *testing.T) { // classify off a stale Failed Pod. With an older Failed Pod and a newer // Succeeded one and no Job condition, WatchJob must report Succeeded. #224. func TestWatchJob_UnknownFallbackPrefersSucceededOverFailed(t *testing.T) { - prev := finalJobStatusTimeout - finalJobStatusTimeout = 150 * time.Millisecond - defer func() { finalJobStatusTimeout = prev }() + testutil.SwapSeam(t, &finalJobStatusTimeout, 150*time.Millisecond) now := time.Now() failed := jobPod("ingestor-failed", "ingestor", corev1.PodFailed) diff --git a/internal/testutil/seam.go b/internal/testutil/seam.go new file mode 100644 index 00000000..cfaeb31d --- /dev/null +++ b/internal/testutil/seam.go @@ -0,0 +1,33 @@ +// Package testutil holds small helpers shared across the CLI's test suites. +// +// Test-support only: production code must never import this package. It lives +// under internal/ like everything else, so the compiler can't stop a stray +// import — code review must. +package testutil + +import "testing" + +// SwapSeam replaces the value behind ptr — typically a package-level function +// variable used as a test seam (watchJobFn, newAPIClient, helm.Runner, …), but +// any swappable package var (e.g. a timeout) works — with stub for the +// duration of the test, restoring the original via t.Cleanup. +// +// It replaces the hand-rolled three-line save/stub/restore block: +// +// orig := watchJobFn +// watchJobFn = stub +// t.Cleanup(func() { watchJobFn = orig }) +// +// with a single call: +// +// testutil.SwapSeam(t, &watchJobFn, stub) +// +// Cleanup ordering follows t.Cleanup semantics (LIFO), so nested swaps of the +// same seam restore correctly. NOT safe for use with t.Parallel() siblings +// that share the same seam — package-level seams never are. +func SwapSeam[T any](t testing.TB, ptr *T, stub T) { + t.Helper() + orig := *ptr + *ptr = stub + t.Cleanup(func() { *ptr = orig }) +} diff --git a/internal/testutil/seam_test.go b/internal/testutil/seam_test.go new file mode 100644 index 00000000..3aedf29e --- /dev/null +++ b/internal/testutil/seam_test.go @@ -0,0 +1,49 @@ +package testutil + +import "testing" + +// The seam must hold the stub for the test body and be restored by Cleanup — +// including nested swaps of the SAME seam, which must unwind LIFO back to the +// original. +func TestSwapSeam_SwapsAndRestores(t *testing.T) { + seam := func() string { return "original" } + + t.Run("inner", func(t *testing.T) { + SwapSeam(t, &seam, func() string { return "outer-stub" }) + if got := seam(); got != "outer-stub" { + t.Fatalf("seam() = %q after swap, want %q", got, "outer-stub") + } + + t.Run("nested", func(t *testing.T) { + SwapSeam(t, &seam, func() string { return "inner-stub" }) + if got := seam(); got != "inner-stub" { + t.Fatalf("seam() = %q after nested swap, want %q", got, "inner-stub") + } + }) + + // The nested subtest's Cleanup has run: back to the outer stub. + if got := seam(); got != "outer-stub" { + t.Fatalf("seam() = %q after nested cleanup, want %q (LIFO restore broken)", got, "outer-stub") + } + }) + + if got := seam(); got != "original" { + t.Fatalf("seam() = %q after all cleanups, want %q (restore broken)", got, "original") + } +} + +// Non-function seams (durations, limits) are first-class too — T is any. +func TestSwapSeam_NonFunctionValue(t *testing.T) { + limit := 100 + + t.Run("inner", func(t *testing.T) { + SwapSeam(t, &limit, 5) + if limit != 5 { + t.Fatalf("limit = %d after swap, want 5", limit) + } + }) + + if limit != 100 { + t.Fatalf("limit = %d after cleanup, want 100", limit) + } +} From 23b385335e89e902ffd2252fd1ec96b3f3dce922 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 14 Jul 2026 16:38:31 +0200 Subject: [PATCH 2/2] ci: harden gremlins mutation workflow (Bugbot #306) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Reject a zero timeout-coefficient. The digit check accepted `0`/`00`, which collapses gremlins' per-mutant timeout so survivors report TIMED OUT instead of KILLED/LIVED — the exact failure mode this input guards against. Now require a positive integer (> 0), matching the error text. - Add `set -o pipefail` to the run step. The implicit default shell is `bash -e {0}` (no pipefail), so `gremlins | tee` masked a non-zero gremlins exit behind tee's 0, letting a broken suite pass the job — contradicting the workflow's "tool/test health fails the run" contract. Co-Authored-By: Claude Fable 5 --- .github/workflows/mutation.yml | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/.github/workflows/mutation.yml b/.github/workflows/mutation.yml index d2760a7b..0bc26267 100644 --- a/.github/workflows/mutation.yml +++ b/.github/workflows/mutation.yml @@ -67,13 +67,21 @@ jobs: PKG: ${{ inputs.package }} TCOEF: ${{ inputs.timeout_coefficient }} run: | + # The implicit default shell is `bash -e {0}` (no pipefail), so a + # failure in `gremlins | tee` below would otherwise be masked by + # tee's exit code. pipefail makes tool/test health fail the job, as + # this workflow's contract promises. + set -o pipefail case "$PKG" in (*[!a-zA-Z0-9/_.-]*|""|/*|*..*) echo "::error::invalid package path '$PKG' (want e.g. internal/push)"; exit 1;; esac case "$TCOEF" in - (*[!0-9]*|"") + (""|*[!0-9]*) echo "::error::invalid timeout_coefficient '$TCOEF' (want a positive integer)"; exit 1;; + (*[1-9]*) ;; # all digits with at least one non-zero -> positive + (*) + echo "::error::invalid timeout_coefficient '$TCOEF' (want a positive integer greater than 0)"; exit 1;; esac if [ ! -d "$PKG" ]; then echo "::error::no such package directory: $PKG"; exit 1