Skip to content

Extract the flow-cleanup tracker: 50 hand-copied variants, 4 axes of drift #1108

Description

@Victor-w-Madeira

Problem

The "capture every flow the page creates from its POST /api/v1/flows → 201 responses, delete them id-scoped in afterEach" block is hand-copied into 50 spec files. It is the suite's standard id-scoped cleanup (#490/#681/#515, never the delete-all of #553), but there is no shared implementation — so every copy drifted independently, and a fix to one does not reach the other 49.

Measured on main (fe966d4), over the 50 files that carry both createdFlowIds and a page.on("response") listener:

axis majority minority
accumulator type 49 use string[] 1 uses Set
settles pending resp.json() before cleaning up 1
navigates off the canvas before deleting 2 (about:blank)
failed deleteFlow is silently swallowed 45 .catch(() => {}) 9 log it
URL match 49 .includes("/api/v1/flows") 4 check the exact pathname

Why each divergence matters

  • The unsettled-body race (49 files). The listener calls resp.json(), which resolves a tick later, so an id can land in the array after afterEach has already read it. The flow then leaks permanently — the last test in a worker has no later hook to sweep it. Only api/flows/api-component-regression.spec.ts holds those reads and settles them first (pendingCaptures, added in fix: select node parameter fields by test id, never by DOM id (LE-2037) #1105).
  • Deleting under a mounted editor (48 files). project-management: residual 404s after the destructive lane — an editor polling a deleted flow, and a folder refetched after its own deletion #1023 measured this: an editor left on the canvas keeps polling GET /flows/{id}/events?since= and 404s once the flow is gone; the fixture logs each as 🚨 Backend Error and the deterministic pipeline's VALIDATE gate hard-stops on them. fix(project-management): tear the page down before the data and stop leaking folders (#1023) #1103 fixed it in the folder specs with an unconditional about:blank. Worth stating honestly: this does not fire in every spec — a probe on api/flows/api-component-regression.spec.ts (failure forced after the component run, editor mounted, flow deleted underneath) logged zero backend errors either way, because the build's event stream is already closed. It bites the specs whose editor is still polling.
  • Silent cleanup failure (45 files). deleteFlow throws on purpose — it already absorbs 404-as-done and one transient 5xx, so anything left is a real failure (401/403/422). .catch(() => {}) turns exactly the signal the helper exists to raise back into a silent leak.
  • Loose URL match (49 files). .includes("/api/v1/flows") also matches /api/v1/flows/batch/ and /flows/upload/. Both answer 201 with a list body carrying no top-level id, so this is fragility, not a live bug — but it is the reason the 4 stricter copies exist.

Proposal

Extract one implementation and migrate the 50 call sites onto it. Two shapes, and the trade-off is real:

  1. A helper — e.g. tests/helpers/flows/track-created-flows.ts exposing the listener registration plus the teardown. Opt-in per spec, so migration is incremental and each PR's blast radius stays small.
  2. A fixture in tests/fixtures/fixtures.ts — automatic for every spec, no per-file boilerplate at all. But tests/fixtures/** is suite-wide for impacted-specs-by-import.mjs: it resolves to every spec and raises the reviewer warning to dispatch manual.yml for a full run (pr-validation selects no E2E specs when a shared helper or Page Object changes #1054).

Whichever shape wins, it should absorb the strictest behaviour from each axis above: Set, settled body reads, exact-pathname match, navigate off the canvas before deleting, and log a failed deleteFlow instead of swallowing it.

Scope note

50 files is too much for one PR to validate credibly. Suggested split: one PR adding the shared implementation with unit tests (tests/helpers/flows/track-created-flows.test.ts, per CONTRIBUTING → Unit tests), then migration PRs by area — each one runnable and reviewable on its own.

Acceptance

  • One shared implementation, unit-tested
  • Every migrated spec keeps its current green/red status (no behaviour change beyond the cleanup)
  • No 🚨 Backend Error introduced in the migrated areas
  • The four stricter behaviours above are the default, not per-file choices

Follow-up to #1105 (where the divergence surfaced in review) and #1103/#1023.

Metadata

Metadata

Labels

follow-upApproved exception: follow-up of merged work (ROADMAP Intake)qa-infraQA testing infrastructure: workflows, automation, evidence, tracking

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions