From 62601840aa7abbe9f40736f8cd732eb9f8c6b73e Mon Sep 17 00:00:00 2001 From: omar-nahhas-agent Date: Tue, 4 Aug 2026 02:05:44 +0100 Subject: [PATCH] fix(e2e): traces really are always captured now (completes DEC-073) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v2.1.4 flipped the trace default to "on" in playwright-base.config.ts and I reported DEC-073 as delivered. It was not: two other settings still forced retain-on-failure and both took precedence over the one I changed. 1. The reusable workflow hardcoded BDD_TRACE: ${{ inputs.capture-all-traces && "on" || "retain-on-failure" }} so every consumer run explicitly passed retain-on-failure into the container. 2. The BDD suite has its OWN config (e2e/bdd/playwright.config.ts) — the suite consumers actually run via run-bdd.sh — whose default was also retain-on-failure. Same duplicated-setting pattern as the collector: I fixed one copy and missed the others. Proven by share-chart sync PR #19 on v2.1.4: "copied 0 trace(s), 4 screenshot(s), 4 video(s)" — screenshots and videos flowed (the collector fix landed) while traces did not, on a fully green run. Replaces the boolean capture-all-traces flip with a trace-mode string input (default "on", accepts any Playwright mode) so there is a real escape hatch for disk-pressure cases without silently reverting the decision. capture-all-traces is kept so existing callers keep working, but it can no longer suppress traces. --- .github/workflows/plugin-e2e.yml | 21 +++++++++++++++++++-- CHANGELOG.md | 12 ++++++++++++ e2e/bdd/playwright.config.ts | 12 +++++++++--- 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/.github/workflows/plugin-e2e.yml b/.github/workflows/plugin-e2e.yml index 4833936..1f7a5a7 100644 --- a/.github/workflows/plugin-e2e.yml +++ b/.github/workflows/plugin-e2e.yml @@ -30,10 +30,22 @@ on: type: string default: "tests/_testkit" capture-all-traces: - description: "Capture a Playwright trace for EVERY scenario (not just failures). Default: failing only." + # Kept for compatibility with existing callers (the caller template passes + # it from a workflow_dispatch input). It is now a no-op in the default + # direction: traces are ALWAYS captured (DEC-073). Setting it false no + # longer suppresses them — use trace-mode if you really want that. + description: "Deprecated — traces are always captured (DEC-073). Use trace-mode to override." required: false type: boolean default: false + trace-mode: + # Escape hatch for the rare case where trace capture itself is the + # problem (disk pressure on a huge suite). Accepts any Playwright trace + # mode: on | retain-on-failure | on-first-retry | off. + description: "Playwright trace mode. Default 'on' (DEC-073 — a green run must leave evidence too)." + required: false + type: string + default: "on" secrets: # DEC-075: all three are now OPTIONAL. They were `required: true` back when # the devkit was a PRIVATE repo and the submodule could only be cloned with @@ -227,7 +239,12 @@ jobs: A0_IMAGE: ${{ steps.resolve.outputs.a0_image }} A0_POD_ENV: ${{ steps.resolve.outputs.pod_env }} GHCR_TOKEN: ${{ secrets.GHCR_PULL_TOKEN }} - BDD_TRACE: ${{ inputs.capture-all-traces && 'on' || 'retain-on-failure' }} + # DEC-073: traces are ALWAYS captured, so a green run leaves evidence to + # diff a later regression against. This line used to hardcode + # 'retain-on-failure' unless capture-all-traces was set, which silently + # overrode the config default and meant every consumer's green e2e + # shipped 0 traces — the exact defect DEC-073 was written to fix. + BDD_TRACE: ${{ inputs.trace-mode }} run: | set -euo pipefail mkdir -p dist "$GITHUB_WORKSPACE/artifacts" diff --git a/CHANGELOG.md b/CHANGELOG.md index 154dccc..4222f56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,18 @@ the frozen Make target contract (SPEC Appendix E.1), the reusable workflow input `Makefile.devkit` / `.devkit.yml` interface, or a tightening of the enforcement gates. **MINOR** = new backward-compatible targets/checks/assets. **PATCH** = fixes that don't change the contract. +## Unreleased + +- **Fix: traces still were not captured on green runs in consumers** (DEC-073, completing it). + v2.1.4 changed the trace default to `on` in `playwright-base.config.ts`, but two other places + still forced `retain-on-failure` and won: the reusable workflow hardcoded + `BDD_TRACE: ${{ inputs.capture-all-traces && 'on' || 'retain-on-failure' }}`, and the BDD suite + keeps its **own** `e2e/bdd/playwright.config.ts` whose default was also `retain-on-failure`. + Consumers therefore still shipped `copied 0 trace(s)` on a green run — the exact defect DEC-073 + was written to fix, reported as fixed while two of the three settings still disagreed. + New `trace-mode` input (default `on`) replaces the boolean flip; `capture-all-traces` is kept for + caller compatibility but is now a no-op in the default direction. + ## v2.1.4 — 2026-08-04 - **Fix: Dependabot PRs could never go green** (DEC-075). The reusable workflow declared its diff --git a/e2e/bdd/playwright.config.ts b/e2e/bdd/playwright.config.ts index 3320949..3d42ad0 100644 --- a/e2e/bdd/playwright.config.ts +++ b/e2e/bdd/playwright.config.ts @@ -20,12 +20,18 @@ const RED_PROOF = process.env.BDD_SKIP_INSTALL === "1"; export default defineConfig({ testDir, reporter: [["list"]], timeout: 120000, workers: 1, fullyParallel: false, // trace = the rich single-file artifact (network + DOM snapshots + console + video + timeline), - // viewable via `npx playwright show-trace` / trace.playwright.dev. Default: only failing scenarios - // (retain-on-failure); set BDD_TRACE=on (the workflow's capture-all-traces dispatch input) for every one. + // viewable via `npx playwright show-trace` / trace.playwright.dev. + // + // Default "on" (DEC-073): a GREEN run must leave evidence too, otherwise there + // is no baseline to diff a later regression against and a scenario can only be + // diagnosed after it has already broken. This default was "retain-on-failure", + // which is the same defect DEC-073 fixed in playwright-base.config.ts — the + // BDD suite kept its own copy of the setting and was missed. Override with + // BDD_TRACE / the workflow's trace-mode input. use: { baseURL: process.env.A0_BASE || "http://localhost:8099", video: RED_PROOF ? "off" : "on", screenshot: RED_PROOF ? "off" : "on", - trace: RED_PROOF ? "off" : ((process.env.BDD_TRACE as any) || "retain-on-failure"), + trace: RED_PROOF ? "off" : ((process.env.BDD_TRACE as any) || "on"), }, });