fix(e2e): eliminate publishers-tab render race via BeforeAll + Eventually (bookshelf-5kje) #903

Merged

zombor merged 1 commit from bd-bookshelf-5kje into main 2026-07-03 17:23:02 +00:00

Conversation 3 Commits 1 Files changed 1 +18 -9

zombor commented 2026-07-03 15:49:34 +00:00

Owner

Copy link

Summary

• Root cause found: The Describe("after clicking the Publishers tab", Ordered, ...) used JustBeforeEach which runs before each It. With 2 Its, the full navigate→waitStable→waitController→click sequence ran TWICE. On the second re-navigation, a brief paint window exists between waitForStimulusController('sub-tab') returning and the publishers button being render-stable in Chromium under CI load. MustElement polled for 60s and panicked with "context deadline exceeded".
• Why #892 was insufficient: bookshelf-rpr4 / #892 fixed the shared-deadline exhaustion (multiple blocking ops consuming one 60s budget). It did NOT eliminate the duplicate-navigation race — the per-It re-navigate still re-exposed the render gap.
• Fix applied:
  1. JustBeforeEach → BeforeAll: navigate once and click once for both Its. The tab-click state persists across the Ordered Its — no repeated re-render.
  2. MustElement(...).MustClick() → Eventually wait for button + MustElement(...).MustClick(): tolerates any residual render delay deterministically without panicking.
  3. Added page = refreshPageTimeout(page) at the start of each inner It (go-rod Ordered gotcha: absolute deadline must be reset per step when reusing a page).

Test plan

• CI browser suite passes green (the flake reproduced under load; BeforeAll eliminates the duplicate-navigation race)
• e2e-policy-check passes (all top-level Describes remain Ordered — verified locally)
• go build -tags e2e ./e2e/browser/... compiles cleanly
• Assertions are preserved: Publishers panel visible + Authors panel hidden after clicking the tab

Closes bead bookshelf-5kje on merge.

## Summary - **Root cause found:** The `Describe("after clicking the Publishers tab", Ordered, ...)` used `JustBeforeEach` which runs before *each* `It`. With 2 Its, the full navigate→waitStable→waitController→click sequence ran TWICE. On the second re-navigation, a brief paint window exists between `waitForStimulusController('sub-tab')` returning and the publishers button being render-stable in Chromium under CI load. `MustElement` polled for 60s and panicked with "context deadline exceeded". - **Why #892 was insufficient:** bookshelf-rpr4 / #892 fixed the shared-deadline exhaustion (multiple blocking ops consuming one 60s budget). It did NOT eliminate the duplicate-navigation race — the per-It re-navigate still re-exposed the render gap. - **Fix applied:** 1. `JustBeforeEach` → `BeforeAll`: navigate once and click once for both Its. The tab-click state persists across the Ordered Its — no repeated re-render. 2. `MustElement(...).MustClick()` → `Eventually` wait for button + `MustElement(...).MustClick()`: tolerates any residual render delay deterministically without panicking. 3. Added `page = refreshPageTimeout(page)` at the start of each inner `It` (go-rod Ordered gotcha: absolute deadline must be reset per step when reusing a page). ## Test plan - [ ] CI browser suite passes green (the flake reproduced under load; BeforeAll eliminates the duplicate-navigation race) - [ ] `e2e-policy-check` passes (all top-level Describes remain Ordered — verified locally) - [ ] `go build -tags e2e ./e2e/browser/...` compiles cleanly - [ ] Assertions are preserved: Publishers panel visible + Authors panel hidden after clicking the tab Closes bead bookshelf-5kje on merge.

wf-detail-older-execution.png

55 KiB

recompute-match-score-kebab-open.png

109 KiB

zombor added 1 commit 2026-07-03 15:49:35 +00:00

fix(e2e): eliminate publishers-tab render race via BeforeAll + Eventually wait (bookshelf-5kje) ... 79a1900142

