yonlu/yellowjacket
1
1 Fork 0
Code Issues 61 Pull Requests 1 Actions Packages Projects Releases 13 Wiki Activity

e2e state leaks between spec files: a staged job outlives the spec that staged it #168

New Issue
Closed
opened 2026-08-21 00:19:34 +00:00 by logan · 3 comments
logan commented 2026-08-21 00:19:34 +00:00
Collaborator
Copy link
Copy source

Report

The e2e app is one long-lived process shared by every spec file and by
every make e2e invocation, and /__test/emit writes to stores that
nothing clears. So state staged by one spec file is still there for the
next, and for the next run.

Tripped over while doing #57. A draft assertion read "with the top bar
gone, the content starts at y=0". It passed run after run in isolation
and failed inside a suite run, because top-bar-fit.spec.ts had staged
a long-titled scan with testctl.emit('JobsChanged', …) and <job-band>
is a real grid row whenever work is in flight (#62) — so the content
started at 55.

Findings

  • dev-headless.sh daemonises and make e2e drives the app that is
    already running; this is deliberate and documented (starting one per
    run would rebuild the frontend every time).
  • /__test/emit is exactly the right tool and is not the problem —
    the point of it is to force a backend event without staging the work.
    What is missing is any statement of who owns the state afterwards.
  • jobs-on-a-phone.spec.ts and top-bar-fit.spec.ts both stage jobs
    and neither clears them; nothing has needed them to until a third
    spec measured a layout.
  • The failure mode is the worst kind: the spec that leaks passes,
    and an unrelated spec fails     — and it fails only in a suite run and
    only in file order, so it reads as a flake.

Direction

Two halves, and the second is the one that generalises.

Give testctl a way to say "no jobs" and have the specs that stage
them clean up — emit('JobsChanged', []) in an afterEach is enough
and needs no backend change.

And write down the rule that actually prevents this, which #57 arrived
at by accident: measure against the thing next to you, not against an
absolute coordinate. contentTop === 0 was quietly also asserting
"and no background job is running", which is not something that spec
was about or could arrange; contentTop === jobBandBottom is true
either way. An absolute coordinate in a shell measurement is an implicit
claim about everything above it.

Filed rather than fixed here because the cleanup belongs with whoever
next touches the job specs, and because the rule is worth stating
before it is enforced.

**Report** The e2e app is one long-lived process shared by every spec file and by every `make e2e` invocation, and `/__test/emit` writes to stores that nothing clears. So state staged by one spec file is still there for the next, and for the next *run*. Tripped over while doing #57. A draft assertion read "with the top bar gone, the content starts at y=0". It passed run after run in isolation and failed inside a suite run, because `top-bar-fit.spec.ts` had staged a long-titled scan with `testctl.emit('JobsChanged', …)` and `<job-band>` is a real grid row whenever work is in flight (#62) — so the content started at 55. **Findings** - `dev-headless.sh` daemonises and `make e2e` drives the app that is already running; this is deliberate and documented (starting one per run would rebuild the frontend every time). - `/__test/emit` is exactly the right tool and is not the problem — the point of it is to force a backend event without staging the work. What is missing is any statement of who owns the state afterwards. - `jobs-on-a-phone.spec.ts` and `top-bar-fit.spec.ts` both stage jobs and neither clears them; nothing has needed them to until a third spec measured a layout. - The failure mode is the worst kind: **the spec that leaks passes, and an unrelated spec fails** — and it fails only in a suite run and only in file order, so it reads as a flake. **Direction** Two halves, and the second is the one that generalises. Give `testctl` a way to say "no jobs" and have the specs that stage them clean up — `emit('JobsChanged', [])` in an `afterEach` is enough and needs no backend change. And write down the rule that actually prevents this, which #57 arrived at by accident: **measure against the thing next to you, not against an absolute coordinate.** `contentTop === 0` was quietly also asserting "and no background job is running", which is not something that spec was about or could arrange; `contentTop === jobBandBottom` is true either way. An absolute coordinate in a shell measurement is an implicit claim about everything above it. Filed rather than fixed here because the cleanup belongs with whoever next touches the job specs, and because the rule is worth stating before it is enforced.
logan added the Kind/Testing
Reviewed
Confirmed
1
Priority
Low
4
 labels 2026-08-21 00:19:34 +00:00
logan commented 2026-08-26 11:41:30 +00:00
Author
Collaborator
Copy link
Copy source

The leak half does not reproduce, measured under this issue's own
conditions. Posting the evidence rather than acting on it, because
the fix this asks for — an afterEach clearing jobs — would be a
change that fixes nothing, and the second half is worth keeping either
way.

What was measured. A throwaway probe spec was added that makes
exactly the draft assertion described here — at 390px, with the top bar
gone, where does the content start — and reports the numbers rather
than asserting them. zz- so it sorts last, i.e. after both
jobs-on-a-phone.spec.ts and top-bar-fit.spec.ts.

alone:                       PROBE {"contentTop":0,"bandBottom":0,"bandRows":0}
after the two job specs:     PROBE {"contentTop":0,"bandBottom":0,"bandRows":0}
in a full `make e2e` run:    PROBE {"contentTop":0,"bandBottom":0,"bandRows":0}   (245 passed)

contentTop is 0 in a suite run, which is the value this issue says
fails there. A second probe asked the backend directly, on a fresh page
straight after top-bar-fit.spec.ts had staged its long-titled scan:

{"stagedIds":[],"activeFromBackend":[],"bandRows":0,"bandVisible":true,"bandHTML":"<!---->"}

No top-bar-fit or phone: job in jobs.Service.GetJobs, and the band
renders an empty template.

Why, and it is structural rather than lucky. /__test/emit calls
events.Deliver and nothing else — it does not touch the job registry,
which is the point of it ("force a backend event without staging the
work"). Playwright gives each test a fresh context and page, and
job-store is a singleton in that page which rebuilds itself from
GetJobs() on load. So a staged JobsChanged reaches exactly the one
page that was open when it was emitted, and the next test — in the same
file or a different one — starts from the backend's own registry, which
never heard about it.

So there is no state for an afterEach to clear. Two smaller
consequences fall out of the same fact and are worth knowing before
someone writes another one: top-bar-fit.spec.ts's closing "Leave the
app as the next spec expects to find it" viewport reset and
queue-selection.spec.ts's comment that "an open panel outlives the
page" are both written on this belief. The queue one is genuinely
different — the queue is backend state and does outlive the page —
but the open panel is not.

What is still true is the rule, and it is the half that generalises:
contentTop === 0 was quietly also asserting "and no background job is
running". That is not a leak between files; it is an absolute
coordinate carrying an implicit claim about everything above it, and
the current spec is right to measure main.top - band.bottom instead.
It happens to be the case that the job it measures against is one its
own test stages.

Left for a decision rather than taken: whether this becomes a
docs-only issue for the rule (.pi/skills/yellowjacket-dev/references/
would be the place), or is closed as Reviewed/Invalid with the rule
folded into whichever spec next needs it. Either is defensible and
neither is mine to pick; nothing has been changed and no branch was
pushed.

Measured on main at 245647f, against make dev-headless SEED=default
with YJ_CORE_INDEX_URL pointed at a dead address.

**The leak half does not reproduce, measured under this issue's own conditions.** Posting the evidence rather than acting on it, because the fix this asks for — an `afterEach` clearing jobs — would be a change that fixes nothing, and the second half is worth keeping either way. **What was measured.** A throwaway probe spec was added that makes exactly the draft assertion described here — at 390px, with the top bar gone, where does the content start — and reports the numbers rather than asserting them. `zz-` so it sorts last, i.e. after both `jobs-on-a-phone.spec.ts` and `top-bar-fit.spec.ts`. ``` alone: PROBE {"contentTop":0,"bandBottom":0,"bandRows":0} after the two job specs: PROBE {"contentTop":0,"bandBottom":0,"bandRows":0} in a full `make e2e` run: PROBE {"contentTop":0,"bandBottom":0,"bandRows":0} (245 passed) ``` `contentTop` is 0 in a suite run, which is the value this issue says fails there. A second probe asked the backend directly, on a fresh page straight after `top-bar-fit.spec.ts` had staged its long-titled scan: ``` {"stagedIds":[],"activeFromBackend":[],"bandRows":0,"bandVisible":true,"bandHTML":"<!---->"} ``` No `top-bar-fit` or `phone:` job in `jobs.Service.GetJobs`, and the band renders an empty template. **Why, and it is structural rather than lucky.** `/__test/emit` calls `events.Deliver` and nothing else — it does not touch the job registry, which is the point of it ("force a backend event without staging the work"). Playwright gives each *test* a fresh context and page, and `job-store` is a singleton in that page which rebuilds itself from `GetJobs()` on load. So a staged `JobsChanged` reaches exactly the one page that was open when it was emitted, and the next test — in the same file or a different one — starts from the backend's own registry, which never heard about it. So there is no state for an `afterEach` to clear. Two smaller consequences fall out of the same fact and are worth knowing before someone writes another one: `top-bar-fit.spec.ts`'s closing "Leave the app as the next spec expects to find it" viewport reset and `queue-selection.spec.ts`'s comment that "an open panel outlives the page" are both written on this belief. The queue one is genuinely different — the *queue* is backend state and does outlive the page — but the open panel is not. **What is still true is the rule**, and it is the half that generalises: `contentTop === 0` was quietly also asserting "and no background job is running". That is not a leak between files; it is an absolute coordinate carrying an implicit claim about everything above it, and the current spec is right to measure `main.top - band.bottom` instead. It happens to be the case that the job it measures against is one its own test stages. **Left for a decision** rather than taken: whether this becomes a docs-only issue for the rule (`.pi/skills/yellowjacket-dev/references/` would be the place), or is closed as `Reviewed/Invalid` with the rule folded into whichever spec next needs it. Either is defensible and neither is mine to pick; nothing has been changed and no branch was pushed. Measured on `main` at 245647f, against `make dev-headless SEED=default` with `YJ_CORE_INDEX_URL` pointed at a dead address.
yonlu self-assigned this 2026-09-23 11:26:01 +00:00
yonlu added the
Status
In Progress
 label 2026-09-23 11:26:01 +00:00
yonlu commented 2026-09-23 11:26:01 +00:00
Owner
Copy link
Copy source

Claiming. Both halves of the Direction, since the second is the one that generalises.

  1. test.afterEach emitting JobsChanged with [] in the two specs that stage jobs (jobs-on-a-phone.spec.ts, top-bar-fit.spec.ts), so the state they create dies with them. No backend change needed.
  2. The rule written into .pi/skills/yellowjacket-dev/references/harness.md: an absolute coordinate in a shell measurement is an implicit claim about everything above it, so measure against the neighbouring element — contentTop === jobBandBottom, not contentTop === 0.

Branch batch/168-228-254.

Claiming. Both halves of the Direction, since the second is the one that generalises. 1. `test.afterEach` emitting `JobsChanged` with `[]` in the two specs that stage jobs (`jobs-on-a-phone.spec.ts`, `top-bar-fit.spec.ts`), so the state they create dies with them. No backend change needed. 2. The rule written into `.pi/skills/yellowjacket-dev/references/harness.md`: an absolute coordinate in a shell measurement is an implicit claim about everything above it, so measure against the neighbouring element — `contentTop === jobBandBottom`, not `contentTop === 0`. Branch `batch/168-228-254`.
yonlu commented 2026-09-23 11:55:18 +00:00
Owner
Copy link
Copy source

Shipped on batch/168-228-254 — and one measured finding that changes the issue rather than confirming it.

Both halves are done. The two specs that stage jobs (jobs-on-a-phone.spec.ts, top-bar-fit.spec.ts) now clear them in test.afterEach with emit('JobsChanged', []) — the store replaces its whole list from every snapshot, so no testctl change is needed. The rule is written into .pi/skills/yellowjacket-dev/references/harness.md: measure against the element next to you, not an absolute coordinate, because an absolute number in a shell measurement is also a claim about everything above it.

The leak did not reproduce, and the mechanism as described cannot happen. A temporary probe, with a positive control:

  • the control staged a job on its own page and asserted the shell actually moves at a phone width — job-band renders a row and .main-panel's top goes from 0 to 55. So the measurement can see a job.
  • the very next test, staging nothing, had no job row and top === 0. It passed.

The reason is the page lifecycle. Every test gets a fresh page, and JobStore.init() refetches GetJobs() from the backend registry on mount — while /__test/emit calls events.Deliver, which touches connected frontends and no backend state (backend/testctl/handlers_dev.go). So a staged job is gone before the next spec starts, and it is gone before the next make e2e run too. Nothing in the suite registers a real job either (no spec calls AddLibrary/StartScan).

That means the 55px offset the draft assertion saw in that suite run had another cause, and it is not in evidence. I could not reproduce it and did not invent one; if it comes back, the reproduction to start from is that probe rather than the emit.

The cleanup stays anyway — it costs a line, it is what makes the state's owner explicit, and it would matter the moment one spec keeps a page alive — but the comments in both files now say what was measured instead of asserting the mechanism.

Verified: make e2e full suite, 255 passed (chromium), and make skill-check.

Shipped on `batch/168-228-254` — and one measured finding that changes the issue rather than confirming it. **Both halves are done.** The two specs that stage jobs (`jobs-on-a-phone.spec.ts`, `top-bar-fit.spec.ts`) now clear them in `test.afterEach` with `emit('JobsChanged', [])` — the store replaces its whole list from every snapshot, so no `testctl` change is needed. The rule is written into `.pi/skills/yellowjacket-dev/references/harness.md`: measure against the element next to you, not an absolute coordinate, because an absolute number in a shell measurement is also a claim about everything above it. **The leak did not reproduce, and the mechanism as described cannot happen.** A temporary probe, with a positive control: - the control staged a job on its own page and asserted the shell actually moves at a phone width — `job-band` renders a row and `.main-panel`'s top goes from 0 to 55. So the measurement can see a job. - the very next test, staging nothing, had no job row and `top === 0`. It passed. The reason is the page lifecycle. Every test gets a **fresh page**, and `JobStore.init()` refetches `GetJobs()` from the backend registry on mount — while `/__test/emit` calls `events.Deliver`, which touches connected frontends and **no backend state** (`backend/testctl/handlers_dev.go`). So a staged job is gone before the next spec starts, and it is gone before the next `make e2e` run too. Nothing in the suite registers a real job either (no spec calls `AddLibrary`/`StartScan`). That means the 55px offset the draft assertion saw in that suite run had **another cause, and it is not in evidence**. I could not reproduce it and did not invent one; if it comes back, the reproduction to start from is that probe rather than the emit. The cleanup stays anyway — it costs a line, it is what makes the state's owner explicit, and it would matter the moment one spec keeps a page alive — but the comments in both files now say what was measured instead of asserting the mechanism. Verified: `make e2e` full suite, 255 passed (chromium), and `make skill-check`.
yonlu closed this issue 2026-09-27 01:45:20 +00:00
gitea-actions Bot removed the
Status
In Progress
 label 2026-09-27 01:45:56 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Labels
Clear labels
Area/Design
Area/Downloads
Area/Explore
Area/Library-UI
Area/Metadata
Area/Packaging
Area/Player
Area/Queue
Area/Settings
Area/Shell-Nav
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
 Platform/Android
Platform/Desktop
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Medium
3
The priority is medium
Priority
Low
4
The priority is low
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
In Progress
Somebody is actively working on this right now
No labels Kind/Testing
Priority
Low
4
Reviewed
Confirmed
1
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logan yonlu
No Assignees yonlu
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: yonlu/yellowjacket#168