test(e2e): split multi-Expect It + drop bare sleep (bookshelf-tr2i) #531

Merged

zombor merged 1 commit from bd-bookshelf-tr2i into main 2026-06-12 16:05:41 +00:00

Conversation 2 Commits 1 Files changed 1 +36 -22

zombor commented 2026-06-12 15:02:24 +00:00

Owner

Copy link

Summary

• Split the single It block in metadata_compare_cover_screenshot_test.go (which had 4 Expect calls) into 4 single-Expect It blocks, each sharing the same BeforeEach/JustBeforeEach per project convention (one Expect per It).
• Removed the bare time.Sleep(200 * time.Millisecond) "layout settle" — DOM injection via MustEval is synchronous and MustElement already gates on presence; the wall-clock sleep was redundant and a flake risk.
• Added proper BeforeEach/JustBeforeEach structure: setup (nav, DOM inject, screenshot) in BeforeEach; width reads in JustBeforeEach; assertions in It.
• No behaviour change — tests verify the same CSS constraints.

Test plan

• make build — compiles cleanly
• CI e2e browser tests pass (same assertions, now split across 4 Its)

Closes bead bookshelf-tr2i on merge.

## Summary - Split the single `It` block in `metadata_compare_cover_screenshot_test.go` (which had 4 `Expect` calls) into 4 single-Expect `It` blocks, each sharing the same `BeforeEach`/`JustBeforeEach` per project convention (one Expect per It). - Removed the bare `time.Sleep(200 * time.Millisecond)` "layout settle" — DOM injection via `MustEval` is synchronous and `MustElement` already gates on presence; the wall-clock sleep was redundant and a flake risk. - Added proper `BeforeEach`/`JustBeforeEach` structure: setup (nav, DOM inject, screenshot) in `BeforeEach`; width reads in `JustBeforeEach`; assertions in `It`. - No behaviour change — tests verify the same CSS constraints. ## Test plan - [ ] `make build` — compiles cleanly - [ ] CI e2e browser tests pass (same assertions, now split across 4 Its) Closes bead bookshelf-tr2i on merge.

zombor added 1 commit 2026-06-12 15:02:24 +00:00

test(e2e): split multi-Expect It + drop bare sleep (bookshelf-tr2i) ... a07e29d1ed

Split the single It block in metadata_compare_cover_screenshot_test.go
(4 Expects) into 4 single-Expect It blocks sharing BeforeEach/JustBeforeEach.
Remove the bare time.Sleep(200ms) — DOM injection via MustEval is synchronous
and MustElement already gates on presence; the sleep was redundant.

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

zombor commented 2026-06-12 15:16:38 +00:00

Author

Owner

Copy link