JustBeforeEach ran the full navigate→waitStable→waitController→click sequence
before EACH It in the nested Ordered Describe, meaning it ran twice. On the
second re-navigation a brief window exists between waitForStimulusController
returning and the publishers tab button being paint-stable under CI load —
MustElement polled for 60s and panicked with "context deadline exceeded".
#892 / bookshelf-rpr4 fixed the shared-deadline problem but not this race.

Fix: replace JustBeforeEach with BeforeAll so navigate+click runs ONCE for
both Its. Add an Eventually-based wait before MustClick so any residual render
delay is tolerated deterministically without a hard panic. Add refreshPageTimeout
at the start of each It (go-rod gotcha: Ordered journeys reuse page, deadline
must be reset per step).

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

forgejo-actions commented 2026-07-03 15:52:42 +00:00

Copy link

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) 

forgejo-actions commented 2026-07-03 15:52:51 +00:00

Copy link

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. 

zombor commented 2026-07-03 15:57:42 +00:00

Author

Owner

Copy link

Code Review — bookshelf-5kje (PR #903)

Diff reviewed: origin/main...origin/bd-bookshelf-5kje (head 79a190014265)
File: e2e/browser/journey_library_stats_test.go

---

Phase 0 — DEMO

This is a test-only flake-fix PR. No runnable DEMO block applies (the "demo" is CI green, confirmed). Proceeding to content review.

---

Findings

Semantic correctness of JustBeforeEach → BeforeAll

The old JustBeforeEach ran navigate+click before each of the two inner It steps — twice total. Both It assertions (Publishers panel is visible and Authors panel is now hidden) were asserting the same post-click state both times, so the second navigate+click was purely redundant and introduced the re-navigation timing window this PR fixes. The new BeforeAll runs the sequence once; in Ginkgo Ordered, state persists across It steps, so both Its observe the correct shared post-click state. No assertion was testing a pre-click/default state that BeforeAll would now break. Coverage and test meaning are fully preserved.

Both assertions remain distinct and meaningful

Publishers panel is visible and Authors panel is now hidden are two independent behavioral observations of the same SubTabController tab-switch: one panel becomes shown, the other becomes hidden. Both remain real, non-vacuous assertions against the shared post-click state. Neither is made redundant by the structural change.

Root cause addressed, not just timeout widened

The race was: second navigate in the old JustBeforeEach hit a brief window between waitForStimulusController returning and the publishers button being paint-stable, causing MustElement to panic. Moving to BeforeAll eliminates the second navigate+click entirely — a root fix. The added Eventually before MustClick is belt-and-suspenders for render-readiness on even the single run and correctly waits for the exact element the subsequent click targets ([data-sub-tab-panel-param='publishers']).

go-rod gotcha: per-It refreshPageTimeout placement

CLAUDE.md requires Ordered journeys reusing a page to reset page.Timeout at the start of each It. The fix adds page = refreshPageTimeout(page) as the first line of both inner It blocks (:225 and :242). Correct and consistent with the pattern across the entire browser suite.

Eventually timeout budget

30s/100ms for the button-existence poll is consistent with existing usage across the browser suite. The refreshPageTimeout immediately before grants a fresh 60s page-level deadline, so the poll can consume up to 30s and the subsequent MustClick still has ≥30s of margin. Correct.

No sibling tab-interaction specs with the same unaddressed race

No other nested Describe in this file uses a per-It JustBeforeEach navigate+click pattern. The outer It steps poll already-rendered page state without re-navigating. No residual instances of the same bug pattern.

E2E journey policy

Top-level Describe is Ordered — complies with the anti-regrowth guard. Package is browser_test. Build tag //go:build e2e present. Justification comment at top of file explains why real Chromium is required. All conformant.

---

[MINOR] e2e/browser/journey_library_stats_test.go:215-219 — TOCTOU double-lookup between Eventually and MustClick
Eventually polls until the element exists, then MustElement re-queries the DOM to get a handle for MustClick. In theory the element could briefly disappear between the two calls; in practice this button is server-rendered and static, so the window is essentially zero. Noting for awareness only; no correctness risk in this context.

---

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Code Review — bookshelf-5kje (PR #903) **Diff reviewed:** `origin/main...origin/bd-bookshelf-5kje` (head `79a190014265`) **File:** `e2e/browser/journey_library_stats_test.go` --- ### Phase 0 — DEMO This is a test-only flake-fix PR. No runnable DEMO block applies (the "demo" is CI green, confirmed). Proceeding to content review. --- ### Findings **Semantic correctness of JustBeforeEach → BeforeAll** The old `JustBeforeEach` ran navigate+click before each of the two inner `It` steps — twice total. Both `It` assertions (`Publishers panel is visible` and `Authors panel is now hidden`) were asserting the same post-click state both times, so the second navigate+click was purely redundant and introduced the re-navigation timing window this PR fixes. The new `BeforeAll` runs the sequence once; in Ginkgo `Ordered`, state persists across `It` steps, so both Its observe the correct shared post-click state. No assertion was testing a pre-click/default state that `BeforeAll` would now break. Coverage and test meaning are fully preserved. **Both assertions remain distinct and meaningful** `Publishers panel is visible` and `Authors panel is now hidden` are two independent behavioral observations of the same SubTabController tab-switch: one panel becomes shown, the other becomes hidden. Both remain real, non-vacuous assertions against the shared post-click state. Neither is made redundant by the structural change. **Root cause addressed, not just timeout widened** The race was: second navigate in the old `JustBeforeEach` hit a brief window between `waitForStimulusController` returning and the publishers button being paint-stable, causing `MustElement` to panic. Moving to `BeforeAll` eliminates the second navigate+click entirely — a root fix. The added `Eventually` before `MustClick` is belt-and-suspenders for render-readiness on even the single run and correctly waits for the exact element the subsequent click targets (`[data-sub-tab-panel-param='publishers']`). **go-rod gotcha: per-It `refreshPageTimeout` placement** CLAUDE.md requires `Ordered` journeys reusing a `page` to reset `page.Timeout` at the start of each `It`. The fix adds `page = refreshPageTimeout(page)` as the first line of both inner `It` blocks (`:225` and `:242`). Correct and consistent with the pattern across the entire browser suite. **`Eventually` timeout budget** 30s/100ms for the button-existence poll is consistent with existing usage across the browser suite. The `refreshPageTimeout` immediately before grants a fresh 60s page-level deadline, so the poll can consume up to 30s and the subsequent `MustClick` still has ≥30s of margin. Correct. **No sibling tab-interaction specs with the same unaddressed race** No other nested `Describe` in this file uses a per-It JustBeforeEach navigate+click pattern. The outer `It` steps poll already-rendered page state without re-navigating. No residual instances of the same bug pattern. **E2E journey policy** Top-level `Describe` is `Ordered` — complies with the anti-regrowth guard. Package is `browser_test`. Build tag `//go:build e2e` present. Justification comment at top of file explains why real Chromium is required. All conformant. --- [MINOR] e2e/browser/journey_library_stats_test.go:215-219 — TOCTOU double-lookup between Eventually and MustClick `Eventually` polls until the element exists, then `MustElement` re-queries the DOM to get a handle for `MustClick`. In theory the element could briefly disappear between the two calls; in practice this button is server-rendered and static, so the window is essentially zero. Noting for awareness only; no correctness risk in this context. --- REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor merged commit 85daceb8e2 into main 2026-07-03 17:23:02 +00:00

zombor referenced this pull request from a commit 2026-07-03 17:23:04 +00:00

Merge pull request 'fix(e2e): eliminate publishers-tab render race via BeforeAll + Eventually (bookshelf-5kje)' (#903) from bd-bookshelf-5kje 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

2 participants

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

No description provided.