fix(e2e): sweep blocking page.Element()-in-Eventually to non-blocking page.Elements() (systemic go-rod de-flake) (bookshelf-eu18m) #1331

Merged

zombor merged 1 commit from bd-bookshelf-eu18m into main 2026-08-04 19:34:14 +00:00

Conversation 2 Commits 1 Files changed 23 +143 -129

zombor commented 2026-08-04 18:38:56 +00:00

Owner

Copy link

Summary

• Root cause: go-rod's page.Element(sel)/MustElement(sel) blocks internally up to the page timeout (e.g. 60s) when the element is absent, so using it inside Eventually(budget, interval) is broken: the inner call blocks past the Eventually budget and the retry loop never fires. This is the systemic root of the recurring 'context deadline exceeded' / BeforeAll-timeout go-rod flakes.
• Fix: Converted every such occurrence across the full e2e/browser suite to the non-blocking page.Elements(sel) pattern (returns immediately with empty slice when absent; check len(els) > 0 before indexing into els[0]). Same treatment for page.Has/MustHas and page.MustElements inside Eventually loops.
• Scope: 23 files touched, 53 individual conversions. The 3 files being deleted by concurrent bz643.1 PR (journey_oi1l2_screenshot, journey_library_counts_sidebar, journey_cover_card_series) are deliberately skipped.

Test plan

• go build -tags e2e ./e2e/browser/... compiles clean
• make e2e-policy-check passes (all Describes are Ordered journey containers)
• CI green (browser suite expected to be much more stable — no more Eventually-blocked-by-page-timeout flakes)

Closes bead bookshelf-eu18m on merge.

## Summary - **Root cause:** go-rod's `page.Element(sel)`/`MustElement(sel)` blocks internally up to the page timeout (e.g. 60s) when the element is absent, so using it inside `Eventually(budget, interval)` is broken: the inner call blocks past the Eventually budget and the retry loop never fires. This is the systemic root of the recurring 'context deadline exceeded' / BeforeAll-timeout go-rod flakes. - **Fix:** Converted every such occurrence across the full `e2e/browser` suite to the non-blocking `page.Elements(sel)` pattern (returns immediately with empty slice when absent; check `len(els) > 0` before indexing into `els[0]`). Same treatment for `page.Has`/`MustHas` and `page.MustElements` inside Eventually loops. - **Scope:** 23 files touched, 53 individual conversions. The 3 files being deleted by concurrent bz643.1 PR (`journey_oi1l2_screenshot`, `journey_library_counts_sidebar`, `journey_cover_card_series`) are deliberately skipped. ## Test plan - [x] `go build -tags e2e ./e2e/browser/...` compiles clean - [x] `make e2e-policy-check` passes (all Describes are Ordered journey containers) - [ ] CI green (browser suite expected to be much more stable — no more Eventually-blocked-by-page-timeout flakes) Closes bead bookshelf-eu18m on merge.

zombor added 1 commit 2026-08-04 18:38:57 +00:00

fix(e2e): sweep blocking page.Element()-in-Eventually to non-blocking page.Elements() (bookshelf-eu18m) ... fe556ad76c

go-rod's page.Element(sel)/MustElement(sel) blocks internally up to the
page timeout (e.g. 60s) when the element is absent, so using it as the
polled condition inside Eventually(budget, interval) is broken: the inner
call blocks PAST the Eventually budget and the retry loop never fires.

Convert every such occurrence across the full e2e/browser suite to the
non-blocking page.Elements(sel) pattern (returns immediately; check
len(els) > 0 before indexing). Also converts page.Has/MustHas and
page.MustElements inside Eventually loops for the same reason.

23 files touched, 53 individual conversions. Files being deleted by the
concurrent bz643.1 PR (journey_oi1l2_screenshot, journey_library_counts_
sidebar, journey_cover_card_series) are deliberately skipped.

Verified: go build -tags e2e ./e2e/browser/... compiles clean;
make e2e-policy-check passes.

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

zombor referenced this pull request 2026-08-04 18:51:40 +00:00

ci(e2e): add 2 E2E-Browser matrix shards alongside existing job (phase 2a) (bookshelf-nwo0v.2) #1322

zombor commented 2026-08-04 18:54:03 +00:00

Author

Owner

Copy link

Security Review — PR #1331 (bookshelf-eu18m)

Scope: 53 mechanical conversions of blocking page.Element() / page.MustHas() / page.MustElements() inside Eventually poll loops to the non-blocking page.Elements() equivalent, across 23 e2e/browser/ test files.

File boundary check

All 23 changed files are under e2e/browser/ and carry //go:build e2e + package browser_test. Zero production code files are touched. Confirmed mechanically — no file outside e2e/browser/ appears in the diff.

Security assertion integrity (CSP, auth, OIDC)

