Skip to content

chore(scripts): check.sh runs the browser suites, and the cycle suite is repeatable (#314) - #322

Merged
JArmandoAnaya merged 2 commits into
mainfrom
chore/314-check-browser-suites
Aug 5, 2026
Merged

chore(scripts): check.sh runs the browser suites, and the cycle suite is repeatable (#314)#322
JArmandoAnaya merged 2 commits into
mainfrom
chore/314-check-browser-suites

Conversation

@JArmandoAnaya

@JArmandoAnaya JArmandoAnaya commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Closes #314.

bash scripts/check.sh ran no browser suite while calling itself the canonical
"before you say it works" invocation. Two of the three suites were invisible to it, and
the real-server cycle run was three separate times the only one to catch a regression
during the 2026-08 remediation run (#306, #308, #309) — one of which shipped on a green
run of this script and went red in CI.

Two parts, because the second was blocked on the first.

Part 1 — the cycle suite is repeatable now

--repeat-each could not amplify the cycle suite. scripts/cycle_server.sh rebuilds the
workspace once per server start, --repeat-each reuses that one server, and the spec
created its project from a fixed literal — so repeat 2 onward died on
POST /projects → 409, a wall standing in front of everything the suite is about.
Repetition meant N whole invocations at about ninety seconds of rebuild each. (Found while
proving #319's fix; recorded in that PR's body as "found, not fixed".)

cycle/cycle.spec.ts derives the project name from testInfo.repeatEachIndex. Unique
names rather than teardown: teardown that has to survive a mid-run crash is a second bug
surface, and there is nothing to tear down when nothing collides.

The project is the only name that had to move, and the spec now says so, so the next
collision is looked for rather than assumed — a release tag is unique per dataset, a batch
name is not unique at all, and a source's idempotency key (project, kind, path, fps)
leads with the project. All three are already scoped by a project that is new. Verified
rather than reasoned about: three POST /projects → 201, zero 409 anywhere in the run,
and the only 4xx at all is the expected GET /projects/{id}/schema → 404 on a schema-less
project, once per repetition.

Part 2 — check.sh runs all three suites

  • The full run is the default; --fast is the exception. That inversion is the point:
    the previous default was silently the fast one.
  • browser is addressable as a group (bash scripts/check.sh browser), and it is a
    complete run on its own — each Playwright config's webServer.command builds
    @visionset/annotator and @visionset/ui-core first, because frontend/app resolves
    them through their dist/.
  • CI=1 is set by the script, for the Playwright steps, from frontend/app, in a
    subshell so the cd cannot leak into a later group. It is load-bearing:
    reuseExistingServer: !process.env.CI means a stale vite server on :5273 answers instead
    of the build under test. That lesson lived in a skill and in three people's memories; it
    lives in the tooling now.
  • Skipping prints a banner, after the verdict. The banner exists to qualify "All checks
    passed", so it has to be the thing still on screen once that line has scrolled off. It
    names both suites, both CI jobs, the 3×-only-detector fact, and the command that runs
    them. It prints whenever the browser suites did not run, not only under --fast — a
    partial run is the same lie however it was asked for.
  • Per-step timings, printed as a table with a total. The full gate costs minutes now,
    so it is worth seeing where they go, and worth noticing the day a step quietly doubles.
  • --fast together with an explicit browser group is refused, not resolved. Either
    answer would be a guess about which half of a contradictory command line was meant.

The script header now documents the three suites, the CI=1 requirement and the stale-5273
failure mode it prevents, and the git ls-files caveat (gates read the index, so an
unstaged new file is invisible to them and passes locally while failing in CI).

Found, not fixed

  • macOS still ships bash 3.2, where "${arr[@]}" on an empty array is an unbound
    variable under set -u
    and kills the script. Nothing reaches the new timing table empty
    array today, but it is one refactor away, so it is length-checked with the reason written
    down — the existing failed block has always been written that way and never said why.

No CI or ruleset change

CI's annotator e2e (chromium) and browser cycle (chromium) jobs already run these
suites, and they stay separate jobs — each is a required check with its own Playwright
cache and its own report artifact. No job name moved, so the main ruleset's twelve
contexts are untouched. annotator bench (chromium, manual) stays out of the default here
for the same reason it stays out of the ruleset: it is workflow_dispatch-only.

Test plan

Acceptance for Part 1 — --repeat-each=3 against a single persistent workspace:

$ CI=1 npx playwright test -c playwright.cycle.config.ts --repeat-each=3
  3 passed (51.0s)
EXIT=0

$ grep -c '409 Conflict' log
0
$ grep 'POST /projects HTTP' log
"POST /projects HTTP/1.1" 201 Created
"POST /projects HTTP/1.1" 201 Created
"POST /projects HTTP/1.1" 201 Created

51 s for three repetitions, against ~90 s of rebuild per repetition before.

Acceptance for Part 2 — one full bash scripts/check.sh, and one --fast showing the
banner. Both below.

Full bash scripts/check.sh — all three suites, exit 0:

Timing
    391s  python tests
      1s  ruff (lint)
      0s  ruff (format)
     19s  mypy
      2s  import contracts
     11s  frontend build
     32s  frontend tests
     22s  frontend lint
      3s  openapi drift
      1s  generated client drift
      2s  mcp tool reference drift
      1s  version sync
    123s  annotator + app e2e (chromium)      -> 179 passed (2.0m)
     28s  browser cycle, real server (chromium) -> 1 passed (26.3s)
    636s  total

All checks passed.

Worth reading off that table: the browser suites are 24% of the wall clock and
python tests is 61% of it.
The cost of honesty here is smaller than the cost of
pytest, which nobody was proposing to skip.

Edge cases, all exit 2 with the reason named: --nope (unknown flag), nope (unknown
group), browser --fast (contradiction).

bash scripts/check.sh --fast — browser suites skipped, exit 0, banner last:

Timing
    396s  python tests
      ...
    474s  total

All checks passed.

=============================================================================
 !!  THE BROWSER SUITES DID NOT RUN  --  this is not what CI runs         !!
=============================================================================
 skipped:  annotator + app e2e           CI job: annotator e2e (chromium)
           browser cycle, real server    CI job: browser cycle (chromium)

 The real-server cycle run was three separate times the ONLY suite to
 catch a regression during the 2026-08 remediation run (#306, #308, #309).

 Run them:  bash scripts/check.sh browser
=============================================================================

…each works

`scripts/cycle_server.sh` rebuilds the workspace once per *server start* and
`--repeat-each` reuses that one server, so a project created from a fixed
literal made the flag useless: repeat 2 onward died on `POST /projects -> 409`,
a wall standing in front of everything the suite is about. Repetition meant N
whole invocations at about ninety seconds of rebuild each.

Unique names rather than teardown, because teardown that has to survive a
mid-run crash is a second bug surface, and there is nothing to tear down when
nothing collides.

The project is the only name that had to move, and the spec now says so, so the
next collision is looked for rather than assumed: a release tag is unique per
dataset, a batch name is not unique at all, and a source's idempotency key
(project, kind, path, fps) leads with the project.

Verified rather than reasoned about: --repeat-each=3 gives three
`POST /projects -> 201`, zero 409 anywhere in the run, and the only 4xx at all
is the expected schema-less-project 404, once per repetition.

Refs #314
…ings

check.sh ran no browser suite while calling itself the canonical "before you
say it works" invocation. Two of the three suites were invisible to it, and the
real-server cycle run was three separate times the *only* one to catch a
regression during the 2026-08 remediation run (#306, #308, #309) -- one of
which shipped on a green run of this script and went red in CI.

The full run is the default and --fast is the exception. That inversion is the
point: the previous default was silently the fast one.

- `browser` is addressable as a group and is a complete run on its own, because
  each Playwright config's webServer.command builds the engine and the design
  system first.
- CI=1 is set by the script, for the Playwright steps, from frontend/app, in a
  subshell so the cd cannot leak into a later group. It is load-bearing:
  reuseExistingServer: !CI means a stale vite server on :5273 answers instead of
  the build under test. The lesson lived in a skill and in memories; it lives in
  the tooling now.
- Skipping prints a banner, *after* the verdict -- it exists to qualify "All
  checks passed", so it has to be the thing still on screen once that line has
  scrolled off. It prints whenever the browser suites did not run, not only
  under --fast: a partial run is the same lie however it was asked for.
- Per-step timings with a total, so the cost is visible and a step that quietly
  doubles gets noticed.
- --fast together with an explicit `browser` group is refused rather than
  resolved. Either answer would guess at which half of a contradictory command
  line was meant.

The timing table is length-checked before expanding, and the reason is written
down: macOS still ships bash 3.2, where "${arr[@]}" on an empty array is an
unbound variable under set -u and kills the script. The neighbouring `failed`
block had always been written that way and never said why.

No CI or ruleset change. The two jobs already run these suites and stay
separate -- each is a required check with its own cache and report artifact --
and no job name moved.

Refs #314
@JArmandoAnaya
JArmandoAnaya enabled auto-merge (squash) August 5, 2026 02:15
@JArmandoAnaya
JArmandoAnaya merged commit fe0e9dd into main Aug 5, 2026
14 checks passed
@JArmandoAnaya
JArmandoAnaya deleted the chore/314-check-browser-suites branch August 5, 2026 02:23
JArmandoAnaya added a commit that referenced this pull request Aug 21, 2026
… is repeatable (#314) (#322)

* test(app): the cycle suite's project name is run-scoped, so --repeat-each works

`scripts/cycle_server.sh` rebuilds the workspace once per *server start* and
`--repeat-each` reuses that one server, so a project created from a fixed
literal made the flag useless: repeat 2 onward died on `POST /projects -> 409`,
a wall standing in front of everything the suite is about. Repetition meant N
whole invocations at about ninety seconds of rebuild each.

Unique names rather than teardown, because teardown that has to survive a
mid-run crash is a second bug surface, and there is nothing to tear down when
nothing collides.

The project is the only name that had to move, and the spec now says so, so the
next collision is looked for rather than assumed: a release tag is unique per
dataset, a batch name is not unique at all, and a source's idempotency key
(project, kind, path, fps) leads with the project.

Verified rather than reasoned about: --repeat-each=3 gives three
`POST /projects -> 201`, zero 409 anywhere in the run, and the only 4xx at all
is the expected schema-less-project 404, once per repetition.

Refs #314

* chore(scripts): check.sh runs the browser suites, with --fast and timings

check.sh ran no browser suite while calling itself the canonical "before you
say it works" invocation. Two of the three suites were invisible to it, and the
real-server cycle run was three separate times the *only* one to catch a
regression during the 2026-08 remediation run (#306, #308, #309) -- one of
which shipped on a green run of this script and went red in CI.

The full run is the default and --fast is the exception. That inversion is the
point: the previous default was silently the fast one.

- `browser` is addressable as a group and is a complete run on its own, because
  each Playwright config's webServer.command builds the engine and the design
  system first.
- CI=1 is set by the script, for the Playwright steps, from frontend/app, in a
  subshell so the cd cannot leak into a later group. It is load-bearing:
  reuseExistingServer: !CI means a stale vite server on :5273 answers instead of
  the build under test. The lesson lived in a skill and in memories; it lives in
  the tooling now.
- Skipping prints a banner, *after* the verdict -- it exists to qualify "All
  checks passed", so it has to be the thing still on screen once that line has
  scrolled off. It prints whenever the browser suites did not run, not only
  under --fast: a partial run is the same lie however it was asked for.
- Per-step timings with a total, so the cost is visible and a step that quietly
  doubles gets noticed.
- --fast together with an explicit `browser` group is refused rather than
  resolved. Either answer would guess at which half of a contradictory command
  line was meant.

The timing table is length-checked before expanding, and the reason is written
down: macOS still ships bash 3.2, where "${arr[@]}" on an empty array is an
unbound variable under set -u and kills the script. The neighbouring `failed`
block had always been written that way and never said why.

No CI or ruleset change. The two jobs already run these suites and stay
separate -- each is a required check with its own cache and report artifact --
and no job name moved.

Refs #314
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tooling: check.sh runs no browser suite, and the real-server cycle run was three times the only detector

1 participant