diff --git a/docs/developers/reference/test-hygiene.md b/docs/developers/reference/test-hygiene.md index c869078ed..37a7a280a 100644 --- a/docs/developers/reference/test-hygiene.md +++ b/docs/developers/reference/test-hygiene.md @@ -105,9 +105,88 @@ purpose — see "What NOT to convert"): `PREDICATE_TO_COUNT_FIELD` map size is *itself* the canary that fails CI when a projection predicate is added without a count column. -## Related +## Flake audit (#2813) — "CI flake" that was a deterministic signature regression + +> **Filed under issue [#2813](https://github.com/sap-tutorials/tutorials-ims/issues/2813).** +> Same class as #1089 (a source-of-truth shape grew in one PR; a test in a +> *different* file encoded the old shape), one layer up: a **function signature** +> widened, and a stale test kept calling the old shape. + +### What happened + +`scripts/__tests__/backfill-images-slug-filter.test.ts > restricts enumeration +to the target slug` read as a CI flake (`expected [] to deeply equal ['beta']`, +intermittent, only in the 4-worker forks pool, survived `retry:1`). PR #2812 +diagnosed it as a non-hermetic `beforeAll` fixture + async-import-shim timing +race and shipped a `beforeEach` rewrite + a shim-resolution guard. + +**That diagnosis was wrong.** The test fails **1/1 locally in isolation** — it is +deterministic. Root cause: PR #2805 (`48da7d835`) widened + +```diff +- export function collectImageUrls(publicDir: string, slug?: string) ++ export function collectImageUrls(publicDir: string, slugs?: string[]) +``` + +and updated the production call site and the *new* `.js` sibling test +(`test/unit/backfill-images-slug-filter.test.js`, which calls with arrays), but +left this older `.ts` test passing a bare string `'beta'`. The impl does +`for (const s of slugs?.length ? slugs : readdir(...))`; a string has truthy +`.length` and **iterates by character** (`'b','e','t','a'`) → no matching dirs → +empty map. `retry:1` then re-ran it, it lost again, and the plausible +"Node22/Vitest-4 forks pool" story made a dead signature read as noise. + +#2812's guard (`expect(typeof collectImageUrls).toBe('function')`) was a **no-op** +for this bug — the binding *was* a function; the *call shape* was wrong. + +### The fix (this PR) + +- Call sites pass arrays (`['beta']`, `['does-not-exist']`), matching the #2805 + signature. +- The `beforeEach` guard now asserts the **call contract**, not just the binding: + `expect(collectImageUrls(publicDir, ['beta']).size).toBe(1)`. A future + signature/shape drift (string vs `string[]`, renamed param) fails loudly at the + guard line instead of as a cryptic empty-map assertion. Verified: reverting any + call to a bare string fails all 5 tests *at the guard*, not as a no-op. + +### retry:1 policy decision — **keep it, scoped to the real failure** + +The scope step the issue asked for (surface every retry-dependent pass) was run +against the **actual CI environment** by scanning the unit-tier log of the DEV +merge `fdceca151` (the commit that *contains* #2812 and still went red). Result: + +- **Exactly one** `(retry xN)` marker in the whole 1490-file / 11,541-test run — + `restricts enumeration to the target slug`, and it **lost both attempts** + (`× … (retry x1)`). Summary: `1 failed | 1487 passed | 2 skipped`. +- There is **no swarm of timing flakes** hiding behind `retry:1`. The single + retried test was the deterministic regression above. + +So `retry:1` did **not** mask flake debt here — it masked a *real red*, by making +a deterministic failure look stochastic. **Recommendation: keep `retry:1`** (it +genuinely absorbs the cold-boot starvation blip the config comment documents — +the smoke tier has the same lever for the same reason) **but treat any +retried-*then-failed* test as a deterministic bug first**, not a flake: reproduce +it in isolation (`npx vitest run `) before reaching for "it's the forks +pool". A test that needs a retry to go green, or fails both tries, is signal, not +noise. No config change is warranted on this evidence; the fix is diagnostic +discipline, codified here. + +### Structural note — the flake *shape* is pervasive, so don't mechanically guard it + +An audit of the suite for #2812's structural shape (per-file `beforeAll`/`afterAll` +shared fixture **and** reaching a module through the workspace-first async-import +shim) found the shape is **everywhere**, not rare: **119** modules contain the +shim (`try { await import('@tutorials/…') } catch { await import() }`), +**615** `test/**` files use `beforeAll`/`afterAll`, and **432** boot a shared CAP +fixture via module-level `cds.test()`. Hundreds of files have *both* ingredients +and are green. Bolting a contract guard onto all of them would be churn against a +hypothesis this audit **disproved** — the shim/`beforeAll` shape was not the cause +of the one real failure. Add a contract guard where a test's call shape can drift +from its SUT's signature (the pattern above), not everywhere the shim appears. + - Issue [#1089](https://github.com/sap-tutorials/tutorials-ims/issues/1089) — audit + convert - PR [#1088](https://github.com/sap-tutorials/tutorials-ims/pull/1088) — the fix that motivated this pattern - Issue [#1043](https://github.com/sap-tutorials/tutorials-ims/issues/1043) — CSV-column parallel gotcha (different mechanism, same class) +- Issue [#2813](https://github.com/sap-tutorials/tutorials-ims/issues/2813) — flake audit: the "CI flake" that was a deterministic `string`→`string[]` signature regression (see section above); PR [#2812](https://github.com/sap-tutorials/tutorials-ims/pull/2812) — the misdiagnosed first attempt - [`cap-cds-gotchas.md`](./cap-cds-gotchas.md) — general CAP/CDS test-hygiene guidance diff --git a/scripts/__tests__/backfill-images-slug-filter.test.ts b/scripts/__tests__/backfill-images-slug-filter.test.ts index f51533449..ba57347b1 100644 --- a/scripts/__tests__/backfill-images-slug-filter.test.ts +++ b/scripts/__tests__/backfill-images-slug-filter.test.ts @@ -12,14 +12,22 @@ function page(slug: string, img: string): string { } describe('backfill-images --slug filter', () => { - // Per-test (not per-file) fixtures. `collectImageUrls` reaches `extractImgCdnUrls` - // through the workspace-first async-import shim in srv/lib/image-warm-utils.js; - // under the CI forks pool with `retry:1`, a per-file `beforeAll` left the fixture - // and the lazily-resolved binding exposed to cross-attempt/worker timing, which - // surfaced as a non-reproducible `[] !== ['beta']` on retry (#2808 CI flake — - // code was correct, the test was non-hermetic). beforeEach rebuilds the fixture - // for every attempt; the fixture guard below turns any future silent `[]` from a - // broken shim resolution into a loud, diagnosable failure instead of a flake. + // Per-test (not per-file) fixtures, rebuilt for every attempt. + // + // History: #2812 recast the CI failure here (`[] !== ['beta']`, retried-and- + // still-red) as a forks-pool timing flake and moved the fixtures to beforeEach + // + added a shim-resolution guard. That diagnosis was wrong — the failure was + // deterministic (fails 1/1 locally in isolation). Real cause: #2805 widened + // `collectImageUrls(publicDir, slug?: string)` to `(publicDir, slugs?: string[])` + // and updated the production call site + the new .js sibling test, but this + // older .ts test kept passing a bare string `'beta'`. The impl does + // `for (const s of slugs?.length ? slugs : readdir(...))`, so a string iterates + // by CHARACTER ('b','e','t','a') → no matching dirs → empty map. `retry:1` then + // masked it as noise. The fix is to call with arrays, matching the signature. + // + // The guard below asserts the call CONTRACT (a known-good array call yields a + // non-empty map), so a future signature/shape drift fails loudly here instead + // of as a cryptic empty-map assertion three lines down. let root: string let publicDir: string beforeEach(() => { @@ -30,10 +38,13 @@ describe('backfill-images --slug filter', () => { mkdirSync(join(tut, 'beta'), { recursive: true }) writeFileSync(join(tut, 'alpha', 'index.html'), page('alpha', 'a.png')) writeFileSync(join(tut, 'beta', 'index.html'), page('beta', 'b.png')) - // Fixture + import-shim sanity: fail loudly (not as an empty-map flake) if the - // file isn't on disk or the lazily-imported binding didn't resolve to a fn. + // Fixture + call-contract sanity: fail loudly (not as an empty-map flake) if + // the fixture file isn't on disk, the shim-resolved binding isn't a function, + // or the function's slug-filter contract regresses (e.g. a string vs string[] + // signature drift that iterates by character and silently yields []). expect(readFileSync(join(tut, 'beta', 'index.html'), 'utf8')).toContain('/img-cdn?u=') expect(typeof collectImageUrls).toBe('function') + expect(collectImageUrls(publicDir, ['beta']).size).toBe(1) }) afterEach(() => { rmSync(root, { recursive: true, force: true }) }) @@ -44,14 +55,14 @@ describe('backfill-images --slug filter', () => { }) it('restricts enumeration to the target slug', () => { - const map = collectImageUrls(publicDir, 'beta') + const map = collectImageUrls(publicDir, ['beta']) expect([...map.values()]).toEqual(['beta']) expect(map.size).toBe(1) expect([...map.keys()][0]).toContain('/beta/b.png') }) it('returns an empty map for an unknown slug (no throw)', () => { - const map = collectImageUrls(publicDir, 'does-not-exist') + const map = collectImageUrls(publicDir, ['does-not-exist']) expect(map.size).toBe(0) })