journey_read_book_test.go — two CSP checks (Expect(cspReader.Violations()).To(BeEmpty()) and Expect(cspDetail.Violations()).To(BeEmpty())) are both outside the converted Eventually blocks. The conversion only changes how the poll loop detects that the page has finished loading before the assertion fires. The Violations() assertions themselves are unchanged context lines.

journey_manage_library_test.go — same pattern. The Expect(csp.Violations()).To(BeEmpty()) assertion is an unchanged context line; only the preceding Eventually DOM-ready poll was converted.

journey_oidc_group_mapping_test.go — the converted blocks are purely DOM-visibility checks (panel open/closed, modal hidden attribute, mapping row presence/absence). The authorization logic under test is the server-side OIDC group → role mapping; the test exercises it by driving form submit and asserting DOM state changes, none of which is altered by the conversion.

All Violations(), BeEmpty(), ContainSubstring, Equal, BeNumerically Gomega matchers are unchanged. The conversion is symmetric: every previous err == nil && el != nil guard becomes err == nil && len(els) > 0; every el.SomeMethod() becomes els[0].SomeMethod(). No condition was weakened, inverted, or dropped.

No secrets / PII

No credentials, tokens, API keys, or PII appear in the diff.

Summary

This is a pure flake-fix mechanical refactor. The pattern changed is exactly as described: page.Element() blocks the poll loop for up to the page timeout when the element is absent; page.Elements() returns immediately with an empty slice, allowing Eventually to retry on its own cadence. No security assertion is weakened, no production code is modified, no authz/scoping logic is touched.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #1331 (bookshelf-eu18m) **Scope:** 53 mechanical conversions of blocking `page.Element()` / `page.MustHas()` / `page.MustElements()` inside `Eventually` poll loops to the non-blocking `page.Elements()` equivalent, across 23 `e2e/browser/` test files. ### File boundary check All 23 changed files are under `e2e/browser/` and carry `//go:build e2e` + `package browser_test`. Zero production code files are touched. Confirmed mechanically — no file outside `e2e/browser/` appears in the diff. ### Security assertion integrity (CSP, auth, OIDC) **journey_read_book_test.go** — two CSP checks (`Expect(cspReader.Violations()).To(BeEmpty())` and `Expect(cspDetail.Violations()).To(BeEmpty())`) are both **outside** the converted `Eventually` blocks. The conversion only changes how the poll loop detects that the page has finished loading before the assertion fires. The `Violations()` assertions themselves are unchanged context lines. **journey_manage_library_test.go** — same pattern. The `Expect(csp.Violations()).To(BeEmpty())` assertion is an unchanged context line; only the preceding `Eventually` DOM-ready poll was converted. **journey_oidc_group_mapping_test.go** — the converted blocks are purely DOM-visibility checks (panel open/closed, modal hidden attribute, mapping row presence/absence). The authorization logic under test is the server-side OIDC group → role mapping; the test exercises it by driving form submit and asserting DOM state changes, none of which is altered by the conversion. All `Violations()`, `BeEmpty()`, `ContainSubstring`, `Equal`, `BeNumerically` Gomega matchers are unchanged. The conversion is symmetric: every previous `err == nil && el != nil` guard becomes `err == nil && len(els) > 0`; every `el.SomeMethod()` becomes `els[0].SomeMethod()`. No condition was weakened, inverted, or dropped. ### No secrets / PII No credentials, tokens, API keys, or PII appear in the diff. ### Summary This is a pure flake-fix mechanical refactor. The pattern changed is exactly as described: `page.Element()` blocks the poll loop for up to the page timeout when the element is absent; `page.Elements()` returns immediately with an empty slice, allowing `Eventually` to retry on its own cadence. No security assertion is weakened, no production code is modified, no authz/scoping logic is touched. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-04 18:55:39 +00:00

Author

Owner

Copy link

