amaurycoudr/chess-project

Fork 0

e2e: CI drives the built bundle, a missing await fails lint, and the suite's budgets become decisions #204

New issue

Closed

opened 2026-09-02 07:40:21 +00:00 by root · 3 comments

root commented

2026-09-02 07:40:21 +00:00

(Migrated from gitlab.core-project.space)

Imported from GitHub amaurycoudr/chess-project#204, opened by @amaurycoudr on 2026-09-02 07:40 UTC.
Closed on GitHub on 2026-09-02 13:25 UTC as completed.

Problem Statement

The browser-seam suite is the only oracle for the Room refactor (#191), and two things about it are not true today.

CI is not testing what it claims to test. The workflow builds the packages and sets E2E_TARGET=built so the suite drives the bundle a Player actually receives. It doesn't. Turbo 2 runs in strict env mode by default, and the test:slow task declares no passthrough for that variable, so it arrives at the runner as undefined and the suite silently falls back to the dev servers. Three separate comments in the repo promise the opposite, and the CI build step exists to serve a guarantee that is not being delivered. A regression that only appears in the production bundle would pass CI.

A dropped await would silently disable an assertion. Every helper in the suite returns a promise. Omit one await and the assertion never runs, the test still reports green, and nothing objects — typescript/no-floating-promises is a type-aware rule, and type-aware linting is not enabled in this repo, so pnpm lint does not catch it. Every assertion is correctly awaited today; the exposure is the next edit.

Beyond those two defects, several of the suite's most important numbers are unstated defaults rather than decisions. The budget for a Move to cross from one Player's browser to the other's is the built-in five seconds, and it appears nowhere. The poll that holds a request open while asserting local-first application inherits that same five seconds, and everything it guards must finish inside it. There is no ceiling on a wedged run. On CI there is no HTML report, so triage starts from a raw trace with no index. A seven-Move Game that fails on the fifth reports as a locator error on a square, with the Ply recoverable only from the trace.

Solution

One hardening pass over the browser-seam suite that makes CI test what it says it tests, makes a missing await a lint failure, and converts the suite's remaining implicit budgets into stated decisions.

Two of the eight changes fix defects. The rest are cheap, and each one replaces a silent default with something a reader can see and argue with.

After this, a Player-visible regression that exists only in the built bundle fails CI; a dropped await fails the five-command gate; every timeout that governs cross-browser propagation is named in the config next to the reason for its value; and a failing run on CI arrives with an indexed report, a trace, a recording per Player, and a step name identifying which Ply broke.

User Stories

As a maintainer, I want the browser suite on CI to drive the built bundle, so that a green pipeline is evidence about the artifact a Player receives rather than about a dev server.
As a maintainer, I want the run to fail loudly when it cannot drive the built bundle, so that a broken wiring is a red pipeline instead of a quieter guarantee.
As a developer, I want the env var carrying the target to be declared where it crosses the task-graph boundary, so that the workflow's stated intent is the behaviour that actually happens.
As a developer reading the runner config, I want the CI-versus-local target decision to be legible in one place, so that I can tell which servers a run drove without tracing an env var through three files.
As a developer, I want a missing await before an assertion to fail lint, so that the suite cannot go green while silently asserting nothing.
As a developer, I want that protection to cover the whole workspace and not only the browser suite, so that the engine and API get the same guarantee without a second decision later.
As a developer, I want the floating-promise check to run inside the existing five-command gate, so that it is enforced at the same moment as every other rule and needs no separate habit.
As a developer, I want the budget for a Move to cross between two Players' browsers to be a named value in the config, so that the single most important number in the suite is visible rather than inherited.
As a developer, I want that budget to carry the reason for its value, so that a later change to it is a decision rather than a nudge.
As a developer, I want the poll that holds the Move request open to state its own timeout, so that the work it guards is not silently racing an inherited default.
As a developer debugging that test, I want a timeout failure there to name what actually expired, so that I am not sent to an unrelated assertion in the other browser.
As a maintainer, I want a ceiling on total suite time, so that a wedged server cannot burn a CI runner's full default allowance.
As a maintainer, I want the CI job to carry its own time limit, so that the ceiling holds even when the failure is outside the runner's control.
As a developer triaging a CI failure, I want an HTML report alongside the trace, so that I can see which test failed and why before opening a trace file.
As a developer, I want that report uploaded with the existing artifacts, so that everything needed to diagnose a failure arrives together.
As a developer, I want the report to stay off in a local run, so that the terminal output I rely on while writing a test does not change.
As a developer watching a Game test fail, I want the failing Ply named in the report, so that I know which Move broke without reading the trace.
As a developer, I want each Move in the mate sequence to appear as its own step, so that a long Game reads as a sequence rather than one opaque assertion.
As a maintainer, I want the worker count to be an explicit choice, so that the resource cost of a test holding two browser contexts is accounted for rather than assumed.
As a maintainer, I want the reason for that choice recorded, so that the next person to change it knows what it was balancing.
As a developer, I want every one of these values to survive pnpm lint, pnpm typecheck and pnpm format:check, so that the hardening pass does not itself break the gate.
As a developer, I want the browser suite to stay outside the five-command gate, so that this change does not start a browser during a commit.
As an agent picking this ticket up, I want the two defects distinguishable from the six hardening items, so that a partial implementation still lands the parts that matter most.

Implementation Decisions

Scope is apps/e2e, the Turbo task graph, the CI workflow, and the root lint configuration. No front-end, API or engine code changes. No test is added, removed or renamed; one existing test gains step boundaries.

The built-artifact guarantee is enforced twice, deliberately. The Turbo test:slow task declares a passthrough for the target env var, which is the actual fix — it makes the workflow's existing declaration work. The runner config then throws when it is running under CI and has not been told to serve the built artifact. The first makes it correct; the second makes the whole class of failure loud rather than silent, and it follows the precedent already set in that file, where an unset port throws with a message naming the fix rather than defaulting. This shape came from checking the option against the running task graph:

turbo.json   test:slow: passThroughEnv: ["E2E_TARGET"]

runner config:
  if (!startedByHand && !servesBuiltArtifact) {
    throw new TypeError("CI must drive the built artifact — E2E_TARGET=built did not reach the task");
  }

The task sets cache: false, so passThroughEnv is correct here and env — which would additionally fold the variable into a cache key — is not needed.

The missing-await gap is closed with type-aware linting, workspace-wide. This requires adding the oxlint-tsgolint package as a dev dependency and enabling type-aware rules in the root lint configuration. Verified against the pinned oxlint: with the package installed and type-aware rules on, typescript/no-floating-promises reports an unawaited promise-returning call; without it, the same code reports clean. Prefer enabling it through the root configuration's typeAware option rather than adding a flag to each package's lint script, so the setting lives in one place; if that does not apply to per-package runs, fall back to the flag and note why. Expect the gate to get slower — that is the cost being accepted, and it buys the same protection for the engine and API.

Timeouts become named constants next to the ones already there. The runner config already names its per-test and server-start budgets as constants with comments. The cross-browser propagation budget (the assertion timeout) and the total suite ceiling (the global timeout) join them in the same style. The propagation budget is the one that governs how long a Move has to reach the other Player's browser, and its comment should say so — it is the number that decides whether a slow fan-out reads as a failure.

The poll holding the Move request open gets an explicit timeout, comfortably larger than the assertions it guards, so that the test's own budget is stated rather than inherited from the assertion default. This is the test asserting that a Move is applied locally before the Room has answered.

The CI job gets a time limit, and the reporter becomes environment-dependent: the existing terminal reporter locally, and that reporter plus an HTML report that does not try to open a browser on CI. The report directory joins the existing artifact upload so a failure's report, trace and per-Player recordings arrive together. The report directory is already ignored by version control.

Each Move in the checkmate Game becomes a named step, labelled by its Ply and the squares it joins, so a failure names the Move. This pulls a runner import into a spec that currently takes everything from the suite's fixtures module; that is acceptable and should be the only such import.

The worker count becomes explicit with a comment recording what it balances. The value matters less than the fact that it is chosen: a test here holds two browser contexts, so the real cost per worker is double what the default assumes. Note that the official guidance is genuinely self-contradictory on this point — one page recommends a single worker on CI for reproducibility, another recommends parallelism — so the comment should record the local reasoning rather than cite a rule.

Explicitly unchanged: the retry count stays at zero, and the reason already recorded in the config still holds — a retry lets a flake report green, and this suite is the refactor's only oracle. The suite stays Chromium-only. Setup continues to go through the user interface, per this package's own standards. None of these is revisited here.

Testing Decisions

A good test here asserts what a Player can perceive and nothing about how the app is built — this package's standards already govern that, and this ticket does not relax them. The suite's existing tests are the prior art and the shape to preserve.

This ticket adds no new test, and that is the decision, not an omission. Seven of the eight changes are configuration; their correctness is observable in the gate and in a single suite run, and a test asserting that a config literal holds a particular value would assert the implementation rather than a behaviour.

The eighth change is itself the test. The guard in the runner config is the verification for the built-artifact guarantee: it converts a silent misconfiguration into a startup failure on every CI run, which is stronger and cheaper than a test that would have to reproduce the task graph to observe the same thing. It follows the existing precedent in that file, where a missing port throws rather than defaulting.

The floating-promise change is verified by the gate. Its correctness claim is "lint now fails on an unawaited assertion", which pnpm lint demonstrates directly. The implementer should confirm both directions before considering it done — that a deliberately unawaited call fails, and that the suite as it stands passes — and should not leave the deliberately-broken version behind.

No new seam is introduced. The browser suite remains the only browser-driving seam in the repo, and it stays outside the five-command gate and outside the pre-commit hook, so none of this starts a browser during a commit. pnpm test:slow remains the way to run it.

Acceptance is the five commands green, plus one full browser-suite run passing with the new timeouts and step boundaries in place, and the report and artifacts landing where the workflow expects them.

Out of Scope

Changing the retry count, the browser matrix, or the user-interface-driven setup policy. Each was assessed and each is a deliberate, documented divergence that stands.
Adding tests, sharding, a blob reporter, or merge-reports. The suite is three tests; sharding solves a problem it does not have.
Any change to the front-end or API to make a test easier. This package's standards forbid it, and nothing here needs it.
Adding accessibility assertions, visual regression, or a console-error guard. Each may be worth its own ticket; none is part of this pass.
Restructuring the suite into page objects. The current helper-and-fixture shape was assessed as correct for its size.
Adopting test-id selectors. The suite is role-first by policy and unusually disciplined about it.
Revisiting retryStrategy, which is inert while retries are zero.

Further Notes

This spec comes from a full audit of the suite against Playwright's official guidance for the pinned version and against seven open-source reference suites. The audit is committed alongside this work and holds the criteria checklist, the benchmark table of contested practices, and the per-criterion assessment with evidence. It scored the suite 42 compliant, 9 partial, 7 violated.

Three facts behind this ticket were established by experiment rather than read from documentation, because the documentation is silent or secondary sources disagree:

Turbo's strict env mode drops an undeclared variable, reproduced in an isolated workspace on the pinned Turbo. The same probe confirms the CI variable does pass through, which is why the local-versus-CI server decision is correct today and only the target is lost.
The assertion default that governs cross-browser propagation is five seconds, read from the installed runner rather than assumed.
Type-aware linting does catch the unawaited assertion, and the standard configuration does not, both reproduced against the pinned oxlint.

Two things the suite already does are better than the common advice and must not be "tidied" while working nearby. Recording is requested per browser context rather than through the runner's video option, because that option reaches only contexts the runner creates itself — setting it in the config would record nothing and report no error. And the rule that a behaviour spanning two Players is asserted in both browsers is what makes the suite able to catch a broken fan-out at all. Both are documented in this package's standards; leave them alone.

The audit also flagged a conflict worth knowing about while working here: the single broadest consensus in the research — seed state through the API, not the interface — runs against this package's written standard, which mandates interface-driven setup so that setup and assertion share one seam. That standard stands and this ticket does not touch it, but an implementer who "improves" setup by adding a back door would be violating it.

> _Imported from GitHub [amaurycoudr/chess-project#204](https://github.com/amaurycoudr/chess-project/issues/204), opened by [@amaurycoudr](https://github.com/amaurycoudr) on 2026-09-02 07:40 UTC._ > _Closed on GitHub on 2026-09-02 13:25 UTC as completed._  --- ## Problem Statement The browser-seam suite is the only oracle for the Room refactor (#191), and two things about it are not true today. **CI is not testing what it claims to test.** The workflow builds the packages and sets `E2E_TARGET=built` so the suite drives the bundle a Player actually receives. It doesn't. Turbo 2 runs in strict env mode by default, and the `test:slow` task declares no passthrough for that variable, so it arrives at the runner as `undefined` and the suite silently falls back to the dev servers. Three separate comments in the repo promise the opposite, and the CI build step exists to serve a guarantee that is not being delivered. A regression that only appears in the production bundle would pass CI. **A dropped `await` would silently disable an assertion.** Every helper in the suite returns a promise. Omit one `await` and the assertion never runs, the test still reports green, and nothing objects — `typescript/no-floating-promises` is a type-aware rule, and type-aware linting is not enabled in this repo, so `pnpm lint` does not catch it. Every assertion is correctly awaited today; the exposure is the next edit. Beyond those two defects, several of the suite's most important numbers are unstated defaults rather than decisions. The budget for a Move to cross from one Player's browser to the other's is the built-in five seconds, and it appears nowhere. The poll that holds a request open while asserting local-first application inherits that same five seconds, and everything it guards must finish inside it. There is no ceiling on a wedged run. On CI there is no HTML report, so triage starts from a raw trace with no index. A seven-Move Game that fails on the fifth reports as a locator error on a square, with the Ply recoverable only from the trace. ## Solution One hardening pass over the browser-seam suite that makes CI test what it says it tests, makes a missing `await` a lint failure, and converts the suite's remaining implicit budgets into stated decisions. Two of the eight changes fix defects. The rest are cheap, and each one replaces a silent default with something a reader can see and argue with. After this, a Player-visible regression that exists only in the built bundle fails CI; a dropped `await` fails the five-command gate; every timeout that governs cross-browser propagation is named in the config next to the reason for its value; and a failing run on CI arrives with an indexed report, a trace, a recording per Player, and a step name identifying which Ply broke. ## User Stories 1. As a maintainer, I want the browser suite on CI to drive the built bundle, so that a green pipeline is evidence about the artifact a Player receives rather than about a dev server. 2. As a maintainer, I want the run to fail loudly when it cannot drive the built bundle, so that a broken wiring is a red pipeline instead of a quieter guarantee. 3. As a developer, I want the env var carrying the target to be declared where it crosses the task-graph boundary, so that the workflow's stated intent is the behaviour that actually happens. 4. As a developer reading the runner config, I want the CI-versus-local target decision to be legible in one place, so that I can tell which servers a run drove without tracing an env var through three files. 5. As a developer, I want a missing `await` before an assertion to fail lint, so that the suite cannot go green while silently asserting nothing. 6. As a developer, I want that protection to cover the whole workspace and not only the browser suite, so that the engine and API get the same guarantee without a second decision later. 7. As a developer, I want the floating-promise check to run inside the existing five-command gate, so that it is enforced at the same moment as every other rule and needs no separate habit. 8. As a developer, I want the budget for a Move to cross between two Players' browsers to be a named value in the config, so that the single most important number in the suite is visible rather than inherited. 9. As a developer, I want that budget to carry the reason for its value, so that a later change to it is a decision rather than a nudge. 10. As a developer, I want the poll that holds the Move request open to state its own timeout, so that the work it guards is not silently racing an inherited default. 11. As a developer debugging that test, I want a timeout failure there to name what actually expired, so that I am not sent to an unrelated assertion in the other browser. 12. As a maintainer, I want a ceiling on total suite time, so that a wedged server cannot burn a CI runner's full default allowance. 13. As a maintainer, I want the CI job to carry its own time limit, so that the ceiling holds even when the failure is outside the runner's control. 14. As a developer triaging a CI failure, I want an HTML report alongside the trace, so that I can see which test failed and why before opening a trace file. 15. As a developer, I want that report uploaded with the existing artifacts, so that everything needed to diagnose a failure arrives together. 16. As a developer, I want the report to stay off in a local run, so that the terminal output I rely on while writing a test does not change. 17. As a developer watching a Game test fail, I want the failing Ply named in the report, so that I know which Move broke without reading the trace. 18. As a developer, I want each Move in the mate sequence to appear as its own step, so that a long Game reads as a sequence rather than one opaque assertion. 19. As a maintainer, I want the worker count to be an explicit choice, so that the resource cost of a test holding two browser contexts is accounted for rather than assumed. 20. As a maintainer, I want the reason for that choice recorded, so that the next person to change it knows what it was balancing. 21. As a developer, I want every one of these values to survive `pnpm lint`, `pnpm typecheck` and `pnpm format:check`, so that the hardening pass does not itself break the gate. 22. As a developer, I want the browser suite to stay outside the five-command gate, so that this change does not start a browser during a commit. 23. As an agent picking this ticket up, I want the two defects distinguishable from the six hardening items, so that a partial implementation still lands the parts that matter most. ## Implementation Decisions **Scope is `apps/e2e`, the Turbo task graph, the CI workflow, and the root lint configuration.** No front-end, API or engine code changes. No test is added, removed or renamed; one existing test gains step boundaries. **The built-artifact guarantee is enforced twice, deliberately.** The Turbo `test:slow` task declares a passthrough for the target env var, which is the actual fix — it makes the workflow's existing declaration work. The runner config then throws when it is running under CI and has not been told to serve the built artifact. The first makes it correct; the second makes the whole class of failure loud rather than silent, and it follows the precedent already set in that file, where an unset port throws with a message naming the fix rather than defaulting. This shape came from checking the option against the running task graph: ``` turbo.json test:slow: passThroughEnv: ["E2E_TARGET"] runner config: if (!startedByHand && !servesBuiltArtifact) { throw new TypeError("CI must drive the built artifact — E2E_TARGET=built did not reach the task"); } ``` The task sets `cache: false`, so `passThroughEnv` is correct here and `env` — which would additionally fold the variable into a cache key — is not needed. **The missing-`await` gap is closed with type-aware linting, workspace-wide.** This requires adding the `oxlint-tsgolint` package as a dev dependency and enabling type-aware rules in the root lint configuration. Verified against the pinned oxlint: with the package installed and type-aware rules on, `typescript/no-floating-promises` reports an unawaited promise-returning call; without it, the same code reports clean. Prefer enabling it through the root configuration's `typeAware` option rather than adding a flag to each package's lint script, so the setting lives in one place; if that does not apply to per-package runs, fall back to the flag and note why. Expect the gate to get slower — that is the cost being accepted, and it buys the same protection for the engine and API. **Timeouts become named constants next to the ones already there.** The runner config already names its per-test and server-start budgets as constants with comments. The cross-browser propagation budget (the assertion timeout) and the total suite ceiling (the global timeout) join them in the same style. The propagation budget is the one that governs how long a Move has to reach the other Player's browser, and its comment should say so — it is the number that decides whether a slow fan-out reads as a failure. **The poll holding the Move request open gets an explicit timeout**, comfortably larger than the assertions it guards, so that the test's own budget is stated rather than inherited from the assertion default. This is the test asserting that a Move is applied locally before the Room has answered. **The CI job gets a time limit**, and the reporter becomes environment-dependent: the existing terminal reporter locally, and that reporter plus an HTML report that does not try to open a browser on CI. The report directory joins the existing artifact upload so a failure's report, trace and per-Player recordings arrive together. The report directory is already ignored by version control. **Each Move in the checkmate Game becomes a named step**, labelled by its Ply and the squares it joins, so a failure names the Move. This pulls a runner import into a spec that currently takes everything from the suite's fixtures module; that is acceptable and should be the only such import. **The worker count becomes explicit** with a comment recording what it balances. The value matters less than the fact that it is chosen: a test here holds two browser contexts, so the real cost per worker is double what the default assumes. Note that the official guidance is genuinely self-contradictory on this point — one page recommends a single worker on CI for reproducibility, another recommends parallelism — so the comment should record the local reasoning rather than cite a rule. **Explicitly unchanged:** the retry count stays at zero, and the reason already recorded in the config still holds — a retry lets a flake report green, and this suite is the refactor's only oracle. The suite stays Chromium-only. Setup continues to go through the user interface, per this package's own standards. None of these is revisited here. ## Testing Decisions A good test here asserts what a Player can perceive and nothing about how the app is built — this package's standards already govern that, and this ticket does not relax them. The suite's existing tests are the prior art and the shape to preserve. **This ticket adds no new test, and that is the decision, not an omission.** Seven of the eight changes are configuration; their correctness is observable in the gate and in a single suite run, and a test asserting that a config literal holds a particular value would assert the implementation rather than a behaviour. **The eighth change is itself the test.** The guard in the runner config is the verification for the built-artifact guarantee: it converts a silent misconfiguration into a startup failure on every CI run, which is stronger and cheaper than a test that would have to reproduce the task graph to observe the same thing. It follows the existing precedent in that file, where a missing port throws rather than defaulting. **The floating-promise change is verified by the gate.** Its correctness claim is "lint now fails on an unawaited assertion", which `pnpm lint` demonstrates directly. The implementer should confirm both directions before considering it done — that a deliberately unawaited call fails, and that the suite as it stands passes — and should not leave the deliberately-broken version behind. **No new seam is introduced.** The browser suite remains the only browser-driving seam in the repo, and it stays outside the five-command gate and outside the pre-commit hook, so none of this starts a browser during a commit. `pnpm test:slow` remains the way to run it. **Acceptance is the five commands green, plus one full browser-suite run passing with the new timeouts and step boundaries in place**, and the report and artifacts landing where the workflow expects them. ## Out of Scope - Changing the retry count, the browser matrix, or the user-interface-driven setup policy. Each was assessed and each is a deliberate, documented divergence that stands. - Adding tests, sharding, a blob reporter, or `merge-reports`. The suite is three tests; sharding solves a problem it does not have. - Any change to the front-end or API to make a test easier. This package's standards forbid it, and nothing here needs it. - Adding accessibility assertions, visual regression, or a console-error guard. Each may be worth its own ticket; none is part of this pass. - Restructuring the suite into page objects. The current helper-and-fixture shape was assessed as correct for its size. - Adopting test-id selectors. The suite is role-first by policy and unusually disciplined about it. - Revisiting `retryStrategy`, which is inert while retries are zero. ## Further Notes This spec comes from a full audit of the suite against Playwright's official guidance for the pinned version and against seven open-source reference suites. The audit is committed alongside this work and holds the criteria checklist, the benchmark table of contested practices, and the per-criterion assessment with evidence. It scored the suite 42 compliant, 9 partial, 7 violated. Three facts behind this ticket were established by experiment rather than read from documentation, because the documentation is silent or secondary sources disagree: - Turbo's strict env mode drops an undeclared variable, reproduced in an isolated workspace on the pinned Turbo. The same probe confirms the `CI` variable **does** pass through, which is why the local-versus-CI server decision is correct today and only the target is lost. - The assertion default that governs cross-browser propagation is five seconds, read from the installed runner rather than assumed. - Type-aware linting does catch the unawaited assertion, and the standard configuration does not, both reproduced against the pinned oxlint. Two things the suite already does are better than the common advice and must not be "tidied" while working nearby. Recording is requested per browser context rather than through the runner's video option, because that option reaches only contexts the runner creates itself — setting it in the config would record nothing and report no error. And the rule that a behaviour spanning two Players is asserted in both browsers is what makes the suite able to catch a broken fan-out at all. Both are documented in this package's standards; leave them alone. The audit also flagged a conflict worth knowing about while working here: the single broadest consensus in the research — seed state through the API, not the interface — runs against this package's written standard, which mandates interface-driven setup so that setup and assertion share one seam. That standard stands and this ticket does not touch it, but an implementer who "improves" setup by adding a back door would be violating it.

root (Migrated from gitlab.core-project.space) closed this issue

2026-09-02 13:25:35 +00:00

root commented

2026-10-13 10:19:27 +00:00

(Migrated from gitlab.core-project.space)

mentioned in commit 0f6e3d5460

mentioned in commit 0f6e3d5460299645e80934f27ea3265587a55a15

root commented

2026-10-13 10:19:27 +00:00

(Migrated from gitlab.core-project.space)

mentioned in commit 9579723a3f

mentioned in commit 9579723a3f504540c47a60c25747d725920d3993