test(backfill-images): fix the real signature regression #2812 misdiagnosed as a flake (#2813)

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 <file>`) 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(<bundle>) }`),
+**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

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)
   })