diff --git a/.github/workflows/pr-workflow.yaml b/.github/workflows/pr-workflow.yaml index e26f0fcab..981608767 100644 --- a/.github/workflows/pr-workflow.yaml +++ b/.github/workflows/pr-workflow.yaml @@ -38,6 +38,10 @@ jobs: # A bound well clear of the observed runtime. Without one the platform # default applies, and a wedged step burns a free-tier slot for six hours. timeout-minutes: 45 + env: + # Where each runner writes its JUnit. Every invocation needs its own file; + # a shared path means the last writer wins. + ARTIFACTS: ${{ github.workspace }}/_artifacts steps: - name: Checkout uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5.1.0 @@ -51,16 +55,39 @@ jobs: # this guards, and that happens on the push to main, not on the PR. - name: Verify immutable PostgreSQL migrations run: hack/verify/postgresql-migrations.sh - - run: go test -race -v ./... + # gotestsum wraps go test to emit JUnit; the go test arguments are unchanged. + - name: unit tests + run: hack/run-tool.sh gotestsum --junitfile "${ARTIFACTS}/unit.xml" + --jsonfile "${ARTIFACTS}/unit.json" --format standard-verbose + -- -race -v ./... # tools/apitool has its own module so we need to run it explicitly. + # run-tool.sh resolves the repo root itself, so it works from the subdir. - name: apitool tests working-directory: tools/apitool - run: go test -race -v ./... + run: ../../hack/run-tool.sh gotestsum --junitfile "${ARTIFACTS}/apitool.xml" + --jsonfile "${ARTIFACTS}/apitool.json" --format standard-verbose + -- -race -v ./... # Root-gated tests (overlay mounts, whiteout mknod, trusted.* xattrs, ...) # skip for the unprivileged runner user above; rerun the packages that # contain them (any test importing internal/roottest) under sudo. - name: root-gated tests + env: + ROOT_JUNIT_FILE: ${{ github.workspace }}/_artifacts/root.xml run: hack/run-root-tests.sh -race -v + # An exit code cannot distinguish "all passed" from "none ran". A registered + # file that was never written fails too. + - name: Require tests to have run + if: always() + run: | + make build-junittool + bin/junittool verify -manifest "${ARTIFACTS}/expected-junit.txt" + - name: Upload test results + if: always() + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + with: + name: test-results-unit + path: ${{ env.ARTIFACTS }} + if-no-files-found: error - name: verify run: hack/verify-all.sh # One kind cluster exercises BOTH runtimes. Free x86-64 ubuntu-latest runners @@ -95,6 +122,9 @@ jobs: ) }} env: E2E_ATENET_DATAPLANE: ${{ matrix.dataplane }} + # E2E_JUNIT_FILE is set per step, not here: the six lanes below share this + # job, so one job-level path would have each overwrite the last. + ARTIFACTS: ${{ github.workspace }}/_artifacts steps: - name: Checkout uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5.1.0 @@ -151,6 +181,8 @@ jobs: hack/install-ate-kind.sh --deploy-demo-egress hack/install-ate-kind.sh --deploy-demo-egress-microvm - name: Run E2E tests (gVisor) + env: + E2E_JUNIT_FILE: ${{ github.workspace }}/_artifacts/e2e-gvisor.xml run: hack/run-e2e-kind.sh -v -args --no-color - name: Run E2E tests (micro-VM) # The same suites again, with every fixture repointed at its micro-VM @@ -160,6 +192,7 @@ jobs: # two runs do not contend for the one kind node. env: E2E_SANDBOX_CLASS: microvm + E2E_JUNIT_FILE: ${{ github.workspace }}/_artifacts/e2e-microvm.xml run: hack/run-e2e-kind.sh -v -args --no-color - name: Deploy MITM egress (sdsmint) # Swap the passthrough egress gateway for the sdsmint variant, which @@ -177,6 +210,7 @@ jobs: # internal/e2e/suites/egressmitm). env: E2E_EGRESS_MITM: "1" + E2E_JUNIT_FILE: ${{ github.workspace }}/_artifacts/e2e-mitm.xml run: hack/run-e2e-kind.sh ./internal/e2e/suites/egressmitm -v -args --no-color - name: Run E2E tests (egress MITM trust, micro-VM) # The same proof with the probe on the micro-VM runtime. Trust DELIVERY @@ -186,6 +220,7 @@ jobs: env: E2E_EGRESS_MITM: "1" E2E_SANDBOX_CLASS: microvm + E2E_JUNIT_FILE: ${{ github.workspace }}/_artifacts/e2e-mitm-microvm.xml run: hack/run-e2e-kind.sh ./internal/e2e/suites/egressmitm -v -args --no-color - name: Deploy MITM egress demo # The networking suite's egress tests need a fixture that trusts the @@ -196,12 +231,30 @@ jobs: - name: Run E2E tests (networking, MITM egress) env: E2E_EGRESS_MITM: "1" + E2E_JUNIT_FILE: ${{ github.workspace }}/_artifacts/e2e-networking-mitm.xml run: hack/run-e2e-kind.sh ./internal/e2e/suites/networking -run '^TestActorEgress' -v -args --no-color - name: Run E2E tests (networking, MITM egress, micro-VM) env: E2E_EGRESS_MITM: "1" E2E_SANDBOX_CLASS: microvm + E2E_JUNIT_FILE: ${{ github.workspace }}/_artifacts/e2e-networking-mitm-microvm.xml run: hack/run-e2e-kind.sh ./internal/e2e/suites/networking -run '^TestActorEgress' -v -args --no-color + # Every lane must have reported tests; a `-run` filter that stops matching + # otherwise leaves its lane green and empty. The set comes from the manifest + # each lane registers itself in, so adding a lane needs no change here, and + # run-e2e.sh refuses to run in CI without E2E_JUNIT_FILE. + - name: Require every lane to have run tests + if: always() + run: | + make build-junittool + bin/junittool verify -manifest "${ARTIFACTS}/expected-junit.txt" + - name: Upload test results + if: always() + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + with: + name: test-results-e2e-${{ matrix.dataplane }} + path: ${{ env.ARTIFACTS }} + if-no-files-found: error - name: Dump diagnostics on failure if: failure() run: | diff --git a/.gitignore b/.gitignore index 867494a16..ec27aa29c 100644 --- a/.gitignore +++ b/.gitignore @@ -17,6 +17,10 @@ __pycache__/ # Generated scratch directory for benchmark automation /benchmarking/automation/scratch/ +# Test artifacts (JUnit XML, gotestsum JSON). CI writes these into the +# workspace, and hack/verify-all.sh refuses to run against a dirty tree. +/_artifacts/ + # Python gRPC clients, generated by benchmarking/locust/codegen/generate.sh /benchmarking/locust/common/*_pb2.py /benchmarking/locust/common/*_pb2_grpc.py @@ -55,8 +59,13 @@ Thumbs.db # Local worktrees /.worktrees/ -# Stray local build outputs (go build ./tools/... without -o) +# Stray outputs from `go build` without -o. Root-module tools land at the repo +# root; tools with their own go.mod land beside their source. Use +# `make build-`, which writes to /bin/ above. /validate-image-cache +/setup-gcp +/tools/apitool/apitool +/tools/junittool/junittool # Substrate agent workspace scratchpads .agents/scratch/ diff --git a/AGENTS.md b/AGENTS.md index 7a7a0b096..7ae6acf36 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -83,6 +83,7 @@ See the [metric registry](docs/observability.md#the-metric-registry) section of 2. Ensure changes do not break existing tests. 3. Run `make verify` locally before requesting a code review to catch common issues like missed copyright headers or formatting drift. 4. For end-to-end tests involving the actual infrastructure, ensure you have a running cluster (setup via `hack/ate-dev-env.sh.example` and `go run ./tools/setup-gcp bootstrap`). +5. A test that skips when a precondition is missing — Docker, a cluster, an artifact directory — must **fail** on it in CI, because a skip and a pass are the same exit code. Resolve strictness through a named predicate rather than an inline `os.Getenv("CI")`; `cmd/ateapi/internal/store/dockerenv.Required()` is the reference implementation. Prove the strict branch red before merging: a guard only ever observed passing is not known to guard anything. See `docs/dev/best-practices/ci-fail-closed.md`. ## Security Considerations diff --git a/Makefile b/Makefile index 0d6a97f3a..53bdc0786 100644 --- a/Makefile +++ b/Makefile @@ -96,6 +96,13 @@ build-ate-setup: build-atenet: $(GO) build -ldflags "$(LDFLAGS)" -o $(BINDIR)/atenet ./cmd/atenet +# Registers the JUnit files a CI run will write, and verifies they hold tests. +# Separate module, so it builds by path. -o keeps the binary in $(BINDIR), +# which is gitignored and removed by `clean`. +.PHONY: build-junittool +build-junittool: + $(GO) -C tools/junittool build -o $(CURDIR)/$(BINDIR)/junittool . + .PHONY: build-demos build-demos: $(KO) build $(KO_NAMING) $(KO_FLAGS) \ diff --git a/docs/code-style-guide.md b/docs/code-style-guide.md index 3935bb0ae..16f597930 100644 --- a/docs/code-style-guide.md +++ b/docs/code-style-guide.md @@ -39,6 +39,11 @@ far from its cause. - Table-driven tests with `t.Run` subtests are the default shape. - Prefer a real test implementation when one exists: the PostgreSQL test fixture for the store, `envtest` for the Kubernetes API. Release resources with `t.Cleanup`. +- A test that skips on a missing precondition must fail on it in CI instead — + a skipped test and a passing one are the same exit code. Resolve strictness + through a named predicate (`dockerenv.Required()`), and prove the strict + branch red before merging. See + [CI fail-closed preconditions](dev/best-practices/ci-fail-closed.md). ## TODOs diff --git a/docs/dev/best-practices/ci-fail-closed.md b/docs/dev/best-practices/ci-fail-closed.md new file mode 100644 index 000000000..d6b2bba34 --- /dev/null +++ b/docs/dev/best-practices/ci-fail-closed.md @@ -0,0 +1,110 @@ +# CI Fail-Closed Preconditions + +How to write a test that skips locally when a dependency is absent, but fails +in CI. One code path, strictness resolved from the environment. + +Counterpart to [Code Style Guide](../../code-style-guide.md) § Testing. + +## Why + +A test needing Docker, a cluster, or an artifact directory has three options: + +| | Locally | In CI | +|---|---|---| +| Hard-fail everywhere | Unrunnable without setup | Correct | +| Skip everywhere | Runnable | **Green having run nothing** | +| Skip locally, fail in CI | Runnable | Correct | + +A skip and a pass are the same exit code. ~280 PostgreSQL-backed tests once +skipped when their testcontainer failed to start, including a 2,800-line store +contract, and the job stayed green. + +## Convention + +Resolve strictness in one named function. GitHub Actions sets `CI=true`. + +### Go + +Reference implementation, `cmd/ateapi/internal/store/dockerenv.Required()`: + +```go +func Required() bool { + return os.Getenv("CI") == "true" || os.Getenv("REQUIRE_DOCKER") == "true" +} +``` + +Callers branch on it (`storetest.go:187-192`, `atepg/main_test.go:105-110`): + +```go +if containerErr != nil { + if dockerenv.Required() { + t.Fatalf("PostgreSQL testcontainer unavailable and required "+ + "(CI or REQUIRE_DOCKER is set): %v", containerErr) + } + t.Skipf("PostgreSQL testcontainer unavailable (requires Docker): %v", containerErr) +} +``` + +Both messages name the cause; the fatal one also names what made it fatal. + +### Bash + +```bash +if [[ -z "${E2E_JUNIT_FILE:-}" && "${CI:-}" == "true" ]]; then + echo "run-e2e.sh: E2E_JUNIT_FILE must be set when CI=true." >&2 + echo " Set it to a path unique to this run, e.g." >&2 + echo " E2E_JUNIT_FILE=\"\${ARTIFACTS}/e2e-gvisor.xml\"" >&2 + exit 1 +fi +``` + +## Rules + +1. **One named predicate.** `Required()`, `inCI()`. An inline `os.Getenv("CI")` + cannot be audited or changed in one place. +2. **Make the strict branch reachable locally.** Pair `CI` with an override: + `REQUIRE_DOCKER=true`, or a flag such as junittool's `-reject-duplicate`. +3. **Failures state the fix.** Name the variable to set and a valid value. + Assume the reader has not seen this check before. +4. **Never warn instead of failing.** Warnings in a green run are not read. + +## Verifying both branches + +Only the permissive branch runs by default. Force the strict branch before +merging: + +```bash +# Docker precondition, without Docker +sudo systemctl stop docker.socket docker.service +CI=true go test ./cmd/ateapi/internal/store/... # expect FAIL, not SKIP + +# JUnit precondition, unset +CI=true hack/run-e2e.sh ./internal/e2e/suites/example # expect exit 1 + +# Duplicate registration +CI=true go -C tools/junittool run . register /tmp/a.xml +CI=true go -C tools/junittool run . register /tmp/a.xml # expect exit 1 +``` + +Two traps that have produced false passes: + +- `DOCKER_HOST=/nonexistent` does not disable Docker. testcontainers walks a + six-source resolution chain and reaches the real daemon. +- `systemctl stop docker` leaves `docker.socket` active, which restarts the + daemon on the next connection. Stop both units. + +## When not to use this + +- **The precondition should always hold.** Absence is a bug: fail everywhere. +- **The test can assert it instead.** A skip on cluster state usually means a + missing precondition check in the harness. +- **The test can create what it needs.** Do that rather than branching. + +## Current users + +`dockerenv.Required()` is the reference implementation. The same shape is in +`hack/run-e2e.sh` and `hack/run-root-tests.sh` (a JUnit path is mandatory in +CI) and `tools/junittool` (re-registering a path is fatal in CI). + +`git grep -n 'CI.*==.*true'` finds the current set. Not maintained as an +inventory. diff --git a/hack/run-e2e.sh b/hack/run-e2e.sh index a081cec92..9ed696e9f 100755 --- a/hack/run-e2e.sh +++ b/hack/run-e2e.sh @@ -148,12 +148,28 @@ test_argv+=(-args --e2e) test_argv+=(${extra_e2e_args[@]+"${extra_e2e_args[@]}"}) test_argv+=(${e2e_args[@]+"${e2e_args[@]}"}) +# CI requires a JUnit file: it checks that every test run reported tests, and a +# run without one cannot be checked. Optional locally, where this stays a plain +# `go test`. +if [[ -z "${E2E_JUNIT_FILE:-}" && "${CI:-}" == "true" ]]; then + echo "run-e2e.sh: E2E_JUNIT_FILE must be set when CI=true." >&2 + echo " Set it to a path unique to this run, e.g." >&2 + echo " E2E_JUNIT_FILE=\"\${ARTIFACTS}/e2e-gvisor.xml\"" >&2 + echo " CI verifies each run reported at least one test; a run that writes" >&2 + echo " no JUnit file cannot be verified." >&2 + exit 1 +fi + # E2E_JUNIT_FILE opts into a machine-readable record of the run: the XML for # report consumers, the JSON event stream for failure analysis. Unset, this is a # plain go test and gotestsum is never built, so a local run needs no toolchain # beyond go itself. if [[ -n "${E2E_JUNIT_FILE:-}" ]]; then mkdir -p "$(dirname "${E2E_JUNIT_FILE}")" + # Claim the path before running, so a run killed part-way still owes a file + # that `junittool verify` will report as missing. junittool rejects a path + # another run already claimed, which would otherwise be overwritten. + go -C "${ROOT}/tools/junittool" run . register "${E2E_JUNIT_FILE}" exec "${ROOT}/hack/run-tool.sh" gotestsum \ --junitfile "${E2E_JUNIT_FILE}" \ --jsonfile "${E2E_JUNIT_FILE%.xml}.json" \ diff --git a/hack/run-root-tests.sh b/hack/run-root-tests.sh index b1b8eb534..f9b354f7e 100755 --- a/hack/run-root-tests.sh +++ b/hack/run-root-tests.sh @@ -51,8 +51,21 @@ test_args=(-count=1 -timeout "${ROOT_TEST_TIMEOUT:-10m}" "$@") # that under sudo would leave root-owned entries in the user's build cache. # The runner is only ever a prefix — the privilege dispatch below is unchanged, # because a root-gated package run without root self-skips and reports success. +# CI requires a JUnit file; see hack/run-e2e.sh for the same precondition. +if [[ -z "${ROOT_JUNIT_FILE:-}" && "${CI:-}" == "true" ]]; then + echo "run-root-tests.sh: ROOT_JUNIT_FILE must be set when CI=true." >&2 + echo " Set it to a path unique to this run, e.g." >&2 + echo " ROOT_JUNIT_FILE=\"\${ARTIFACTS}/root.xml\"" >&2 + echo " CI verifies each run reported at least one test; a run that writes" >&2 + echo " no JUnit file cannot be verified." >&2 + exit 1 +fi + if [[ -n "${ROOT_JUNIT_FILE:-}" ]]; then mkdir -p "$(dirname "${ROOT_JUNIT_FILE}")" + # Claim the path before running. Runs as the invoking user, before the sudo + # dispatch below, so the manifest is not left root-owned. + go -C "${ROOT}/tools/junittool" run . register "${ROOT_JUNIT_FILE}" runner=("$("${ROOT}/hack/run-tool.sh" --print-bin-path gotestsum)" --junitfile "${ROOT_JUNIT_FILE}" --jsonfile "${ROOT_JUNIT_FILE%.xml}.json" diff --git a/tools/junittool/go.mod b/tools/junittool/go.mod new file mode 100644 index 000000000..0ec975ac9 --- /dev/null +++ b/tools/junittool/go.mod @@ -0,0 +1,3 @@ +module github.com/agent-substrate/substrate/tools/junittool + +go 1.27.0 diff --git a/tools/junittool/junit.go b/tools/junittool/junit.go new file mode 100644 index 000000000..135bb0fdd --- /dev/null +++ b/tools/junittool/junit.go @@ -0,0 +1,162 @@ +// Copyright 2026 Google LLC +// +// 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 main + +import ( + "encoding/xml" + "fmt" + "io" + "os" + "path/filepath" + "sort" + "text/tabwriter" +) + +// testSuites matches the JUnit gotestsum writes; only counts are read. Summed +// from the testsuite children, not the root attribute, so a root disagreeing +// with its children cannot inflate the total. +type testSuites struct { + Suites []struct { + Name string `xml:"name,attr"` + Tests int `xml:"tests,attr"` + Failures int `xml:"failures,attr"` + Errors int `xml:"errors,attr"` + Skipped int `xml:"skipped,attr"` + } `xml:"testsuite"` +} + +type totals struct { + tests, failures, errors, skipped int +} + +func (t *totals) add(o totals) { + t.tests += o.tests + t.failures += o.failures + t.errors += o.errors + t.skipped += o.skipped +} + +func parse(path string) (totals, error) { + raw, err := os.ReadFile(path) + if err != nil { + return totals{}, err + } + var ts testSuites + if err := xml.Unmarshal(raw, &ts); err != nil { + return totals{}, fmt.Errorf("parsing %s: %w", path, err) + } + var out totals + for _, s := range ts.Suites { + out.add(totals{tests: s.Tests, failures: s.Failures, errors: s.Errors, skipped: s.Skipped}) + } + return out, nil +} + +// expand resolves each argument to JUnit files. A directory contributes the +// *.xml inside it; anything else passes through literally, so a missing file +// reaches parse() and is reported. +func expand(args []string) ([]string, error) { + var out []string + for _, a := range args { + info, err := os.Stat(a) + if err != nil || !info.IsDir() { + out = append(out, a) + continue + } + matches, err := filepath.Glob(filepath.Join(a, "*.xml")) + if err != nil { + return nil, fmt.Errorf("expanding %s: %w", a, err) + } + sort.Strings(matches) + out = append(out, matches...) + } + return out, nil +} + +// row is one file's contribution. err set means the counts are meaningless. +type row struct { + path string + t totals + err error +} + +// result is the verify decision, separate from reporting so it can be tested +// without capturing output. +type result struct { + rows []row + grand totals + empty []string + unreadable []string +} + +// ok reports whether every file was readable and met the minimum. +func (r result) ok() bool { return len(r.empty) == 0 && len(r.unreadable) == 0 } + +// evaluate reads and classifies every path. min applies per file, never to the +// total, so one empty file fails even when others are full. Order matches the +// manifest. +func evaluate(paths []string, min int) result { + var res result + for _, p := range paths { + t, err := parse(p) + res.rows = append(res.rows, row{path: p, t: t, err: err}) + if err != nil { + res.unreadable = append(res.unreadable, p) + continue + } + res.grand.add(t) + if t.tests < min { + res.empty = append(res.empty, p) + } + } + return res +} + +// write renders the per-file table to out and the reasons for failure to errOut. +func (r result) write(out, errOut io.Writer, min int) { + w := tabwriter.NewWriter(out, 0, 0, 2, ' ', 0) + fmt.Fprintln(w, "FILE\tTESTS\tFAILURES\tERRORS\tSKIPPED") + for _, row := range r.rows { + if row.err != nil { + fmt.Fprintf(w, "%s\t-\t-\t-\t-\n", row.path) + continue + } + fmt.Fprintf(w, "%s\t%d\t%d\t%d\t%d\n", row.path, row.t.tests, row.t.failures, row.t.errors, row.t.skipped) + } + fmt.Fprintf(w, "TOTAL\t%d\t%d\t%d\t%d\n", r.grand.tests, r.grand.failures, r.grand.errors, r.grand.skipped) + w.Flush() + + for _, row := range r.rows { + if row.err != nil { + fmt.Fprintf(errOut, "junittool: %v\n", row.err) + } + } + for _, p := range r.empty { + fmt.Fprintf(errOut, + "junittool: %s reported %d test(s), want at least %d. "+ + "Check the package list and -run filter of the run that wrote it.\n", + p, r.testsFor(p), min) + } +} + +// testsFor returns the count recorded for path, for error messages. +func (r result) testsFor(path string) int { + for _, row := range r.rows { + if row.path == path { + return row.t.tests + } + } + return 0 +} diff --git a/tools/junittool/junit_test.go b/tools/junittool/junit_test.go new file mode 100644 index 000000000..0c915aa21 --- /dev/null +++ b/tools/junittool/junit_test.go @@ -0,0 +1,216 @@ +// Copyright 2026 Google LLC +// +// 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 main + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// write puts content at dir/name and returns the path. +func write(t *testing.T, dir, name, content string) string { + t.Helper() + p := filepath.Join(dir, name) + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + t.Fatalf("writing %s: %v", p, err) + } + return p +} + +const twoSuites = ` + + +` + +// What gotestsum emits when a package matched no tests: suite present, count +// zero. Distinct from a document with no suites at all. +const zeroTests = ` + +` + +func TestParse(t *testing.T) { + dir := t.TempDir() + for _, tc := range []struct { + name string + content string + want totals + wantErr bool + }{ + {name: "two suites are summed", content: twoSuites, + want: totals{tests: 10, failures: 1, errors: 0, skipped: 2}}, + {name: "zero-test suite", content: zeroTests}, + {name: "no suites at all", content: ``}, + // Children win over the root attribute. + {name: "root attribute ignored in favour of children", + content: ``}, + {name: "malformed", content: "not xml at all", wantErr: true}, + {name: "empty file", content: "", wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := parse(write(t, dir, strings.ReplaceAll(tc.name, " ", "_")+".xml", tc.content)) + if tc.wantErr { + if err == nil { + t.Fatalf("parse() = %+v, want error", got) + } + return + } + if err != nil { + t.Fatalf("parse() error = %v", err) + } + if got != tc.want { + t.Errorf("parse() = %+v, want %+v", got, tc.want) + } + }) + } +} + +func TestParseMissingFile(t *testing.T) { + if _, err := parse(filepath.Join(t.TempDir(), "absent.xml")); err == nil { + t.Fatal("parse() on a missing file = nil error, want error") + } +} + +func TestExpand(t *testing.T) { + dir := t.TempDir() + a := write(t, dir, "a.xml", twoSuites) + b := write(t, dir, "b.xml", twoSuites) + write(t, dir, "notes.txt", "ignored") + empty := filepath.Join(dir, "empty") + if err := os.Mkdir(empty, 0o755); err != nil { + t.Fatal(err) + } + + t.Run("directory expands to its xml, sorted", func(t *testing.T) { + got, err := expand([]string{dir}) + if err != nil { + t.Fatalf("expand() error = %v", err) + } + if strings.Join(got, ",") != strings.Join([]string{a, b}, ",") { + t.Errorf("expand() = %v, want [%s %s]", got, a, b) + } + }) + + t.Run("non-xml files are not picked up", func(t *testing.T) { + got, _ := expand([]string{dir}) + for _, p := range got { + if filepath.Ext(p) != ".xml" { + t.Errorf("expand() included %s, want only .xml", p) + } + } + }) + + t.Run("empty directory contributes nothing", func(t *testing.T) { + got, err := expand([]string{empty}) + if err != nil || len(got) != 0 { + t.Errorf("expand(empty dir) = %v, %v; want no paths and no error", got, err) + } + }) + + // Passed through, not dropped, so evaluate() reports it missing. + t.Run("missing path passes through literally", func(t *testing.T) { + missing := filepath.Join(dir, "absent.xml") + got, err := expand([]string{missing}) + if err != nil || len(got) != 1 || got[0] != missing { + t.Errorf("expand(missing) = %v, %v; want [%s]", got, err, missing) + } + }) +} + +func TestEvaluate(t *testing.T) { + dir := t.TempDir() + good := write(t, dir, "good.xml", twoSuites) + good2 := write(t, dir, "good2.xml", twoSuites) + zero := write(t, dir, "zero.xml", zeroTests) + bad := write(t, dir, "bad.xml", "not xml") + missing := filepath.Join(dir, "absent.xml") + + for _, tc := range []struct { + name string + paths []string + min int + wantOK bool + wantEmpty int + wantUnreadable int + wantTotalTests int + }{ + {name: "all populated", paths: []string{good, good2}, min: 1, + wantOK: true, wantTotalTests: 20}, + {name: "single zero-test file", paths: []string{zero}, min: 1, wantEmpty: 1}, + // Why min is per-file: tests in one file must not cover for another + // that reported none. + {name: "one empty file among populated ones", + paths: []string{good, zero}, min: 1, wantEmpty: 1, wantTotalTests: 10}, + {name: "missing file", paths: []string{missing}, min: 1, wantUnreadable: 1}, + {name: "malformed file", paths: []string{bad}, min: 1, wantUnreadable: 1}, + {name: "missing and empty are counted separately", + paths: []string{missing, zero}, min: 1, wantEmpty: 1, wantUnreadable: 1}, + {name: "min above the reported count fails a populated file", + paths: []string{good}, min: 11, wantEmpty: 1, wantTotalTests: 10}, + {name: "min at the reported count passes", + paths: []string{good}, min: 10, wantOK: true, wantTotalTests: 10}, + {name: "no paths is vacuously ok", paths: nil, min: 1, wantOK: true}, + } { + t.Run(tc.name, func(t *testing.T) { + res := evaluate(tc.paths, tc.min) + if res.ok() != tc.wantOK { + t.Errorf("ok() = %v, want %v (empty=%v unreadable=%v)", + res.ok(), tc.wantOK, res.empty, res.unreadable) + } + if len(res.empty) != tc.wantEmpty { + t.Errorf("empty = %v, want %d entries", res.empty, tc.wantEmpty) + } + if len(res.unreadable) != tc.wantUnreadable { + t.Errorf("unreadable = %v, want %d entries", res.unreadable, tc.wantUnreadable) + } + if res.grand.tests != tc.wantTotalTests { + t.Errorf("grand.tests = %d, want %d", res.grand.tests, tc.wantTotalTests) + } + if len(res.rows) != len(tc.paths) { + t.Errorf("rows = %d, want one per path (%d)", len(res.rows), len(tc.paths)) + } + }) + } +} + +// The failure must name the file; otherwise the reader compares them by hand. +func TestWriteNamesTheOffendingFile(t *testing.T) { + dir := t.TempDir() + zero := write(t, dir, "zero.xml", zeroTests) + good := write(t, dir, "good.xml", twoSuites) + + var out, errOut strings.Builder + evaluate([]string{good, zero}, 1).write(&out, &errOut, 1) + + if !strings.Contains(errOut.String(), zero) { + t.Errorf("stderr does not name the empty file %s:\n%s", zero, errOut.String()) + } + if strings.Contains(errOut.String(), good) { + t.Errorf("stderr names the passing file %s, which is noise:\n%s", good, errOut.String()) + } + if !strings.Contains(out.String(), "TOTAL") { + t.Errorf("stdout has no TOTAL row:\n%s", out.String()) + } +} + +func TestTotalsAdd(t *testing.T) { + got := totals{tests: 1, failures: 2, errors: 3, skipped: 4} + got.add(totals{tests: 10, failures: 20, errors: 30, skipped: 40}) + want := totals{tests: 11, failures: 22, errors: 33, skipped: 44} + if got != want { + t.Errorf("add() = %+v, want %+v", got, want) + } +} diff --git a/tools/junittool/main.go b/tools/junittool/main.go new file mode 100644 index 000000000..770c807b2 --- /dev/null +++ b/tools/junittool/main.go @@ -0,0 +1,132 @@ +// Copyright 2026 Google LLC +// +// 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. + +// Command junittool records the JUnit files a CI run will write, then checks +// they reported tests. +// +// junittool register record a file this run will write +// junittool verify -manifest require every recorded file to hold tests +// +// An exit code cannot distinguish "all tests passed" from "no test ran". +// `go test ./...` passes the e2e suites with no cluster, and a package filter +// that stops matching reports success having run nothing. +// +// Registering before the run has two effects: the expected set comes from the +// runs themselves, so adding a run needs no change to verify; and a run that +// died part-way is distinguishable from one that never existed. +// +// No dependencies — it runs on the critical path of every CI job. +package main + +import ( + "flag" + "fmt" + "os" +) + +const usage = `usage: + junittool register [-manifest FILE] [-reject-duplicate] + junittool verify (-manifest FILE | ...) [-min N] +` + +func main() { + if len(os.Args) < 2 { + fmt.Fprint(os.Stderr, usage) + os.Exit(2) + } + switch os.Args[1] { + case "register": + os.Exit(runRegister(os.Args[2:])) + case "verify": + os.Exit(runVerify(os.Args[2:])) + case "-h", "--help", "help": + fmt.Print(usage) + os.Exit(0) + default: + fmt.Fprintf(os.Stderr, "junittool: unknown subcommand %q\n%s", os.Args[1], usage) + os.Exit(2) + } +} + +// inCI follows cmd/ateapi/internal/store/dockerenv.Required: strict in CI, +// permissive locally, where re-running into the same file is ordinary. +// See docs/dev/best-practices/ci-fail-closed.md. +func inCI() bool { return os.Getenv("CI") == "true" } + +func runRegister(args []string) int { + fs := flag.NewFlagSet("register", flag.ExitOnError) + manifest := fs.String("manifest", "", + "manifest to append to (default: expected-junit.txt beside the entry)") + reject := fs.Bool("reject-duplicate", inCI(), + "treat a repeat registration as an error (defaults true when CI=true)") + _ = fs.Parse(args) + + if fs.NArg() != 1 { + fmt.Fprint(os.Stderr, usage) + return 2 + } + entry := fs.Arg(0) + path := *manifest + if path == "" { + path = defaultManifest(entry) + } + if err := register(path, entry, *reject); err != nil { + fmt.Fprintf(os.Stderr, "junittool register: %v\n", err) + return 1 + } + return 0 +} + +func runVerify(args []string) int { + fs := flag.NewFlagSet("verify", flag.ExitOnError) + min := fs.Int("min", 1, "minimum number of test cases EACH file must report") + manifest := fs.String("manifest", "", "manifest listing the JUnit paths that were registered") + _ = fs.Parse(args) + + var paths []string + switch { + case *manifest != "": + if fs.NArg() > 0 { + fmt.Fprintln(os.Stderr, "junittool verify: -manifest takes no positional arguments") + return 2 + } + var err error + if paths, err = readManifest(*manifest); err != nil { + fmt.Fprintf(os.Stderr, "junittool verify: %v\n", err) + return 1 + } + if len(paths) == 0 { + fmt.Fprintf(os.Stderr, + "junittool verify: %s is empty. No test run registered a JUnit file, "+ + "so nothing ran.\n", *manifest) + return 1 + } + case fs.NArg() > 0: + var err error + if paths, err = expand(fs.Args()); err != nil { + fmt.Fprintf(os.Stderr, "junittool verify: %v\n", err) + return 1 + } + default: + fmt.Fprint(os.Stderr, usage) + return 2 + } + + res := evaluate(paths, *min) + res.write(os.Stdout, os.Stderr, *min) + if !res.ok() { + return 1 + } + return 0 +} diff --git a/tools/junittool/manifest.go b/tools/junittool/manifest.go new file mode 100644 index 000000000..c1064cb6d --- /dev/null +++ b/tools/junittool/manifest.go @@ -0,0 +1,104 @@ +// Copyright 2026 Google LLC +// +// 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 main + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" +) + +// errDuplicateEntry is returned when a path is already recorded. Registration +// is the only point where both runs are still visible; after the overwrite the +// first has left no trace. +var errDuplicateEntry = errors.New( + "already registered by an earlier run; give each run its own path, " + + "or the second overwrites the first and only the second is verified") + +// readManifest returns the recorded paths, deduplicated, in first-seen order. +// Duplicates are collapsed, not rejected: a run repeated locally records its +// path again. Rejecting one is register's job, where a repeat and a collision +// are still distinguishable. +func readManifest(path string) ([]string, error) { + raw, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("reading manifest: %w", err) + } + return dedupe(strings.Split(string(raw), "\n")), nil +} + +// dedupe trims, drops blanks, and keeps the first occurrence of each entry. +func dedupe(lines []string) []string { + seen := map[string]bool{} + var out []string + for _, line := range lines { + line = strings.TrimSpace(line) + if line == "" || seen[line] { + continue + } + seen[line] = true + out = append(out, line) + } + return out +} + +// register appends entry to the manifest at path, creating it if absent. With +// rejectDuplicate, a repeat is an error rather than a no-op. Callers register +// before running, so a run killed part-way still has a record, and verify +// reports its file as missing. +func register(path, entry string, rejectDuplicate bool) error { + entry = strings.TrimSpace(entry) + if entry == "" { + return errors.New("refusing to register an empty entry") + } + if strings.ContainsAny(entry, "\n\r") { + return fmt.Errorf("entry %q contains a newline: one path per line", entry) + } + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return fmt.Errorf("creating manifest directory: %w", err) + } + + existing, err := os.ReadFile(path) + if err != nil && !os.IsNotExist(err) { + return fmt.Errorf("reading manifest: %w", err) + } + for _, e := range dedupe(strings.Split(string(existing), "\n")) { + if e != entry { + continue + } + if rejectDuplicate { + return fmt.Errorf("%s: %w", entry, errDuplicateEntry) + } + return nil + } + + f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) + if err != nil { + return fmt.Errorf("opening manifest: %w", err) + } + defer f.Close() + if _, err := fmt.Fprintln(f, entry); err != nil { + return fmt.Errorf("appending to manifest: %w", err) + } + return nil +} + +// defaultManifest puts the manifest beside the JUnit file, so runs sharing an +// artifact directory share a manifest. +func defaultManifest(entry string) string { + return filepath.Join(filepath.Dir(entry), "expected-junit.txt") +} diff --git a/tools/junittool/manifest_test.go b/tools/junittool/manifest_test.go new file mode 100644 index 000000000..981ef7665 --- /dev/null +++ b/tools/junittool/manifest_test.go @@ -0,0 +1,200 @@ +// Copyright 2026 Google LLC +// +// 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 main + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +// lines returns the manifest's contents split for comparison. +func lines(t *testing.T, path string) []string { + t.Helper() + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading %s: %v", path, err) + } + var out []string + for _, l := range strings.Split(string(raw), "\n") { + if l != "" { + out = append(out, l) + } + } + return out +} + +func TestRegisterCreatesAndAppends(t *testing.T) { + m := filepath.Join(t.TempDir(), "nested", "expected-junit.txt") + + // Parent directory does not exist yet: registration can precede any other + // write to the artifact directory. + if err := register(m, "/a/one.xml", true); err != nil { + t.Fatalf("first register: %v", err) + } + if err := register(m, "/a/two.xml", true); err != nil { + t.Fatalf("second register: %v", err) + } + got := lines(t, m) + want := []string{"/a/one.xml", "/a/two.xml"} + if strings.Join(got, ",") != strings.Join(want, ",") { + t.Errorf("manifest = %v, want %v", got, want) + } +} + +func TestRegisterDuplicate(t *testing.T) { + for _, tc := range []struct { + name string + rejectDuplicate bool + wantErr bool + wantLines int + }{ + // Two runs sharing a path: the second overwrites the first, and only + // the second is verified. + {name: "rejected when asked", rejectDuplicate: true, wantErr: true, wantLines: 1}, + // A run repeated locally registers again; not an error, and must not + // duplicate the entry. + {name: "tolerated and not duplicated", rejectDuplicate: false, wantLines: 1}, + } { + t.Run(tc.name, func(t *testing.T) { + m := filepath.Join(t.TempDir(), "expected-junit.txt") + if err := register(m, "/a/one.xml", tc.rejectDuplicate); err != nil { + t.Fatalf("first register: %v", err) + } + err := register(m, "/a/one.xml", tc.rejectDuplicate) + if tc.wantErr { + if !errors.Is(err, errDuplicateEntry) { + t.Fatalf("second register error = %v, want errDuplicateEntry", err) + } + } else if err != nil { + t.Fatalf("second register: %v", err) + } + if got := lines(t, m); len(got) != tc.wantLines { + t.Errorf("manifest = %v, want %d line(s)", got, tc.wantLines) + } + }) + } +} + +// Whole-entry comparison, not substring: paths sharing a prefix are distinct. +func TestRegisterDistinguishesSimilarPaths(t *testing.T) { + m := filepath.Join(t.TempDir(), "expected-junit.txt") + for _, e := range []string{"/a/e2e.xml", "/a/e2e-microvm.xml", "/a/e2e-microvm-mitm.xml"} { + if err := register(m, e, true); err != nil { + t.Fatalf("register(%s): %v", e, err) + } + } + if got := lines(t, m); len(got) != 3 { + t.Errorf("manifest = %v, want 3 distinct entries", got) + } +} + +func TestRegisterRejectsMalformedEntries(t *testing.T) { + for _, tc := range []struct{ name, entry string }{ + {name: "empty", entry: ""}, + {name: "whitespace only", entry: " "}, + {name: "embedded newline", entry: "/a/one.xml\n/a/two.xml"}, + {name: "embedded carriage return", entry: "/a/one.xml\r/a/two.xml"}, + } { + t.Run(tc.name, func(t *testing.T) { + m := filepath.Join(t.TempDir(), "expected-junit.txt") + if err := register(m, tc.entry, true); err == nil { + t.Fatalf("register(%q) = nil, want error", tc.entry) + } + }) + } +} + +// Entries are trimmed on write, so a padded path still matches its file. +func TestRegisterTrimsEntry(t *testing.T) { + m := filepath.Join(t.TempDir(), "expected-junit.txt") + if err := register(m, " /a/one.xml ", true); err != nil { + t.Fatalf("register: %v", err) + } + if got := lines(t, m); len(got) != 1 || got[0] != "/a/one.xml" { + t.Errorf("manifest = %v, want [/a/one.xml]", got) + } + // The trimmed form is what a repeat is compared against. + if err := register(m, "/a/one.xml", true); !errors.Is(err, errDuplicateEntry) { + t.Errorf("repeat after trim = %v, want errDuplicateEntry", err) + } +} + +func TestDefaultManifest(t *testing.T) { + got := defaultManifest("/tmp/artifacts/e2e-gvisor.xml") + want := filepath.Join("/tmp/artifacts", "expected-junit.txt") + if got != want { + t.Errorf("defaultManifest() = %s, want %s", got, want) + } +} + +func TestReadManifest(t *testing.T) { + dir := t.TempDir() + for _, tc := range []struct { + name string + content string + want []string + }{ + {name: "one per line", content: "a.xml\nb.xml\n", want: []string{"a.xml", "b.xml"}}, + {name: "duplicates collapse", content: "a.xml\nb.xml\na.xml\n", want: []string{"a.xml", "b.xml"}}, + {name: "all duplicates collapse to one", content: "a.xml\na.xml\na.xml\n", want: []string{"a.xml"}}, + {name: "first occurrence sets the order", content: "b.xml\na.xml\nb.xml\n", want: []string{"b.xml", "a.xml"}}, + {name: "blank lines ignored", content: "\na.xml\n\n\nb.xml\n", want: []string{"a.xml", "b.xml"}}, + {name: "whitespace trimmed", content: " a.xml \n\tb.xml\t\n", want: []string{"a.xml", "b.xml"}}, + {name: "no trailing newline", content: "a.xml", want: []string{"a.xml"}}, + {name: "empty manifest yields nothing", content: "", want: nil}, + {name: "whitespace-only yields nothing", content: "\n \n\t\n", want: nil}, + } { + t.Run(tc.name, func(t *testing.T) { + p := filepath.Join(dir, strings.ReplaceAll(tc.name, " ", "_")+".txt") + if err := os.WriteFile(p, []byte(tc.content), 0o644); err != nil { + t.Fatal(err) + } + got, err := readManifest(p) + if err != nil { + t.Fatalf("readManifest() error = %v", err) + } + if strings.Join(got, ",") != strings.Join(tc.want, ",") { + t.Errorf("readManifest() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestReadManifestMissingFile(t *testing.T) { + if _, err := readManifest(filepath.Join(t.TempDir(), "absent.txt")); err == nil { + t.Fatal("readManifest() on a missing file = nil error, want error") + } +} + +// What register writes is what readManifest returns. +func TestRegisterThenReadRoundTrip(t *testing.T) { + m := filepath.Join(t.TempDir(), "expected-junit.txt") + want := []string{"/a/gvisor.xml", "/a/microvm.xml", "/a/mitm.xml"} + for _, e := range want { + if err := register(m, e, true); err != nil { + t.Fatalf("register(%s): %v", e, err) + } + } + got, err := readManifest(m) + if err != nil { + t.Fatalf("readManifest: %v", err) + } + if strings.Join(got, ",") != strings.Join(want, ",") { + t.Errorf("round trip = %v, want %v", got, want) + } +}