fix(e2e): sweep MustEval-in-Eventually to Eval+error-check (bookshelf-b96py) #1349

Merged

zombor merged 1 commit from bd-bookshelf-b96py into main 2026-08-06 00:55:59 +00:00

Conversation 3 Commits 1 Files changed 20 +440 -117

zombor commented 2026-08-06 00:20:49 +00:00

Owner

Copy link

Summary

• Converted all page.MustEval() calls used as the return value of Eventually(...) polling callbacks to page.Eval() with an err != nil check that returns the zero value, so CDP transient errors (tab busy, session timeout, eval throw) cause the poll to retry instead of panicking the spec.
• 20 files changed, ~75 conversion sites across func() bool, func() string, and func() int poll callbacks.
• Standalone one-shot MustEval calls after the element is confirmed present are left unchanged — panics there are legitimate failures, not flakes.
• Mirrors the eu18m (#1331) sweep for MustElement-in-Eventually and the ydjo9 (#1342) canonical conversion pattern.

Test plan

• go build -tags e2e ./e2e/browser/... compiles clean.
• CI browser e2e suite confirms no regressions.
• Flake class eliminated: CDP transient errors inside Eventually no longer abort the spec.

Closes bead bookshelf-b96py on merge.

## Summary - Converted all `page.MustEval()` calls used as the return value of `Eventually(...)` polling callbacks to `page.Eval()` with an `err != nil` check that returns the zero value, so CDP transient errors (tab busy, session timeout, eval throw) cause the poll to retry instead of panicking the spec. - 20 files changed, ~75 conversion sites across `func() bool`, `func() string`, and `func() int` poll callbacks. - Standalone one-shot `MustEval` calls after the element is confirmed present are left unchanged — panics there are legitimate failures, not flakes. - Mirrors the eu18m (#1331) sweep for `MustElement`-in-`Eventually` and the ydjo9 (#1342) canonical conversion pattern. ## Test plan - `go build -tags e2e ./e2e/browser/...` compiles clean. - CI browser e2e suite confirms no regressions. - Flake class eliminated: CDP transient errors inside `Eventually` no longer abort the spec. Closes bead bookshelf-b96py on merge.

zombor added 1 commit 2026-08-06 00:20:49 +00:00

fix(e2e): sweep MustEval-inside-Eventually to Eval+error-check (bookshelf-b96py) ... 36f3fe0945

Convert all page.MustEval() calls used as the return value of Eventually polling
callbacks to page.Eval() with an error check returning the zero value on error,
so CDP transient errors cause the poll to retry rather than panicking the spec.

Scope: 20 files, ~75 conversion sites across bool/string/int polling conditions.
Standalone one-shot MustEval calls after the element is confirmed present are
unchanged — panics there are legitimate failures, not flakes. Mirrors the eu18m
(#1331) and ydjo9 (#1342) sweep patterns for MustElement-in-Eventually.

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

zombor commented 2026-08-06 00:33:13 +00:00

Author

Owner

Copy link

Security Review — PR #1349 (bookshelf-b96py)

Scope verification complete. All 20 changed files are under e2e/browser/ — no production code, no middleware, no CSP/auth/header changes were included in this sweep.

What was checked:

• File list: 100% e2e/browser/*_test.go — no smuggled production changes.
• Secrets/credentials: no hardcoded tokens, API keys, or credentials appear in any added JS expression or Go code.
• Security assertions: no test that previously asserted a security control (CSP, auth redirect, ownership 404) had its assertion silently weakened or removed. The JS expressions inside Eval() calls are byte-for-byte identical to the original MustEval() calls — only the Go wrapper changed from panic-on-error to err return with return false/""/0 on failure.
• Pattern uniformity: every addition matches exactly the mechanical conversion pattern (result, err := page.Eval(...) + if err != nil { return <zero> } + return result.Value.<Type>()). No structural additions beyond this pattern were found.

This is a pure test-robustness sweep with no security surface.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #1349 (bookshelf-b96py) Scope verification complete. All 20 changed files are under `e2e/browser/` — no production code, no middleware, no CSP/auth/header changes were included in this sweep. **What was checked:** - File list: 100% `e2e/browser/*_test.go` — no smuggled production changes. - Secrets/credentials: no hardcoded tokens, API keys, or credentials appear in any added JS expression or Go code. - Security assertions: no test that previously asserted a security control (CSP, auth redirect, ownership 404) had its assertion silently weakened or removed. The JS expressions inside `Eval()` calls are byte-for-byte identical to the original `MustEval()` calls — only the Go wrapper changed from panic-on-error to `err` return with `return false`/`""`/`0` on failure. - Pattern uniformity: every addition matches exactly the mechanical conversion pattern (`result, err := page.Eval(...)` + `if err != nil { return <zero> }` + `return result.Value.<Type>()`). No structural additions beyond this pattern were found. This is a pure test-robustness sweep with no security surface. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-06 00:37:01 +00:00

Author

Owner

Copy link

Code Review — bookshelf-b96py (MustEval sweep)

[MAJOR] e2e/browser/journey_book_send_email_test.go:123 — MustEval inside Eventually not converted (2 sites)
Lines 123 and 148 are page.MustEval(...) calls sitting inside Eventually(func() bool { ... }) poll loops. A transient CDP error in either call will panic, aborting the spec (the same class of flake this PR exists to fix). Both follow the exact same pattern as the converted sites.
Fix: apply the same result, err := page.Eval(...); if err != nil { return false }; return result.Value.Bool() conversion to both Eventually bodies (lines 122–131 and 147–155).

[MAJOR] e2e/browser/journey_magic_shelf_rating_dropdown_test.go:66 — MustEval inside Eventually not converted (1 site)
Line 66: return page.MustEval(\() => document.getElementById("magic-shelf-modal") !== null`).Bool()is inside anEventually(func() bool { ... })block. A transient CDP error panics and kills the spec. Fix: convert toresult, err := page.Eval(...); if err != nil { return false }; return result.Value.Bool()`.

[MAJOR] e2e/browser/journey_reset_progress_test.go:94 — MustEval inside Eventually not converted (4 sites)
Lines 94, 110, 121, and 138 are all page.MustEval(...) calls inside Eventually(func() bool { ... }) poll loops; each polls the kebab menu is-open class or the .apd-overlay hidden attribute. A CDP error at any of these panics the spec.
Fix: same conversion pattern applied to all four Eventually bodies.

---

Scope discipline: All 75+ converted sites in the 20 modified files are correctly scoped — every Eval conversion is inside an Eventually/poll block, and standalone one-shot MustEval calls (assertions, DOM manipulations, setup actions) are correctly left as-is.

Conversion correctness: The error-handling pattern is uniformly correct. Two minor style variants appear (if err != nil { return false } vs return evalErr == nil && result.Value.Bool()) but both are safe — the short-circuit && prevents nil-deref on the result pointer when err is non-nil. No nil-deref risk found.

Semantic equivalence: MustEval() returns gson.JSON directly (per go-rod must.go:487), so the old .String() called gson.JSON.String() (the fmt.Stringer implementation: fmt.Sprintf("%v", v)). The new result.Value.Str() calls gson.JSON.Str(). For string-returning JS expressions, both produce the same result. The .Bool() and .Int() paths are identical (result.Value is the same gson.JSON the old code held).

Non-e2e files: Zero — the diff touches only e2e/browser/ test files. No production code changed, no assertions weakened.

REVIEW VERDICT: 0 blocker, 3 major, 0 minor

## Code Review — bookshelf-b96py (MustEval sweep) [MAJOR] e2e/browser/journey_book_send_email_test.go:123 — MustEval inside Eventually not converted (2 sites) Lines 123 and 148 are `page.MustEval(...)` calls sitting **inside** `Eventually(func() bool { ... })` poll loops. A transient CDP error in either call will **panic**, aborting the spec (the same class of flake this PR exists to fix). Both follow the exact same pattern as the converted sites. Fix: apply the same `result, err := page.Eval(...); if err != nil { return false }; return result.Value.Bool()` conversion to both `Eventually` bodies (lines 122–131 and 147–155). [MAJOR] e2e/browser/journey_magic_shelf_rating_dropdown_test.go:66 — MustEval inside Eventually not converted (1 site) Line 66: `return page.MustEval(\`() => document.getElementById("magic-shelf-modal") !== null\`).Bool()` is inside an `Eventually(func() bool { ... })` block. A transient CDP error panics and kills the spec. Fix: convert to `result, err := page.Eval(...); if err != nil { return false }; return result.Value.Bool()`. [MAJOR] e2e/browser/journey_reset_progress_test.go:94 — MustEval inside Eventually not converted (4 sites) Lines 94, 110, 121, and 138 are all `page.MustEval(...)` calls inside `Eventually(func() bool { ... })` poll loops; each polls the kebab menu `is-open` class or the `.apd-overlay` hidden attribute. A CDP error at any of these panics the spec. Fix: same conversion pattern applied to all four `Eventually` bodies. --- **Scope discipline:** All 75+ converted sites in the 20 modified files are correctly scoped — every `Eval` conversion is inside an `Eventually`/poll block, and standalone one-shot `MustEval` calls (assertions, DOM manipulations, setup actions) are correctly left as-is. **Conversion correctness:** The error-handling pattern is uniformly correct. Two minor style variants appear (`if err != nil { return false }` vs `return evalErr == nil && result.Value.Bool()`) but both are safe — the short-circuit `&&` prevents nil-deref on the result pointer when err is non-nil. No nil-deref risk found. **Semantic equivalence:** `MustEval()` returns `gson.JSON` directly (per go-rod `must.go:487`), so the old `.String()` called `gson.JSON.String()` (the `fmt.Stringer` implementation: `fmt.Sprintf("%v", v)`). The new `result.Value.Str()` calls `gson.JSON.Str()`. For string-returning JS expressions, both produce the same result. The `.Bool()` and `.Int()` paths are identical (`result.Value` is the same `gson.JSON` the old code held). **Non-e2e files:** Zero — the diff touches only `e2e/browser/` test files. No production code changed, no assertions weakened. REVIEW VERDICT: 0 blocker, 3 major, 0 minor

zombor commented 2026-08-06 00:42:15 +00:00

Author

Owner

Copy link

Orchestrator note — the 3 [MAJOR] findings in the code review (comment 16624) are FALSE POSITIVES from a stale review base. Verified against origin/bd-bookshelf-b96py and origin/main:

• journey_book_send_email_test.go — does not exist on this branch or on main (migrated to Vitest by bz643.2 / #1344, merged earlier today). The review diffed a pre-#1344 base.
• journey_magic_shelf_rating_dropdown_test.go — does not exist on this branch or main (same #1344 migration).
• journey_reset_progress_test.go — already converted by ydjo9 / #1342: every Eventually(func() bool {…}) body uses page.Eval(…)+err; zero MustEval present.

A full branch sweep (context-scan of every e2e/browser/*_test.go) finds no MustEval inside any Eventually block; all remaining MustEval calls are standalone one-shots, correctly left unchanged. Effective verdict: 0 blocker, 0 major, 0 minor. Proceeding to merge.

**Orchestrator note — the 3 [MAJOR] findings in the code review (comment 16624) are FALSE POSITIVES from a stale review base.** Verified against `origin/bd-bookshelf-b96py` and `origin/main`: - `journey_book_send_email_test.go` — **does not exist** on this branch or on `main` (migrated to Vitest by bz643.2 / #1344, merged earlier today). The review diffed a pre-#1344 base. - `journey_magic_shelf_rating_dropdown_test.go` — **does not exist** on this branch or `main` (same #1344 migration). - `journey_reset_progress_test.go` — **already converted** by ydjo9 / #1342: every `Eventually(func() bool {…})` body uses `page.Eval(…)+err`; zero `MustEval` present. A full branch sweep (context-scan of every `e2e/browser/*_test.go`) finds **no `MustEval` inside any `Eventually` block**; all remaining `MustEval` calls are standalone one-shots, correctly left unchanged. Effective verdict: **0 blocker, 0 major, 0 minor**. Proceeding to merge.

zombor force-pushed bd-bookshelf-b96py from 36f3fe0945 to 1f4e132f76 2026-08-06 00:48:50 +00:00 Compare

zombor merged commit 3ffa5100fd into main 2026-08-06 00:55:59 +00:00

zombor referenced this pull request from a commit 2026-08-06 00:56:02 +00:00

Merge pull request 'fix(e2e): sweep MustEval-in-Eventually to Eval+error-check (bookshelf-b96py)' (#1349) from bd-bookshelf-b96py into main

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

No description provided.