fix(e2e): migrate move_toast from browser to API tier (bookshelf-xif3r) #1339

Merged

zombor merged 1 commit from bd-bookshelf-xif3r into main 2026-08-05 02:55:45 +00:00

Conversation 2 Commits 1 Files changed 3 +327 -249

zombor commented 2026-08-05 02:43:38 +00:00

Owner

Copy link

Summary

• Deletes `e2e/browser/journey_move_toast_test.go` — the last remaining E2E Browser flake. The test hit `context.deadlineExceeded` on line 167 (`els[0].Click` on `.books-select-bar-organize`) inside a 45s `Eventually` under CI load. Three prior fix attempts (b8p9v, eu18m, #1330) confirmed the test is correctly structured (non-blocking `page.Elements`, `refreshPageTimeout` resets, 45s budgets) — the root cause is the inherently-racy async bulk-move workflow + DOM update cycle that cannot be reliably asserted at the browser tier within CI time constraints.
• Adds `e2e/api/journey_14_bulk_move_workflow_test.go`: an `Ordered` journey that boots a fresh DB+app with a real wfengine worker, seeds src/dst libraries + a book, enqueues a bulk move via `POST /books/bulk/move` (asserts 202 + `instance_id`), then polls `GET /books/bulk/move/{instanceID}/status` until `"completed"` within 60s and asserts `failed==1, moved==0` (file missing on disk → permanent error).
• Adds `_pollMoveStatus` Vitest tests to `static/js/test/books_organize_controller.test.js` covering all 4 branches directly: (a) running→retry, (b) completed+failed>0→error toast, (c) completed+failed=0→success toast, (d) 4xx terminal→unavailable toast.

Coverage equivalence: the browser toast was a thin UI layer over the same MoveBooksWorkflow completion. Journey-14 asserts the same terminal workflow state directly at the API/state level (same pattern as Journey-6's EnrichWorkflow/RecalcMatchScoresWorkflow guards); the Vitest tests assert every JS branch of the polling controller.

Test plan

• `make build` — green
• `make test` — green
• `make e2e-policy-check` — green (all top-level Describes are Ordered)
• `go build -tags e2e ./e2e/...` — compiles clean
• CI: E2E Browser suite no longer runs the flaky move_toast journey; E2E API suite runs Journey-14

Closes bead bookshelf-xif3r on merge.

## Summary - Deletes \`e2e/browser/journey_move_toast_test.go\` — the last remaining E2E Browser flake. The test hit \`context.deadlineExceeded\` on line 167 (\`els[0].Click\` on \`.books-select-bar-organize\`) inside a 45s \`Eventually\` under CI load. Three prior fix attempts (b8p9v, eu18m, #1330) confirmed the test is correctly structured (non-blocking \`page.Elements\`, \`refreshPageTimeout\` resets, 45s budgets) — the root cause is the inherently-racy async bulk-move workflow + DOM update cycle that cannot be reliably asserted at the browser tier within CI time constraints. - Adds \`e2e/api/journey_14_bulk_move_workflow_test.go\`: an \`Ordered\` journey that boots a fresh DB+app with a real wfengine worker, seeds src/dst libraries + a book, enqueues a bulk move via \`POST /books/bulk/move\` (asserts 202 + \`instance_id\`), then polls \`GET /books/bulk/move/{instanceID}/status\` until \`"completed"\` within 60s and asserts \`failed==1, moved==0\` (file missing on disk → permanent error). - Adds \`_pollMoveStatus\` Vitest tests to \`static/js/test/books_organize_controller.test.js\` covering all 4 branches directly: (a) running→retry, (b) completed+failed>0→error toast, (c) completed+failed=0→success toast, (d) 4xx terminal→unavailable toast. **Coverage equivalence:** the browser toast was a thin UI layer over the same MoveBooksWorkflow completion. Journey-14 asserts the same terminal workflow state directly at the API/state level (same pattern as Journey-6's EnrichWorkflow/RecalcMatchScoresWorkflow guards); the Vitest tests assert every JS branch of the polling controller. ## Test plan - [x] \`make build\` — green - [x] \`make test\` — green - [x] \`make e2e-policy-check\` — green (all top-level Describes are Ordered) - [x] \`go build -tags e2e ./e2e/...\` — compiles clean - [ ] CI: E2E Browser suite no longer runs the flaky move_toast journey; E2E API suite runs Journey-14 Closes bead bookshelf-xif3r on merge.

zombor added 1 commit 2026-08-05 02:43:39 +00:00

fix(e2e): migrate move_toast from browser to e2e/api tier (bookshelf-xif3r) ... f35615e38b

Deletes e2e/browser/journey_move_toast_test.go — the remaining E2E Browser
flake (context.deadlineExceeded at line 167 on els[0].Click inside a 45s
Eventually). The toast behavior is driven by an async bulk-move workflow
+ SSE toast cycle that is genuinely slow in real Chromium under runner
contention; 3 prior fix attempts (b8p9v, eu18m, #1330) confirmed the test
is correctly structured (non-blocking Elements, refreshPageTimeout, 45s
budgets) but the inherently-racy browser→workflow→DOM timing makes it
impossible to reliably assert at the browser tier within CI time constraints.

Per CLAUDE.md E2E policy: async workflow terminal state belongs at the API
tier, not a racy browser journey. Adds Journey-14 to e2e/api/ which boots
a fresh DB+app with a real wfengine worker, POST /books/bulk/move, and polls
GET .../status until "completed" within 60s — identical behavioral coverage
without Chromium timing dependency. Also adds direct _pollMoveStatus Vitest
tests covering all 4 branches (running→retry, completed+failed, completed+ok,
4xx terminal) in static/js/test/books_organize_controller.test.js.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-08-05 02:54:19 +00:00

Author

Owner

Copy link

Security Review — bookshelf-xif3r / PR #1339

Reviewed per .claude/rules/review-standard.md.

Scope examined

1. Whether deleting e2e/browser/journey_move_toast_test.go drops any security-relevant assertion not re-covered by the replacement.
2. Whether the new API journey (journey_14_bulk_move_workflow_test.go) exercises the bulk-move endpoint with correct user scoping.
3. Secrets / PII in new test fixtures.
4. Production-code change confirmation.

---

Findings

No findings.

Deleted browser test — security coverage audit:
The deleted browser journey made exactly two assertions: (a) an info toast appears immediately after the move POST, and (b) an error toast appears once the workflow completes with failures. Both are DOM/JS-layer assertions about the Stimulus controller toast UI — neither is an authZ, cross-user scoping, CSP, or ownership assertion. There is no security-relevant assertion in the deleted file that is not re-covered.

New API journey — user scoping:
The journey seeds books and libraries mapped to user_id=1 via user_library_mapping, then issues requests through authClient (the standard authenticated session for user 1). The POST /books/bulk/move handler derives userID from the authenticated session (not the request body), filters source books via filterOwnedIDs(userLibraryIDs), and filters destination libraries via ownedLibSet — both keyed off the session userID. The GET .../status handler explicitly checks ownerID != sessionUserID and returns not-found on mismatch. The cross-user denial path (user B polling user A instanceID) is covered by the unit test at internal/bookmove/move_handler_test.go:1001 ("wrong user — ownership mismatch"). The new e2e journey correctly exercises the happy path; the authZ negative path is the right tier for unit tests and is already there.

Wall-clock timing in polling loop:
The time.Now().Before(deadline) / time.Sleep(300ms) polling pattern mirrors Journey-6 (e2e/api/journey_6_wfengine_test.go:239,258) which established this as the accepted idiom for async wfengine assertions. The assertion itself (Expect(state).To(Equal("completed"))) is on the workflow terminal state, not on elapsed time — the flake-prevention rule for wall-clock-in-assertion-path does not apply.

Secrets / PII: None. All fixture data is synthetic (library names Journey14Src/Journey14Dst, book title Journey14 Test Book).

Production code change: Zero. The diff touches only e2e/api/, e2e/browser/, and static/js/test/ — no internal/, cmd/, templates/, static/css/, or Makefile changes.

E2E policy compliance: The new file is a single Ordered Describe with a single BeforeAll that boots one fresh DB+app, satisfying the journey policy. It includes the required justification comment.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — bookshelf-xif3r / PR #1339 Reviewed per `.claude/rules/review-standard.md`. ### Scope examined 1. Whether deleting `e2e/browser/journey_move_toast_test.go` drops any security-relevant assertion not re-covered by the replacement. 2. Whether the new API journey (`journey_14_bulk_move_workflow_test.go`) exercises the bulk-move endpoint with correct user scoping. 3. Secrets / PII in new test fixtures. 4. Production-code change confirmation. --- ### Findings No findings. **Deleted browser test — security coverage audit:** The deleted browser journey made exactly two assertions: (a) an info toast appears immediately after the move POST, and (b) an error toast appears once the workflow completes with failures. Both are DOM/JS-layer assertions about the Stimulus controller toast UI — neither is an authZ, cross-user scoping, CSP, or ownership assertion. There is no security-relevant assertion in the deleted file that is not re-covered. **New API journey — user scoping:** The journey seeds books and libraries mapped to `user_id=1` via `user_library_mapping`, then issues requests through `authClient` (the standard authenticated session for user 1). The `POST /books/bulk/move` handler derives `userID` from the authenticated session (not the request body), filters source books via `filterOwnedIDs(userLibraryIDs)`, and filters destination libraries via `ownedLibSet` — both keyed off the session userID. The `GET .../status` handler explicitly checks `ownerID != sessionUserID` and returns not-found on mismatch. The cross-user denial path (user B polling user A instanceID) is covered by the unit test at `internal/bookmove/move_handler_test.go:1001` ("wrong user — ownership mismatch"). The new e2e journey correctly exercises the happy path; the authZ negative path is the right tier for unit tests and is already there. **Wall-clock timing in polling loop:** The `time.Now().Before(deadline)` / `time.Sleep(300ms)` polling pattern mirrors Journey-6 (`e2e/api/journey_6_wfengine_test.go:239,258`) which established this as the accepted idiom for async wfengine assertions. The assertion itself (`Expect(state).To(Equal("completed"))`) is on the workflow terminal state, not on elapsed time — the flake-prevention rule for wall-clock-in-assertion-path does not apply. **Secrets / PII:** None. All fixture data is synthetic (library names `Journey14Src`/`Journey14Dst`, book title `Journey14 Test Book`). **Production code change:** Zero. The diff touches only `e2e/api/`, `e2e/browser/`, and `static/js/test/` — no `internal/`, `cmd/`, `templates/`, `static/css/`, or `Makefile` changes. **E2E policy compliance:** The new file is a single `Ordered` `Describe` with a single `BeforeAll` that boots one fresh DB+app, satisfying the journey policy. It includes the required justification comment. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-05 02:54:20 +00:00

Author

Owner

Copy link

Code Review — bookshelf-xif3r (PR #1339)

CI + Mergeability

CI state: success (all 7 jobs green). Mergeable: True.

---

Phase 1: Spec Compliance

Bead goal: fix an intermittent E2E Browser flake by migrating the racy bulk-move journey from the browser tier to the API tier. The diff does exactly this — deletes the browser journey, adds an API-tier Journey-14, and adds Vitest branch coverage for _pollMoveStatus. No extra work, no missing requirements.

Coverage parity: the browser journey tested two behaviours via CSS selector assertions: an info toast appearing immediately, and an error toast appearing when the workflow completes with failures. Per CLAUDE.md browser policy: "Browser It asserting only a CSS selector → API test." The deletion is policy-correct. The API journey covers the wiring (POST 202 + workflow completion + exact BulkMoveResult fields). The _pollMoveStatus Vitest tests cover all four JS branches including the success/error toast dispatch. Nothing substantive is lost.

---

Phase 2: Code Quality

e2e/api/journey_14_bulk_move_workflow_test.go — all checks pass:

• Ordered + BeforeAll booting a fresh DB+app once. No per-spec ResetDB() inside It blocks.
• Written justification is present and complete (3 reasons).
• Polling pattern (deadline := time.Now().Add(60s); for time.Now().Before(deadline)) is identical to Journey-6 lines 239/306/364 — established precedent. Assertion is on state == "completed" (string), not on elapsed time. Not the flake pattern the review standard targets.
• The second It has four Expect calls — allowed by the e2e Multi-Expect relaxation rule.
• srcPathID (line 103) is assigned and used at line 107 (INSERT INTO book). No dangling variable.
• File declares package api_test — black-box compliant.

static/js/test/books_organize_controller.test.js — all checks pass:

• vi.useFakeTimers() / vi.useRealTimers() in beforeEach/afterEach — correct.
• vi.advanceTimersByTime(2001) correctly drains the 2000ms MOVE_POLL_INTERVAL_MS setTimeout.
• The for (var i = 0; i < 8; i++) { await Promise.resolve(); } micro-task drain is the project-established pattern for flushing fetch().then().then() chains after fake-timer advance.
• Branch (a) walks running → completed across two timer advances with an intermediate assertion.
• Branches (b), (c), (d) are self-contained single-response tests matching exact toast message substrings from the controller.
• Branch (d) asserts fetch called exactly once — correctly verifies no retry after terminal 4xx.
• make e2e-policy-check will pass: single top-level var _ = Describe("...", Ordered, ...).

---

No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Code Review — bookshelf-xif3r (PR #1339) ### CI + Mergeability CI state: **success** (all 7 jobs green). Mergeable: **True**. --- ### Phase 1: Spec Compliance Bead goal: fix an intermittent E2E Browser flake by migrating the racy bulk-move journey from the browser tier to the API tier. The diff does exactly this — deletes the browser journey, adds an API-tier Journey-14, and adds Vitest branch coverage for `_pollMoveStatus`. No extra work, no missing requirements. Coverage parity: the browser journey tested two behaviours via CSS selector assertions: an info toast appearing immediately, and an error toast appearing when the workflow completes with failures. Per CLAUDE.md browser policy: "Browser `It` asserting only a CSS selector → API test." The deletion is policy-correct. The API journey covers the wiring (POST 202 + workflow completion + exact BulkMoveResult fields). The `_pollMoveStatus` Vitest tests cover all four JS branches including the success/error toast dispatch. Nothing substantive is lost. --- ### Phase 2: Code Quality **e2e/api/journey_14_bulk_move_workflow_test.go** — all checks pass: - `Ordered` + `BeforeAll` booting a fresh DB+app once. No per-spec ResetDB() inside It blocks. - Written justification is present and complete (3 reasons). - Polling pattern (`deadline := time.Now().Add(60s); for time.Now().Before(deadline)`) is identical to Journey-6 lines 239/306/364 — established precedent. Assertion is on `state == "completed"` (string), not on elapsed time. Not the flake pattern the review standard targets. - The second It has four Expect calls — allowed by the e2e Multi-Expect relaxation rule. - `srcPathID` (line 103) is assigned and used at line 107 (INSERT INTO book). No dangling variable. - File declares `package api_test` — black-box compliant. **static/js/test/books_organize_controller.test.js** — all checks pass: - `vi.useFakeTimers()` / `vi.useRealTimers()` in beforeEach/afterEach — correct. - `vi.advanceTimersByTime(2001)` correctly drains the 2000ms MOVE_POLL_INTERVAL_MS setTimeout. - The `for (var i = 0; i < 8; i++) { await Promise.resolve(); }` micro-task drain is the project-established pattern for flushing fetch().then().then() chains after fake-timer advance. - Branch (a) walks `running → completed` across two timer advances with an intermediate assertion. - Branches (b), (c), (d) are self-contained single-response tests matching exact toast message substrings from the controller. - Branch (d) asserts `fetch` called exactly once — correctly verifies no retry after terminal 4xx. - `make e2e-policy-check` will pass: single top-level `var _ = Describe("...", Ordered, ...)`. --- No findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor referenced this pull request 2026-08-05 02:54:36 +00:00

fix(js-test): harden timing-fragile metron detail-fetch tests with vi.waitFor (bookshelf-wro8e) #1337

zombor merged commit 4f13fca647 into main 2026-08-05 02:55:45 +00:00

zombor referenced this pull request from a commit 2026-08-05 02:55:49 +00:00

Merge pull request 'fix(e2e): migrate move_toast from browser to API tier (bookshelf-xif3r)' (#1339) from bd-bookshelf-xif3r into main

zombor referenced this pull request 2026-08-05 15:04:53 +00:00

test(e2e): migrate move_toast from browser to e2e/api (bookshelf-d6a0j) #1332

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1339

No description provided.