zombor/pergamum
1
0
Fork 0
Code Issues Pull requests 1 Projects Releases Packages 2 Wiki Activity Actions

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

Closed
zombor wants to merge 2 commits from bd-bookshelf-d6a0j into main
pull from: bd-bookshelf-d6a0j
merge into: zombor:main
Conversation 4 Commits 2 Files changed 3 +327 -249
zombor commented 2026-08-04 19:39:17 +00:00
Owner
Copy link

Summary

  • Deletes e2e/browser/journey_move_toast_test.go — the #1 remaining E2E-Browser flake source. The browser test resisted 3 separate fix attempts (b8p9v, #1330, eu18m) and still timed out ~60s in BeforeAll on every CI run due to the async bulk-move workflow + SSE toast being genuinely slow in a real Chromium under runner contention.
  • Adds e2e/api/journey_14_bulk_move_workflow_test.go: an Ordered journey that boots a fresh DB+app with a real wfengine (WFEngine:true), seeds src/dst libraries and 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.
  • Coverage is equivalent-or-better: the toast was a thin browser UI layer over the MoveBooksWorkflow completion; this journey asserts the same terminal workflow state directly at the API/state level (same pattern as Journey-6's EnrichWorkflow and RecalcMatchScoresWorkflow guards).

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 should no longer run the flaky move_toast journey; E2E API suite should run Journey-14

Closes bead bookshelf-d6a0j on merge.

## Summary - Deletes `e2e/browser/journey_move_toast_test.go` — the #1 remaining E2E-Browser flake source. The browser test resisted 3 separate fix attempts (b8p9v, #1330, eu18m) and still timed out ~60s in BeforeAll on every CI run due to the async bulk-move workflow + SSE toast being genuinely slow in a real Chromium under runner contention. - Adds `e2e/api/journey_14_bulk_move_workflow_test.go`: an `Ordered` journey that boots a fresh DB+app with a real wfengine (`WFEngine:true`), seeds src/dst libraries and 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. - Coverage is equivalent-or-better: the toast was a thin browser UI layer over the MoveBooksWorkflow completion; this journey asserts the same terminal workflow state directly at the API/state level (same pattern as Journey-6's EnrichWorkflow and RecalcMatchScoresWorkflow guards). ## 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 should no longer run the flaky move_toast journey; E2E API suite should run Journey-14 Closes bead bookshelf-d6a0j on merge.
test(e2e): migrate move_toast from browser to e2e/api (bookshelf-d6a0j)
Some checks failed
/ JS Unit Tests (pull_request) Failing after 4m43s
Details
/ Test Race (pull_request) Successful in 10m46s
Details
/ E2E API (pull_request) Successful in 10m40s
Details
/ E2E Browser (pull_request) Failing after 11m45s
Details
/ Integration (pull_request) Failing after 15m35s
Details
/ Lint (pull_request) Successful in 16m0s
Details
/ Coverage (pull_request) Successful in 19m2s
Details
e7c9ee0085
 
Delete e2e/browser/journey_move_toast_test.go (the #1 remaining E2E-Browser
rerun source — resisted 3 browser fixes and still timed out ~60s in BeforeAll
on every run). The toast was a thin UI layer over the MoveBooksWorkflow;
asserting the same underlying behavior at the API tier is equivalent-or-better
coverage and eliminates the timing dependence on Chromium + SSE polling.

Add e2e/api/journey_14_bulk_move_workflow_test.go: an Ordered journey that
boots a fresh DB+app with a real wfengine (WFEngine:true), seeds source and
destination libraries with temp-dir paths and 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 (the
workflow completes quickly — books have no real files, so processOneMove returns
"source file missing" immediately). This proves the full POST→wfengine→status
wiring without browser timing flakes.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
zombor commented 2026-08-04 20:25:29 +00:00
Author
Owner
Copy link

Security Review — PR #1332 (bookshelf-d6a0j)

This is a pure test migration: delete e2e/browser/journey_move_toast_test.go, add e2e/api/journey_14_bulk_move_workflow_test.go. No production code is touched.

Key questions checked

1. Did the deleted browser test guard any security behaviour?
No. The two browser It blocks asserted only DOM/toast CSS classes (.app-toast--info, .app-toast--error). Authentication, permission enforcement, and library scoping are all enforced at middleware/handler layer — not at the Stimulus toast layer. Deleting the browser test removes zero security coverage.

2. Does the new API journey still exercise the auth + scoped path?
Yes.

  • Both the POST /books/bulk/move and GET /books/bulk/move/{instanceID}/status calls use authClient — a session-authenticated *http.Client that carries the JWT session cookie. This traverses the global AuthMiddleware → PermissionRequired(BookMoveOrganizeRequired) → handler gate. An unauthenticated request would be rejected before reaching the handler.
  • Both srcLibID and dstLibID are seeded with user_library_mapping rows for user 1, so the handler's getUserLibraryIDs + filterOwnedIDs path runs against genuinely user-owned resources.
  • The status endpoint exercises the ownerID == sessionUserID cross-user ownership check: the workflow is enqueued with the session user's ID and polled by the same session client.

3. CSP / inline style? Not present. Toast assertions in the deleted test used class selectors only. No style= attribute introduced.

4. Any other surfaces? No production code changed. No secrets, PII, SSRF, injection, domain-boundary, or workflow-versioning concerns apply to this diff.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #1332 (bookshelf-d6a0j) This is a pure test migration: delete `e2e/browser/journey_move_toast_test.go`, add `e2e/api/journey_14_bulk_move_workflow_test.go`. No production code is touched. ### Key questions checked **1. Did the deleted browser test guard any security behaviour?** No. The two browser `It` blocks asserted only DOM/toast CSS classes (`.app-toast--info`, `.app-toast--error`). Authentication, permission enforcement, and library scoping are all enforced at middleware/handler layer — not at the Stimulus toast layer. Deleting the browser test removes zero security coverage. **2. Does the new API journey still exercise the auth + scoped path?** Yes. - Both the `POST /books/bulk/move` and `GET /books/bulk/move/{instanceID}/status` calls use `authClient` — a session-authenticated `*http.Client` that carries the JWT session cookie. This traverses the global `AuthMiddleware` → `PermissionRequired(BookMoveOrganizeRequired)` → handler gate. An unauthenticated request would be rejected before reaching the handler. - Both `srcLibID` and `dstLibID` are seeded with `user_library_mapping` rows for user 1, so the handler's `getUserLibraryIDs` + `filterOwnedIDs` path runs against genuinely user-owned resources. - The status endpoint exercises the `ownerID == sessionUserID` cross-user ownership check: the workflow is enqueued with the session user's ID and polled by the same session client. **3. CSP / inline style?** Not present. Toast assertions in the deleted test used class selectors only. No `style=` attribute introduced. **4. Any other surfaces?** No production code changed. No secrets, PII, SSRF, injection, domain-boundary, or workflow-versioning concerns apply to this diff. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor
zombor commented 2026-08-04 20:27:18 +00:00
Author
Owner
Copy link

Code Review — bookshelf-d6a0j (PR #1332)

Phase 1: Spec Compliance

The bead goal — migrate the flaky move-toast browser journey to an equivalent e2e/api assertion — is substantially met. The new journey_14 journey boots a fresh wfengine app, enqueues a real MoveBooksWorkflow, polls the status endpoint, and asserts terminal state. Justification comment is present. Ordered + BeforeAll pattern matches existing journeys (Journey-6 template). No production code touched. Deletion removes the full journey_move_toast_test.go with no dangling references.

One coverage gap and one assertion weakness warrant flags.

Phase 2: Findings

[MAJOR] e2e/api/journey_14_bulk_move_workflow_test.go:183 — _pollMoveStatus JS controller is now untested anywhere

The deleted browser journey was the only test that exercised the _pollMoveStatus Stimulus method (books_organize_controller.js lines 594–648 — 5 references). The replacement e2e/api journey proves the server-side wiring (HTTP handler → wfengine → status endpoint) but exercises no JS at all. The static/js/test/books_organize_controller.test.js file has zero tests for _pollMoveStatus (grepped: 0 hits in test file vs 5 in implementation). This means the entire polling-and-toast-resolution path — fetch(statusURL), response branching on data.state, moved/failed count extraction, and the _showFlash variant selection — is now untested.

The bead description calls the replacement "equivalent coverage," but it is not equivalent on the JS side: it is a deliberate trade (JS coverage → API wiring coverage). That trade may be intentional, but removing the only test for a non-trivial JS code path without adding a Vitest replacement is a real gap. The _pollMoveStatus logic has at least four distinct branches (running→retry, completed+failed>0→error toast, completed+moved→success toast, non-ok response→error toast), none of which are now covered.

Suggested fix: add a Vitest unit test in static/js/test/books_organize_controller.test.js for _pollMoveStatus covering the four branches (running→retry, completed+failures, completed+success, 4xx terminal). This does not require Chromium (fetch is already stubbed in the existing test file with vi.fn()), so it belongs in the Vitest tier per CLAUDE.md policy.

[MINOR] e2e/api/journey_14_bulk_move_workflow_test.go:215 — Expect(result).NotTo(BeEmpty()) is a weak terminal-state assertion

The second It asserts state == "completed" and result is non-empty JSON. However, per the project conventions (review-standard.md → "go-workflows marks errored-finished workflows as 'completed'"), state == "completed" alone is insufficient to distinguish a successful registration from a wiring failure. Journey-6's RecalcMatchScoresWorkflow step addresses this by asserting Result == "null" (the specific value a nil-error workflow emits). Here, the workflow always completes with failed=1 (source file missing), so Result == "null" is not the right check — but result could be strengthened. For example: unmarshal the result JSON and assert result.failed == 1 and result.moved == 0, which would catch a wiring error that produces a malformed or empty result payload vs a test-setup error that produces a correct payload with failures.

Suggested fix: after the polling loop, unmarshal result into MoveBooksResultPayload (or a local struct) and assert failed == 1, moved == 0 — proving the workflow ran to a real terminal state, not just that some bytes came back.

REVIEW VERDICT: 0 blocker, 1 major, 1 minor

## Code Review — bookshelf-d6a0j (PR #1332) ### Phase 1: Spec Compliance The bead goal — migrate the flaky move-toast browser journey to an equivalent e2e/api assertion — is substantially met. The new `journey_14` journey boots a fresh wfengine app, enqueues a real MoveBooksWorkflow, polls the status endpoint, and asserts terminal state. Justification comment is present. `Ordered` + `BeforeAll` pattern matches existing journeys (Journey-6 template). No production code touched. Deletion removes the full `journey_move_toast_test.go` with no dangling references. One coverage gap and one assertion weakness warrant flags. --- ### Phase 2: Findings [MAJOR] e2e/api/journey_14_bulk_move_workflow_test.go:183 — `_pollMoveStatus` JS controller is now untested anywhere The deleted browser journey was the only test that exercised the `_pollMoveStatus` Stimulus method (books_organize_controller.js lines 594–648 — 5 references). The replacement e2e/api journey proves the *server-side* wiring (HTTP handler → wfengine → status endpoint) but exercises no JS at all. The `static/js/test/books_organize_controller.test.js` file has zero tests for `_pollMoveStatus` (grepped: 0 hits in test file vs 5 in implementation). This means the entire polling-and-toast-resolution path — `fetch(statusURL)`, response branching on `data.state`, `moved`/`failed` count extraction, and the `_showFlash` variant selection — is now untested. The bead description calls the replacement "equivalent coverage," but it is not equivalent on the JS side: it is a deliberate trade (JS coverage → API wiring coverage). That trade may be intentional, but removing the only test for a non-trivial JS code path without adding a Vitest replacement is a real gap. The `_pollMoveStatus` logic has at least four distinct branches (running→retry, completed+failed>0→error toast, completed+moved→success toast, non-ok response→error toast), none of which are now covered. Suggested fix: add a Vitest unit test in `static/js/test/books_organize_controller.test.js` for `_pollMoveStatus` covering the four branches (running→retry, completed+failures, completed+success, 4xx terminal). This does not require Chromium (fetch is already stubbed in the existing test file with `vi.fn()`), so it belongs in the Vitest tier per CLAUDE.md policy. --- [MINOR] e2e/api/journey_14_bulk_move_workflow_test.go:215 — `Expect(result).NotTo(BeEmpty())` is a weak terminal-state assertion The second It asserts `state == "completed"` and `result` is non-empty JSON. However, per the project conventions (`review-standard.md` → "go-workflows marks errored-finished workflows as 'completed'"), `state == "completed"` alone is insufficient to distinguish a successful registration from a wiring failure. Journey-6's RecalcMatchScoresWorkflow step addresses this by asserting `Result == "null"` (the specific value a nil-error workflow emits). Here, the workflow always completes with `failed=1` (source file missing), so `Result == "null"` is not the right check — but `result` could be strengthened. For example: unmarshal the result JSON and assert `result.failed == 1` and `result.moved == 0`, which would catch a wiring error that produces a malformed or empty result payload vs a test-setup error that produces a correct payload with failures. Suggested fix: after the polling loop, unmarshal `result` into `MoveBooksResultPayload` (or a local struct) and assert `failed == 1, moved == 0` — proving the workflow ran to a real terminal state, not just that some bytes came back. --- REVIEW VERDICT: 0 blocker, 1 major, 1 minor
test: add _pollMoveStatus Vitest coverage + strengthen bulk-move terminal assertion (bookshelf-d6a0j review)
Some checks failed
/ JS Unit Tests (pull_request) Successful in 5m3s
Details
/ E2E API (pull_request) Successful in 5m53s
Details
/ Test Race (pull_request) Successful in 9m35s
Details
/ E2E Browser (pull_request) Failing after 10m27s
Details
/ Coverage (pull_request) Successful in 13m43s
Details
/ Integration (pull_request) Failing after 15m58s
Details
/ Lint (pull_request) Successful in 16m3s
Details
21eece2c5c
 
[MAJOR] Add dedicated _pollMoveStatus Vitest describe block 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.
Calls _pollMoveStatus() directly on the mounted controller, independent of
the full confirm-move flow.

[MINOR] Strengthen Journey-14 terminal assertion: unmarshal BulkMoveResult
and assert failed==1, moved==0 (file missing → permanent error) instead of
NotTo(BeEmpty()) — catches malformed/empty-result wiring bugs that go-workflows
would surface as "completed" with no result.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
zombor commented 2026-08-04 21:15:17 +00:00
Author
Owner
Copy link

Re-review: bookshelf-d6a0j (PR #1332)

Verifying closure of the 1 MAJOR + 1 MINOR from the prior review.

MAJOR — _pollMoveStatus untested: CLOSED

static/js/test/books_organize_controller.test.js now has a dedicated describe("_pollMoveStatus — branch coverage") block (diff lines 1751–1851) with four direct-call tests:

  • (a) running-state → schedules retry, no success toast yet, then succeeds on next poll (Moved 1 file(s), success)
  • (b) completed with failed > 0 → error toast containing failed count (1 failed, error)
  • (c) completed with failed === 0 → success toast with moved count (Moved 3 file(s), success)
  • (d) !r.ok (4xx) → unavailable error toast, fetch called exactly once (no retry)

All four tests drive _pollMoveStatus directly on a mounted controller via stubbed fetch and assert the exact AppDialog.toast variant + message content. The network-error-retry path (err !== null catch) and the max-attempts timeout branch (>= MOVE_POLL_MAX → Still moving) are covered in adjacent _confirmMove tests (lines 1006–1078) that existed before this PR and remain green. Every branch of the method is exercised through the public controller interface. MAJOR closed.

MINOR — weak terminal assertion in e2e: CLOSED

e2e/api/journey_14_bulk_move_workflow_test.go now unmarshals BulkMoveResult and asserts exact field values:

Expect(bulkResult.Failed).To(Equal(1), ...)
Expect(bulkResult.Moved).To(Equal(0), ...)

This catches the go-workflows "errored-finished surfaces as completed with no result" regression that NotTo(BeEmpty()) would miss. MINOR closed.

No new issues found

The e2e multiple-Expect pattern in the single status It step is permitted by the CLAUDE.md Multi-Expect relaxation for e2e journeys. The time.Sleep(300ms) poll loop is an e2e real-workflow poll, not a wall-clock assertion — the standard pattern used by Journey-6. Journey-14 is Ordered as required by the e2e policy guard.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Re-review: bookshelf-d6a0j (PR #1332) Verifying closure of the 1 MAJOR + 1 MINOR from the prior review. ### MAJOR — _pollMoveStatus untested: CLOSED `static/js/test/books_organize_controller.test.js` now has a dedicated `describe("_pollMoveStatus — branch coverage")` block (diff lines 1751–1851) with four direct-call tests: - **(a)** running-state → schedules retry, no success toast yet, then succeeds on next poll (`Moved 1 file(s)`, `success`) - **(b)** completed with `failed > 0` → error toast containing failed count (`1 failed`, `error`) - **(c)** completed with `failed === 0` → success toast with moved count (`Moved 3 file(s)`, `success`) - **(d)** `!r.ok` (4xx) → `unavailable` error toast, `fetch` called exactly once (no retry) All four tests drive `_pollMoveStatus` directly on a mounted controller via stubbed `fetch` and assert the exact `AppDialog.toast` variant + message content. The network-error-retry path (`err !== null` catch) and the max-attempts timeout branch (`>= MOVE_POLL_MAX` → `Still moving`) are covered in adjacent `_confirmMove` tests (lines 1006–1078) that existed before this PR and remain green. Every branch of the method is exercised through the public controller interface. MAJOR closed. ### MINOR — weak terminal assertion in e2e: CLOSED `e2e/api/journey_14_bulk_move_workflow_test.go` now unmarshals `BulkMoveResult` and asserts exact field values: ```go Expect(bulkResult.Failed).To(Equal(1), ...) Expect(bulkResult.Moved).To(Equal(0), ...) ``` This catches the go-workflows "errored-finished surfaces as completed with no result" regression that `NotTo(BeEmpty())` would miss. MINOR closed. ### No new issues found The e2e multiple-`Expect` pattern in the single status `It` step is permitted by the CLAUDE.md Multi-Expect relaxation for e2e journeys. The `time.Sleep(300ms)` poll loop is an e2e real-workflow poll, not a wall-clock assertion — the standard pattern used by Journey-6. Journey-14 is `Ordered` as required by the e2e policy guard. REVIEW VERDICT: 0 blocker, 0 major, 0 minor
zombor commented 2026-08-05 15:04:53 +00:00
Author
Owner
Copy link

Closing as superseded — the move_toast browser→API migration was independently completed and merged as PR #1339 (bookshelf-xif3r): journey_move_toast_test.go is deleted and e2e/api/journey_14_bulk_move_workflow_test.go is on main. This PR's changes are now redundant/conflicting.

Closing as superseded — the move_toast browser→API migration was independently completed and merged as PR #1339 (bookshelf-xif3r): journey_move_toast_test.go is deleted and e2e/api/journey_14_bulk_move_workflow_test.go is on main. This PR's changes are now redundant/conflicting.
zombor closed this pull request 2026-08-05 15:04:57 +00:00
Some checks failed
/ JS Unit Tests (pull_request) Successful in 5m3s
Details
/ E2E API (pull_request) Successful in 5m53s
Required
Details
/ Test Race (pull_request) Successful in 9m35s
Required
Details
/ E2E Browser (pull_request) Failing after 10m27s
Required
Details
/ Coverage (pull_request) Successful in 13m43s
Required
Details
/ Integration (pull_request) Failing after 15m58s
Required
Details
/ Lint (pull_request) Successful in 16m3s
Required
Details

Pull request closed

Please reopen this pull request to perform a merge.
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
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!1332
No description provided.