fix(e2e): widen move_toast BeforeAll Eventually timeouts (residual flake) (bookshelf-49zk3) #1330

Merged

zombor merged 2 commits from bd-bookshelf-49zk3 into main 2026-08-04 17:12:36 +00:00

Conversation 2 Commits 2 Files changed 1 +54 -46

zombor commented 2026-08-04 12:39:16 +00:00

Owner

Copy link

Summary

The journey_move_toast BeforeAll drives a real async bulk-move workflow through SSE/poll to toast resolution. Per-step Eventually timeouts of 10–15s were occasionally too short under CI CPU contention, causing the test to time out at steps like "click organize toolbar button" and "Move Files button should appear" (#1324, #1328).

Root cause: per-step budgets were sized for an uncontended runner. Under CPU starvation, CDP round-trips and Stimulus controller DOM updates take materially longer than the generous median.

Fix: widen every per-step Eventually timeout in the BeforeAll:

• navigate: 30s → 60s
• checkbox appear: 30s → 60s
• click checkbox: 15s → 45s
• organize toolbar appear: 10s → 45s
• click organize button: 15s → 45s
• Move Files button appear: 15s → 45s
• Move Files button after dropdown: 10s → 45s
• click confirm: 15s → 45s

Also bump the BeforeAll page deadline from 3min → 5min to accommodate the new summed worst-case (~330s). All assertions and polling intervals are unchanged — only the wait windows are wider. The refreshPageTimeout(page) call before each step (added by b8p9v) remains in place.

Test plan

• go build -tags e2e ./e2e/browser/... compiles clean
• CI E2E Browser job should pass consistently with the widened budgets

Closes bead bookshelf-49zk3 on merge.

## Summary The `journey_move_toast` BeforeAll drives a real async bulk-move workflow through SSE/poll to toast resolution. Per-step `Eventually` timeouts of 10–15s were occasionally too short under CI CPU contention, causing the test to time out at steps like "click organize toolbar button" and "Move Files button should appear" (#1324, #1328). **Root cause:** per-step budgets were sized for an uncontended runner. Under CPU starvation, CDP round-trips and Stimulus controller DOM updates take materially longer than the generous median. **Fix:** widen every per-step `Eventually` timeout in the BeforeAll: - navigate: 30s → 60s - checkbox appear: 30s → 60s - click checkbox: 15s → 45s - organize toolbar appear: 10s → 45s - click organize button: 15s → 45s - Move Files button appear: 15s → 45s - Move Files button after dropdown: 10s → 45s - click confirm: 15s → 45s Also bump the BeforeAll page deadline from 3min → 5min to accommodate the new summed worst-case (~330s). All assertions and polling intervals are unchanged — only the wait windows are wider. The `refreshPageTimeout(page)` call before each step (added by b8p9v) remains in place. ## Test plan - `go build -tags e2e ./e2e/browser/...` compiles clean - CI E2E Browser job should pass consistently with the widened budgets Closes bead bookshelf-49zk3 on merge.

zombor added 1 commit 2026-08-04 12:39:16 +00:00

fix(e2e): widen move_toast BeforeAll Eventually timeouts to eliminate residual flake (bookshelf-49zk3) ... 5e6bed7335

The journey_move_toast BeforeAll drives a real async bulk-move workflow
through SSE/poll to toast. Per-step Eventually timeouts of 10–15s were
occasionally too short under CI CPU contention (#1324/#1328).

Widen the per-step budgets: navigate 30s→60s, checkbox appear 30s→60s,
all click/appear steps 10-15s→45s. Bump the BeforeAll page deadline from
3min→5min to accommodate the new summed worst-case (~330s). Keep all
assertions and polling intervals unchanged — only the wait windows are wider.

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

zombor added 1 commit 2026-08-04 12:50:00 +00:00

fix(e2e): use Elements (non-blocking) in move_toast Eventually loops to fix root cause ... 2d2e20074b

The prior commit widened Eventually timeouts but missed the root cause:
page.Element() (singular) does internal go-rod polling for up to pageTimeout
(60s) before returning — so the first attempt of any Eventually loop blocks
for 60s, blowing past the 45s budget and causing Gomega to report
"Timed out after 60.000s".

Fix: switch all Element() calls inside Eventually loops to Elements()
(plural, non-blocking) which returns immediately with whatever elements are
in the DOM. Combined with the generous timeout budgets from the prior commit,
the loops now truly retry within their 45s/60s windows.

Also bump the error-toast It from 30s to 60s to give the async
workflow+SSE+poll cycle more headroom.

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

zombor commented 2026-08-04 12:57:22 +00:00

Author

Owner

Copy link

Security Review — PR #1330 (bookshelf-49zk3)

Scope: e2e/browser/journey_move_toast_test.go — browser e2e flake fix; swaps blocking page.Element() calls for non-blocking page.Elements() inside Eventually loops and widens per-step timeouts.

Build gate: //go:build e2e — this file is compiled only when -tags e2e is supplied. It is entirely absent from the production binary and from unit/integration test runs. No production code is touched.

Package: package browser_test — correct black-box test declaration.

---

Security surface analysis

Area	Finding
Multi-user scoping	No per-user data access; test drives a browser journey against its own seeded fixtures. N/A.
AuthZ / RBAC	Auth cookies set via setAuthCookies (existing helper, not modified). No gate removed or weakened.
Injection / SSRF	No URL construction from test input; booksURL is built from a server-controlled suiteEnv.BaseURL and a seeded srcLibID. No new outbound fetch.
Input bounds	No fan-out, no unbounded query, no resource allocation change.
Secrets / PII	No token, key, or PII introduced or logged.
CSP	No template or style= change.
Test weakening	No assertion removed. Selectors identical; Elements()[0] is functionally equivalent to Element() for presence checks. Timeouts are widened (more retries), not shortened — this cannot mask a real failure.
Architecture boundary	Test-only file; no import of workflow engine or domain package from production.

No security finding of any severity. The change is a pure wait-mechanic fix: replacing a blocking single-element fetch with a non-blocking multi-element fetch so the Eventually retry loop can actually iterate within its budget, plus widening budgets to match observed worst-case CI latency. Nothing in the security checklist applies.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #1330 (bookshelf-49zk3) **Scope:** `e2e/browser/journey_move_toast_test.go` — browser e2e flake fix; swaps blocking `page.Element()` calls for non-blocking `page.Elements()` inside `Eventually` loops and widens per-step timeouts. **Build gate:** `//go:build e2e` — this file is compiled only when `-tags e2e` is supplied. It is entirely absent from the production binary and from unit/integration test runs. No production code is touched. **Package:** `package browser_test` — correct black-box test declaration. --- ### Security surface analysis | Area | Finding | |---|---| | Multi-user scoping | No per-user data access; test drives a browser journey against its own seeded fixtures. N/A. | | AuthZ / RBAC | Auth cookies set via `setAuthCookies` (existing helper, not modified). No gate removed or weakened. | | Injection / SSRF | No URL construction from test input; `booksURL` is built from a server-controlled `suiteEnv.BaseURL` and a seeded `srcLibID`. No new outbound fetch. | | Input bounds | No fan-out, no unbounded query, no resource allocation change. | | Secrets / PII | No token, key, or PII introduced or logged. | | CSP | No template or `style=` change. | | Test weakening | No assertion removed. Selectors identical; `Elements()[0]` is functionally equivalent to `Element()` for presence checks. Timeouts are widened (more retries), not shortened — this cannot mask a real failure. | | Architecture boundary | Test-only file; no import of workflow engine or domain package from production. | No security finding of any severity. The change is a pure wait-mechanic fix: replacing a blocking single-element fetch with a non-blocking multi-element fetch so the `Eventually` retry loop can actually iterate within its budget, plus widening budgets to match observed worst-case CI latency. Nothing in the security checklist applies. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-04 12:58:28 +00:00

Author

Owner

Copy link

Code Review: bookshelf-49zk3 / PR #1330

Scope: e2e/browser/journey_move_toast_test.go only — test-only flake fix.

---

Phase 1: Spec Compliance

The bead targets the root cause of the journey_move_toast browser flake: page.Element() (singular) internally blocks up to pageTimeout (60s) when an element is absent, making the surrounding Eventually loop meaningless — the first attempt burns the entire window before Gomega can retry. The fix replaces every page.Element() call inside Eventually with page.Elements() (plural, non-blocking), and widens budgets to give retries real room.

All requirements verified present:

• Every page.Element() inside an Eventually block has been replaced with page.Elements() — no page.Element() remains in the file.
• Empty-slice guard (len(els) > 0 / len(els) == 0) is present on every converted call.
• [0] indexing only occurs after the length check — no index-out-of-range path exists.
• Behavioral assertions (toolbar visible, move-files button not hidden, info toast, error toast) are all preserved unchanged.
• BeforeAll page deadline widened 3min → 5min; individual step budgets widened 10–30s → 45–60s.
• e2e policy: Ordered journey container, package browser_test (black-box), no per-spec ResetDB.

---

Phase 2: Code Quality

[MINOR] e2e/browser/journey_move_toast_test.go:109 — BeforeAll comment overstates worst-case sum by 90s

The comment claims the "sequential Eventually budgets sum to ~330s worst-case" but the actual sum is 60+60+45+45+45+45+45+45 = 390s. The 5-minute outer deadline (300s) is not violated in practice because refreshPageTimeout issues CancelTimeout().Timeout(60s) before each step, so each step runs within a fresh 60s window and the outer deadline only governs the pre-first-refresh setup block (setAuthCookies + incognito creation). The comment's arithmetic is simply wrong and will confuse the next reader. Suggested fix: update the comment to reflect that the 5min deadline guards the pre-refresh preamble, and that each step resets to a fresh 60s window, so the sequential sum does not compound against the outer deadline.

[MINOR] e2e/browser/journey_move_toast_test.go:243 — error-toast It has zero margin between page timeout and Eventually budget

BeforeEach calls refreshPageTimeout (issues a fresh 60s page.Timeout). The error-toast It then runs Eventually(..., 60*time.Second, 500ms). page.Elements() is non-blocking so the page context is not blocked per-call, but if any single call hits a slow CDP round-trip, the 60s page context and the 60s Gomega window expire simultaneously — the page context can cancel the CDP call at the same instant Gomega declares timeout, producing a context-deadline error whose message will be confusing. A 5s margin (e.g. 55s Eventually or 70s page context via a local refreshPageTimeout at the top of that It) eliminates the edge case. Not a correctness bug on healthy runners; a rare-race under extreme starvation.

---

No blockers. No majors.

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## Code Review: bookshelf-49zk3 / PR #1330 **Scope:** `e2e/browser/journey_move_toast_test.go` only — test-only flake fix. --- ### Phase 1: Spec Compliance The bead targets the root cause of the `journey_move_toast` browser flake: `page.Element()` (singular) internally blocks up to `pageTimeout` (60s) when an element is absent, making the surrounding `Eventually` loop meaningless — the first attempt burns the entire window before Gomega can retry. The fix replaces every `page.Element()` call inside `Eventually` with `page.Elements()` (plural, non-blocking), and widens budgets to give retries real room. All requirements verified present: - Every `page.Element()` inside an `Eventually` block has been replaced with `page.Elements()` — no `page.Element()` remains in the file. - Empty-slice guard (`len(els) > 0` / `len(els) == 0`) is present on every converted call. - `[0]` indexing only occurs after the length check — no index-out-of-range path exists. - Behavioral assertions (toolbar visible, move-files button not hidden, info toast, error toast) are all preserved unchanged. - BeforeAll page deadline widened 3min → 5min; individual step budgets widened 10–30s → 45–60s. - e2e policy: `Ordered` journey container, `package browser_test` (black-box), no per-spec `ResetDB`. --- ### Phase 2: Code Quality **[MINOR] e2e/browser/journey_move_toast_test.go:109 — BeforeAll comment overstates worst-case sum by 90s** The comment claims the "sequential Eventually budgets sum to ~330s worst-case" but the actual sum is 60+60+45+45+45+45+45+45 = **390s**. The 5-minute outer deadline (300s) is not violated in practice because `refreshPageTimeout` issues `CancelTimeout().Timeout(60s)` before each step, so each step runs within a fresh 60s window and the outer deadline only governs the pre-first-refresh setup block (setAuthCookies + incognito creation). The comment's arithmetic is simply wrong and will confuse the next reader. Suggested fix: update the comment to reflect that the 5min deadline guards the pre-refresh preamble, and that each step resets to a fresh 60s window, so the sequential sum does not compound against the outer deadline. **[MINOR] e2e/browser/journey_move_toast_test.go:243 — error-toast It has zero margin between page timeout and Eventually budget** `BeforeEach` calls `refreshPageTimeout` (issues a fresh 60s `page.Timeout`). The error-toast `It` then runs `Eventually(..., 60*time.Second, 500ms)`. `page.Elements()` is non-blocking so the page context is not blocked per-call, but if any single call hits a slow CDP round-trip, the 60s page context and the 60s Gomega window expire simultaneously — the page context can cancel the CDP call at the same instant Gomega declares timeout, producing a context-deadline error whose message will be confusing. A 5s margin (e.g. 55s Eventually or 70s page context via a local `refreshPageTimeout` at the top of that `It`) eliminates the edge case. Not a correctness bug on healthy runners; a rare-race under extreme starvation. --- **No blockers. No majors.** REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor force-pushed bd-bookshelf-49zk3 from 2d2e20074b to f470a79ff5 2026-08-04 12:59:48 +00:00 Compare

zombor merged commit 256ad4a7d2 into main 2026-08-04 17:12:36 +00:00

zombor referenced this pull request from a commit 2026-08-04 17:12:39 +00:00

Merge pull request 'fix(e2e): widen move_toast BeforeAll Eventually timeouts (residual flake) (bookshelf-49zk3)' (#1330) from bd-bookshelf-49zk3 into main

zombor referenced this pull request 2026-08-04 19:39:19 +00:00

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

zombor referenced this pull request from a commit 2026-08-05 02:43:25 +00:00

fix(e2e): migrate move_toast from browser to e2e/api tier (bookshelf-xif3r)

zombor referenced this pull request 2026-08-05 02:43:38 +00:00

fix(e2e): migrate move_toast from browser to API tier (bookshelf-xif3r) #1339

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

No description provided.