Code Review — bookshelf-eu18m (PR #1331)

Reviewed per .claude/rules/review-standard.md. All 23 changed files are in e2e/browser/ — no production code touched.

---

Methodology

Systematically verified all 53 conversions across three risk categories:

1. Guard completeness: every [0] access preceded by if err != nil || len(els) == 0 { return false/""/0 }
2. Semantic equivalence: each converted poll still asserts the same condition as the original
3. Over-conversion: bare Must* calls outside Eventually left untouched

---

Findings

Guard completeness — PASS. Every [0] access is inside a len > 0 guard. The 12 conversions that use els, _ := page.Elements(...) (error discarded) never access [0] — they only call len(els) — so no index-out-of-range risk there either.

Semantic equivalence — PASS. All conversions preserve the original assertion:

• page.Element(sel).MustText() → els[0].MustText() under guard (same text check)
• page.Has(sel) / page.MustHas(sel) → len(els) > 0 (non-blocking existence check; page.Has and page.Elements are both immediate DOM snapshots, semantically equivalent)
• !page.MustHas(sel) → len(els) == 0 (negation preserved correctly, e.g. journey_oidc_group_mapping_test.go:203)
• page.MustElements(sel) inside Eventually → page.Elements(sel) with _, _ (no [0] access, only len)

Over-conversion — PASS. Bare Must* calls outside Eventually are not converted:

• e2e/browser/journey_books_list_test.go:231: cbs := page.MustElements(...) (post-Eventually action, left as Must) — correct.
• page.MustElement(...).MustClick() action lines throughout — all correctly left alone.

Variable naming — PASS. The journey_duplicates_test.go hunk correctly renames the inner els to groups to avoid shadowing the outer els used for the dismiss button in the same closure scope.

MustHas-inside-Eventually conversions (OIDC, manage_library) — PASS. The two cases where page.MustElement or page.MustHas was called bare inside Eventually (would panic/propagate if element absent) are now safe: journey_oidc_group_mapping_test.go:121 and journey_manage_library_test.go:93 both properly return false on absence.

File scope — PASS. All 23 files are under e2e/browser/. No internal/, cmd/, templates, or static assets touched.

---

No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Code Review — bookshelf-eu18m (PR #1331) Reviewed per `.claude/rules/review-standard.md`. All 23 changed files are in `e2e/browser/` — no production code touched. --- ### Methodology Systematically verified all 53 conversions across three risk categories: 1. **Guard completeness:** every `[0]` access preceded by `if err != nil || len(els) == 0 { return false/""/0 }` 2. **Semantic equivalence:** each converted poll still asserts the same condition as the original 3. **Over-conversion:** bare `Must*` calls outside `Eventually` left untouched --- ### Findings **Guard completeness — PASS.** Every `[0]` access is inside a `len > 0` guard. The 12 conversions that use `els, _ := page.Elements(...)` (error discarded) never access `[0]` — they only call `len(els)` — so no index-out-of-range risk there either. **Semantic equivalence — PASS.** All conversions preserve the original assertion: - `page.Element(sel).MustText()` → `els[0].MustText()` under guard (same text check) - `page.Has(sel)` / `page.MustHas(sel)` → `len(els) > 0` (non-blocking existence check; `page.Has` and `page.Elements` are both immediate DOM snapshots, semantically equivalent) - `!page.MustHas(sel)` → `len(els) == 0` (negation preserved correctly, e.g. `journey_oidc_group_mapping_test.go:203`) - `page.MustElements(sel)` inside `Eventually` → `page.Elements(sel)` with `_, _` (no `[0]` access, only `len`) **Over-conversion — PASS.** Bare `Must*` calls outside `Eventually` are not converted: - `e2e/browser/journey_books_list_test.go:231`: `cbs := page.MustElements(...)` (post-`Eventually` action, left as Must) — correct. - `page.MustElement(...).MustClick()` action lines throughout — all correctly left alone. **Variable naming — PASS.** The `journey_duplicates_test.go` hunk correctly renames the inner `els` to `groups` to avoid shadowing the outer `els` used for the dismiss button in the same closure scope. **MustHas-inside-Eventually conversions (OIDC, manage_library) — PASS.** The two cases where `page.MustElement` or `page.MustHas` was called bare inside `Eventually` (would panic/propagate if element absent) are now safe: `journey_oidc_group_mapping_test.go:121` and `journey_manage_library_test.go:93` both properly return `false` on absence. **File scope — PASS.** All 23 files are under `e2e/browser/`. No `internal/`, `cmd/`, templates, or static assets touched. --- No findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor force-pushed bd-bookshelf-eu18m from fe556ad76c to 05be7d8b23 2026-08-04 19:23:52 +00:00 Compare

zombor merged commit fbe9bca01c into main 2026-08-04 19:34:14 +00:00

zombor referenced this pull request from a commit 2026-08-04 19:34:19 +00:00

Merge pull request 'fix(e2e): sweep blocking page.Element()-in-Eventually to non-blocking page.Elements() (systemic go-rod de-flake) (bookshelf-eu18m)' (#1331) from bd-bookshelf-eu18m into main

zombor referenced this pull request 2026-08-05 02:53:52 +00:00

CI test-MySQL tuning: redo capacity + buffer pool + io_capacity + table caches (bookshelf-7w1mn) #1335

zombor referenced this pull request 2026-08-05 12:30:10 +00:00

fix(e2e): harden reset_progress journey — Eval + 5s budget (bookshelf-ydjo9) #1342

zombor referenced this pull request from a commit 2026-08-06 00:20:42 +00:00

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

zombor referenced this pull request 2026-08-06 00:20:49 +00:00

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

zombor referenced this pull request from a commit 2026-08-06 00:48:49 +00:00

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

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

No description provided.