Security Review — bookshelf-tr2i (PR #531)

Scope: e2e/browser/metadata_compare_cover_screenshot_test.go only.

Checks performed:

• Secrets/credentials: No tokens, API keys, passwords, or credentials introduced. The only external interaction is the existing uploadMetadataCoverScreenshotToPR call, unchanged.
• Insecure TLS / NoSandbox: No TLS configuration changes. NoSandbox not referenced; launcher config is untouched.
• Network egress: No new outbound endpoints introduced. The screenshot-upload call to the Forgejo PR was already present; diff merely moves it from the It block into BeforeEach.
• Command/HTML injection via test inputs: Test inputs are hard-coded string literals ("Cover Thumbnail Test Book") and integer IDs. No user-controlled or externally sourced data flows into shell commands or HTML template contexts.
• Test harness weakening: The change improves harness structure (one Expect per It, DeferCleanup over bare defer, removal of a wall-clock time.Sleep). No assertion is removed — the four assertions from the original It are preserved across four new It blocks. The BeforeEach/JustBeforeEach split is correct per project conventions.
• Temp file write (/tmp/...): Pre-existing pattern, unchanged in risk profile. Fixed-name suffix is process-indexed (GinkgoParallelProcess()), preventing parallel-test clobber.

Findings: None.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — bookshelf-tr2i (PR #531) **Scope:** `e2e/browser/metadata_compare_cover_screenshot_test.go` only. **Checks performed:** - **Secrets/credentials:** No tokens, API keys, passwords, or credentials introduced. The only external interaction is the existing `uploadMetadataCoverScreenshotToPR` call, unchanged. - **Insecure TLS / NoSandbox:** No TLS configuration changes. `NoSandbox` not referenced; launcher config is untouched. - **Network egress:** No new outbound endpoints introduced. The screenshot-upload call to the Forgejo PR was already present; diff merely moves it from the `It` block into `BeforeEach`. - **Command/HTML injection via test inputs:** Test inputs are hard-coded string literals (`"Cover Thumbnail Test Book"`) and integer IDs. No user-controlled or externally sourced data flows into shell commands or HTML template contexts. - **Test harness weakening:** The change improves harness structure (one `Expect` per `It`, `DeferCleanup` over bare `defer`, removal of a wall-clock `time.Sleep`). No assertion is removed — the four assertions from the original `It` are preserved across four new `It` blocks. The `BeforeEach`/`JustBeforeEach` split is correct per project conventions. - **Temp file write (`/tmp/...`):** Pre-existing pattern, unchanged in risk profile. Fixed-name suffix is process-indexed (`GinkgoParallelProcess()`), preventing parallel-test clobber. **Findings:** None. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-06-12 15:17:00 +00:00

Author

Owner

Copy link

Code Review — bookshelf-tr2i

Reviewed: git diff origin/main...origin/bd-bookshelf-tr2i

Phase 0: DEMO verification

No DEMO block. This is a test-hygiene refactor (no new behavior); no runnable demo is applicable. Proceeding directly to code quality.

Phase 1: Spec compliance

All three bead requirements met:

• 4-Expect It split into 4 single-Expect Its with shared BeforeEach/JustBeforeEach ✓
• time.Sleep(200ms) removed ✓
• BeforeEach/JustBeforeEach structure added ✓

Phase 2: Code quality

Structure:

• BeforeEach is pure setup: page creation, DeferCleanup, seedBook, setAuthCookies, navigation, DOM injection via MustEval, MustElement presence wait, screenshot + upload. Correct.
• JustBeforeEach captures the two getBoundingClientRect() width readings. Correct separation.
• 4 It blocks each have exactly 1 Expect. Correct.

Sleep removal safety:
MustEval is a synchronous round-trip (JS runs to completion before the Go call returns). MustElement("[data-mf-modal]") confirms DOM presence before the width-reading MustEval calls in JustBeforeEach. getBoundingClientRect() is itself synchronous JS — the browser's layout engine computes it before returning. The 200ms sleep was not masking a real async gate; its removal does not introduce a race.

DeferCleanup vs defer:
The old code used defer page.MustClose() inside a single It. The new code correctly uses DeferCleanup(page.MustClose) inside BeforeEach — the Ginkgo-idiomatic way to ensure cleanup runs after each It when setup lives in BeforeEach. Improvement.

Imports:
"time" removed (no longer used). "github.com/go-rod/rod" added and used for the *rod.Page type in the outer var block. Clean.

bookID scoping:
bookID is correctly kept as a var bookID int64 local to BeforeEach since it is only needed within that block to construct the navigation URL.

Findings

No blockers, majors, or minors.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Code Review — bookshelf-tr2i **Reviewed:** `git diff origin/main...origin/bd-bookshelf-tr2i` ### Phase 0: DEMO verification No DEMO block. This is a test-hygiene refactor (no new behavior); no runnable demo is applicable. Proceeding directly to code quality. ### Phase 1: Spec compliance All three bead requirements met: - 4-Expect `It` split into 4 single-Expect `Its` with shared `BeforeEach`/`JustBeforeEach` ✓ - `time.Sleep(200ms)` removed ✓ - `BeforeEach`/`JustBeforeEach` structure added ✓ ### Phase 2: Code quality **Structure:** - `BeforeEach` is pure setup: page creation, `DeferCleanup`, `seedBook`, `setAuthCookies`, navigation, DOM injection via `MustEval`, `MustElement` presence wait, screenshot + upload. Correct. - `JustBeforeEach` captures the two `getBoundingClientRect()` width readings. Correct separation. - 4 `It` blocks each have exactly 1 `Expect`. Correct. **Sleep removal safety:** `MustEval` is a synchronous round-trip (JS runs to completion before the Go call returns). `MustElement("[data-mf-modal]")` confirms DOM presence before the width-reading `MustEval` calls in `JustBeforeEach`. `getBoundingClientRect()` is itself synchronous JS — the browser's layout engine computes it before returning. The 200ms sleep was not masking a real async gate; its removal does not introduce a race. **`DeferCleanup` vs `defer`:** The old code used `defer page.MustClose()` inside a single `It`. The new code correctly uses `DeferCleanup(page.MustClose)` inside `BeforeEach` — the Ginkgo-idiomatic way to ensure cleanup runs after each `It` when setup lives in `BeforeEach`. Improvement. **Imports:** `"time"` removed (no longer used). `"github.com/go-rod/rod"` added and used for the `*rod.Page` type in the outer `var` block. Clean. **`bookID` scoping:** `bookID` is correctly kept as a `var bookID int64` local to `BeforeEach` since it is only needed within that block to construct the navigation URL. ### Findings No blockers, majors, or minors. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor force-pushed bd-bookshelf-tr2i from a07e29d1ed to 2206945ac7 2026-06-12 15:44:18 +00:00 Compare

zombor merged commit e4b713fb4a into main 2026-06-12 16:05:41 +00:00

zombor referenced this pull request from a commit 2026-06-12 16:05:43 +00:00

Merge pull request 'test(e2e): split multi-Expect It + drop bare sleep (bookshelf-tr2i)' (#531) from bd-bookshelf-tr2i into main

zombor referenced this pull request from a commit 2026-06-13 01:11:02 +00:00

test(stats): cover buildHeatmapWeeks mid-week break — fix coverage gate (bookshelf-q9sg)

zombor referenced this pull request 2026-06-13 01:11:13 +00:00

Fix: cover buildHeatmapWeeks break — unblock coverage gate (bookshelf-q9sg) #548

zombor referenced this pull request from a commit 2026-06-13 01:22:08 +00:00

test(stats): cover buildHeatmapWeeks mid-week break — fix coverage gate (bookshelf-q9sg)

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!531

No description provided.