test(e2e): move pure-Stimulus browser specs to Vitest (bookshelf-bz643.2) #1344
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-bz643.2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Migrates 7 browser
Describeblocks (24Its) to Vitest controller unit tests, reducing the oversized browser e2e suite from 78 Describes / 319 Its to 73 Describes / 298 Its — moving toward the ~45-spec / ~12-Describe budget in CLAUDE.md.This is a coverage-migration: each behavior asserted by a deleted browser Describe is verified in the corresponding Vitest controller test first.
Updated after code review: Two coverage gaps identified by review (BLOCKER + MAJOR) were restored:
.bookdrop-action-barusingclosest()on the live DOM; jsdom flat fixture cannot express this.{{if $.LLMVisionAvailable}}template gate shows/hides the Scan All Files button based on real app_settings state.Coverage parity mapping
journey_book_send_email_test.go(8 Its)book_send_email_controller.test.jsjourney_magic_shelf_rating_dropdown_test.go(4 Its)magic_shelf_rule_controller.test.jsjourney_book_detail_subtabs_test.goSubTab Its (3 Its)sub_tab_controller.test.jsjourney_book_file_replace_test.go(3 Its)book_file_replace_controller.test.jsbookdrop_bottom_bar_test.goenable/disable Its (2 Its)bookdrop_review_controller.test.jsbookdrop_bulk_edit_test.go(1 It)bookdrop_bulk_edit_controller.test.jsbookdrop_import_metadata_test.go(1 It)bookdrop_import_metadata_controller.test.jsjourney_library_scan_file_test.goAppDialog It (1 It)library_kebab_menu_controller.test.jsNew Vitest tests added
static/js/test/bookdrop_review_controller.test.js— 2 new tests in theclearSelection()block:disables acceptBtn after clearing a previous selectiondisables rejectBtn after clearing a previous selectionRestored browser tests (review fixes)
e2e/browser/bookdrop_bottom_bar_test.go— 1 structural It (DOM layout guard, closest() on live server DOM)e2e/browser/journey_library_scan_file_test.go— 2 focused Its (LLMVisionAvailable template gate)Test plan
make test— Go unit tests passmake coverage— 100% internal/ coverage gatemake e2e-policy-check— all remaining browser Describes are Ordered journeysmake js-test(Vitest) — 4616 tests pass (4614 baseline + 2 new)go build -tags e2e ./e2e/browser/...— no compile errorsCloses bead bookshelf-bz643.2 on merge.
b727e8a093e2b2bb1632Security Review — PR #1344 (
bookshelf-bz643.2)Test-only diff: 7 browser
Describeblocks deleted, 2 Vitest tests added. No production code changed.Scope
Checked per
.claude/rules/review-standard.md:Findings
Production code impact: Zero. Every changed file is
e2e/browser/*_test.go(deleted) orstatic/js/test/bookdrop_review_controller.test.js(2 tests added). No handler, service, template, or migration touched.Deleted spec security audit:
journey_book_send_email(8 Its deleted): Asserted that the server-rendered modal contained the stub user's seeded providers/recipients. This is a wiring/rendering check, not an authz check. Per-user email provider scoping is tested at the service layer ininternal/email/service_test.go(ListProviders scoped byuserID). No orphaned security coverage.journey_magic_shelf_rating_dropdown(4 Its deleted): Asserted that canonicalcontent_rating/age_rating<select>options render in the rule builder. The Vitest replacement inmagic_shelf_rule_controller.test.jsexhaustively coversEVERYONE/TEEN/MATURE/ADULT/EXPLICITand the numeric age_rating set. No security control was in this test.journey_book_detail_subtabs(3 Its deleted — panel-click and deep-link Its only): Asserted panel visibility and deep-link activation. Pure UI-state tests. No authz or ownership assertions. The remaining journey structure Its (not deleted) continue to cover the controller mount.journey_book_file_replace(3 Its deleted): Asserted modal open/close on Replace button click. No authz assertion. Server-side file ownership and CSRF are tested at the handler layer.bookdrop_bottom_bar(2 Its deleted): Asserted button layout (in.bookdrop-action-bar, not.bookdrop-toolbar) and enable/disable state on selection. Pure UI/layout tests. The Vitest additions coverclearSelection()disablingacceptBtn/rejectBtn.bookdrop_bulk_edit(1 It deleted): Asserted a server-side DB write (publisher inoriginal_metadata). Server-side coverage exists:BulkEditMetadataHandlertest ininternal/bookdrop/review_handler_test.gocovers the handler logic. The route is gated byBulkEditMetadataRequiredmiddleware (RBAC). No orphaned security coverage.bookdrop_import_metadata(1 It deleted): Asserted cross-controllerbookdrop:selectionchangeevent enables the Import Metadata button. Vitestbookdrop_import_metadata_controller.test.jscovers this event explicitly (enables openBtn when selectionchange fires with ids). No security control.journey_library_scan_file(2 Its deleted): Asserted theLLMVisionAvailabletemplate gate renders the "Scan All Files for Metadata" button and thatAppDialog.confirm()shows the count dialog. TheLLMVisionAvailablegate is a server-side template condition; its presence/absence is a server rendering concern not a JS security control. The Vitestlibrary_kebab_menu_controller.test.jscoversscanFiles()+AppDialog.confirm()interaction exhaustively (8 test cases including absent-AppDialog guard). No orphaned security coverage.Secrets/PII: None. Fixture data uses
smtp.example.com,u@example.com,kindle@example.com— clearly synthetic test values.REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Code Review — bookshelf-bz643.2 (E2E Browser Suite Reduction, Slice 1)
PR status: open, mergeable: True. CI: E2E Browser is red on this SHA, but the failure is
journey_reset_progress_test.go:113(a pre-existing timeout flake in a different journey — "Reset Reading Progress" kebab item timed out after 5s). The failing spec is unrelated to this PR's changes.COVERAGE PARITY ANALYSIS
The bead plan states: "expand the Vitest test BEFORE deleting the browser Describe". The diff is 1156 lines deleted, 20 lines added. Five of the seven deleted browser Describes have corresponding Vitest files — but the diff shows zero changes to those five files (
book_send_email_controller.test.js,magic_shelf_rule_controller.test.js,sub_tab_controller.test.js,book_file_replace_controller.test.js,books_select_scan_file_controller.test.js). They existed on main before this PR and are byte-for-byte identical on the branch. The Vitest coverage already present on main is therefore what this PR relies on — the question is whether it was already sufficient.[BLOCKER] e2e/browser/bookdrop_bottom_bar_test.go — structural layout assertions dropped with no Vitest equivalent
The deleted browser suite had two Its. The first (
renders Accept Selected and Reject Selected inside the .bookdrop-action-bar, not in the toolbar) asserted DOM structure:acceptBtn.closest('.bookdrop-action-bar') !== nullANDacceptBtn.closest('.bookdrop-toolbar') === null, plusselectionCountinside.bookdrop-action-bar. This is not about Stimulus logic — it is a template layout regression test. The new Vitest additions instatic/js/test/bookdrop_review_controller.test.js:1224–1242only assert enable/disable behavior ofacceptBtn/rejectBtnafterclearSelection(). ThemountReviewfixture (lines ~113–200) placesacceptBtn,rejectBtn, andselectionCountas direct children of the controller root with no.bookdrop-action-barcontainer, so a structural test could not be written in the existing fixture without changing it. The second It (Accept and Reject buttons become enabled after selecting a row, disabled after clearing) was correctly migrated:clearSelection()disable behavior is now covered. But the structural layout behavior (buttons must be inside.bookdrop-action-barand outside.bookdrop-toolbar) is now untested anywhere. This is a template layout regression guard — the kind of thing the review standard cites as needing a browser test (interactive UI needs browser test/bookdrop-action-barplacement). Either a new Vitest test that places the buttons inside a.bookdrop-action-barcontainer and assertsclosest(), or reinstate this specific It.[MAJOR] e2e/browser/journey_library_scan_file_test.go — Chromium-gated behavior not fully covered in Vitest
The deleted
It("shows the count confirmation dialog when clicking Scan All Files for Metadata in the library kebab")exercised two behaviors: (1) thescanFiles()AppDialog confirm flow — covered bylibrary_kebab_menu_controller.test.js:1454which already existed on main; (2) the LLM vision feature-gate in the server-rendered template ({{if $.LLMVisionAvailable}}controls whether the sidebar even renders the "Scan All Files for Metadata" button). That template gate is not testable in Vitest — the browser test was the only proof that the gate works end-to-end (enabling config → button appears; disabling → button absent). That gate-verification behavior is now unverified. The justification comment in the deleted file (gated by {{if $.LLMVisionAvailable}}) correctly identified this as Chromium-required. Severity raised from MINOR because a regressed visibility gate would silently ship the button to users without LLM vision configured (or hide it from users who have it). Suggested fix: retain a single focused It that seeds LLM vision config, navigates to the sidebar, and asserts the button presence/absence — the AppDialog flow can stay in Vitest.[MINOR] e2e/browser/bookdrop_import_metadata_test.go — deletion justification overstated but behavior is covered
The browser test comment said cross-controller event wiring "requires real Chromium Stimulus connections". This is not accurate:
bookdrop_import_metadata_controller.test.js(already on main) dispatchesbookdrop:selectionchangedirectly and assertsopenBtn.disabledchanges — this is the same behavior. The deletion is substantively safe. The comment was a false Chromium justification that should have been updated before deletion, but it is not a coverage gap.[MINOR] e2e/browser/journey_book_detail_subtabs_test.go:487 —
?subtab=filesdeep-link not explicitly named in Vitest but behavior is coveredThe deleted It
"Files sub-tab is active when navigating with ?tab=details&subtab=files deep-link"navigated to?tab=details&subtab=filesand asserted#subtab-panel-filesbecame visible. Thesub_tab_controller.test.jsCONNECT describe covers?subtab=notesURL param activation. The underlying controller logic (read param → activate matching panel) is identical regardless of which panel ID. The behavior is covered. The risk flagged in the bead as "coverage-risk #9" is assessed as acceptable: the Vitest test exercised the code path; the browser test's value was the full-stack navigation with a real server-rendered book page, which the remaining structure Its injourney_book_detail_subtabs_test.gopartially preserve.[MINOR] static/js/test/bookdrop_review_controller.test.js:1230 — new tests call
onRowCheckboxChange()directly rather than simulating achangeeventThe two new Its call
m.ctrl.onRowCheckboxChange()directly (white-box method invocation) instead of dispatching achangeevent on the checkbox. This is within a_testpackage equivalent (the file is already white-box in the JS sense — it calls controller methods directly), and the existing Vitest suite uses the same pattern extensively, so this is a convention nit rather than a new violation. No coverage impact.e2e-policy-check / count: Removed 7 browser Describe blocks (5 deleted files + 2 Its from
journey_book_detail_subtabs_test.go). The PR moves the suite in the right direction toward the ~45 It target. All remainingDescribeblocks carryOrdered. Policy compliance is maintained.Mergeable: True. CI E2E Browser failure is
journey_reset_progress_test.go:113— a pre-existing flake unrelated to this PR.REVIEW VERDICT: 1 blocker, 1 major, 3 minor
e2b2bb16321a9da505acRe-review — bz643.2 restoration delta (coverage fix commit
1a9da505)Scope: restoration commit only —
e2e/browser/bookdrop_bottom_bar_test.go(new) ande2e/browser/journey_library_scan_file_test.go(replaced). The migration itself was reviewed in comment 16522.CI status at
1a9da505: E2E Browser pass, Coverage pass, Integration pass, Lint pass, JS Unit Tests pass, E2E API pass. Test Race shows failure (pre-existinginternal/bookdrop/watcher_test.gointermittent flake, confirmed not in this PR's diff — not a blocker).BLOCKER fix verification — bookdrop_bottom_bar_test.go
Selector correctness: The restored It queries
[data-bookdrop-review-target="acceptBtn"]and calls.closest('.bookdrop-action-bar'). Cross-referencingtemplates/pages/bookdrop_index.html:662,695: the.bookdrop-action-bardiv is the real containing element, andacceptBtn/rejectBtnare nested inside it. The selector matches the live DOM precisely.Positive and negative assertions both present:
.bookdrop-action-bar— asserted true (line 65).bookdrop-action-bar— asserted true (line 72).bookdrop-toolbar— asserted true (line 80)selectionCountin.bookdrop-action-bar— asserted true (line 88)The negative assertion (
closest('.bookdrop-toolbar') === null) is exactly what the original BLOCKER finding required.State adequacy: BeforeAll inserts a library (required because
acceptBtn/rejectBtnonly render inside the{{if $.Libraries}}branch) and twoPENDING_REVIEWbookdrop files. Without the library the template renders a "No libraries configured" hint instead of the buttons — the setup is correct.Structural/flake checks: Single page per It (fresh incognito),
refreshPageTimeoutcalled after navigation,waitForStimulusController(page, "bookdrop-review")ensures the controller is connected beforepage.Eval. MultipleExpectcalls in a single It — within the browser e2e multi-Expect relaxation (CLAUDE.md). No fixed-sleep anti-patterns.BLOCKER: RESOLVED. The restored It covers exactly the behavior the blocker identified.
MAJOR fix verification — journey_library_scan_file_test.go
Both gate states covered:
seedLLMVisionConfig(true)before navigation, thenEventuallyassertsquerySelector('[data-action*="scanFiles"]') !== nullfor up to 10s.DeferCleanup(func() { seedLLMVisionConfig(false) })from It#1. In GinkgoOrdered,DeferCleanupregistered inside anItruns immediately after thatItcompletes — before the nextItstarts. By the time It#2 executes,seedLLMVisionConfig(false)has already fired. The comment on line 73 correctly documents this dependency.Selector matches template:
[data-action*="scanFiles"]matchesdata-action="click->library-kebab-menu#scanFiles"attemplates/layouts/base.html:224. The button is inside the{{if $.LLMVisionAvailable}}block (line 220), confirming the gate is real.Scope scoping: Both Its use
[data-library-kebab-menu-library-id-value="%d"]as a parent scope for the toggle and scanFiles selectors, correctly scoping to the seeded library."Absent" assertion robustness: It#2 uses
Consistently(..., 2s, 200ms).Should(BeFalse()). The kebab content is server-rendered (confirmed bylibrary_kebab_menu_controller.js— no fetch of menu content), so all DOM is present beforetoggle.MustClick(). The Consistently probe correctly tests that the element never appears over a 2s window after the menu is opened, not a point-in-time check that could race.Serialdeclaration: The Describe carriesOrdered, SerialbecauseseedLLMVisionConfigmutates the sharedapp_settingstable. This prevents parallel-node interference with other LLM Vision journey specs. Correct.Per-It page timeout: Each It creates its own fresh
incognito.MustPage().Timeout(pageTimeout)— no sharedpageacross Its, so no per-It timeout-reset concern applies.MAJOR: RESOLVED. Both gate states are covered with robust assertions.
Form-check: policy compliance
package browser_testin both files. Black-box.DescribecarryOrdered. e2e-policy-check compliant.BeforeEach(suiteEnv.ResetDB())per-spec anti-pattern.Evalcalls are insideEventually/Consistentlyprobes or called afterwaitForStimulusController.Nothing else regressed
The restoration commit touches exactly two files (both test files, no production code). The Describe/It count change from the migration commit (78/319 -> 73/298) is consistent with removing 5+2 browser specs and adding 3 Its here (1 bookdrop-bar + 2 LLM-gate).
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
1a9da505ac964f04b13a