test(tui): de-flake near-tail journal-jump scroll assertion#108
Conversation
The jump handler defers its scroll via a set_timer until the RichLog flushes its lines and grows tall enough to scroll. The test waited on a bare `is_vertical_scroll_end`, which is trivially true in the pre-flush empty state (scroll_y == max_scroll_y == 0), so on a fast capture it sampled anchored=0 before the deferred scroll landed at the tail. The delayed jump then moved the view to 188, tripping `assert 188 == 0`. Gate the wait on `max_scroll_y > 0 and is_vertical_scroll_end` so it samples only after the content is actually rendered at the tail - the tail-variant of the `0 < scroll_y < max_scroll_y` guard its sibling jump test already uses. Also add the flaky marker its two sibling journal-jump tests carry for the orthogonal poll-supersede stall. No product change; the anchor logic was already correct.
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe journal jump test is marked flaky and now waits for the log pane to render scrollable content before checking that it is at the vertical end. ChangesJournal jump test
Estimated code review effort: 1 (Trivial) | ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The flush's own scroll_end can satisfy the settled-at-tail wait while one deferred _scroll_log_to retry is still armed; if that fire landed after the growth-render it would re-scroll against the grown content and re-flake the anchor assert. Pause two timer periods so the chain provably drains at the tail before the anchor is sampled.
pbean
left a comment
pbean
left a comment
There was a problem hiding this comment.
Validated end-to-end — approving. Every claim in the description checks out against the code:
_scroll_log_toreally does defer via a 0.05sset_timerretry chain until the previously-hidden RichLog flushes (dashboard.py:302-317), and Textual'sis_vertical_scroll_endisscroll_offset.y == max_scroll_y or not self.size— trivially True on an unflushed/unsized pane, exactly as diagnosed. The failure arithmetic matches too: 201 content lines − 13-row viewport = 188.- Product anchor confirmed correct (
_log_follow_tail = Falseat jump; the growth-render preservesscroll_yvia clear+rewrite withscroll_end=False), so this was genuinely test synchronization, not product behavior. - Sibling parity checks out: both other journal-jump tests carry the same flaky marker, and line 560 was the only bare scroll-end wait in the suite.
I pushed one small hardening commit on-branch (3bb17c1): the new wait can be satisfied by the flush's own scroll_end while one deferred _scroll_log_to retry is still armed (≤50 ms out). If the growth-render from _tick landed before that pending fire, the late fire would re-scroll against the grown content (~194 vs the 188 anchor) — same-shaped flake, just a narrower window. Pausing two timer periods before sampling anchored provably drains the chain at the tail. Pre-existing, not something your change introduced.
One heads-up on the testing methodology: -p no:flaky disables the flaky package, but this repo uses pytest-rerunfailures, whose plugin name is rerunfailures — so your 5× run silently still had reruns armed. -p no:rerunfailures is the flag that does what you intended. I re-ran the target 10× with it genuinely disabled (plus the drain commit): 10/10 green, so your wait fix is indeed deterministic on its own. Siblings green, trunk clean.
2 participants
What
De-flake
test_journal_jump_near_tail_does_not_chase_growing_log, which intermittently failed in CI withassert 188 == 0. Test-only change.Why
The jump handler (
_jump_to_log_event→_scroll_log_to) defers its scroll via aset_timeruntil theRichLogflushes its lines and grows tall enough to scroll. The test waited on a bareis_vertical_scroll_end, which is trivially true in the pre-flush empty state (scroll_y == max_scroll_y == 0). On a fast capture it sampledanchored = 0before the deferred scroll landed at the tail; the delayed jump then moved the view to 188, trippingassert 188 == 0.The
188was the legit jump landing (old content max), never tail-chasing — the product anchor (_log_follow_tail = False) was already correct. This is a test-synchronization race, not a product bug.How
max_scroll_y > 0 and is_vertical_scroll_endsoanchoredis sampled only after content is actually rendered at the tail. This is the tail-variant of the0 < scroll_y < max_scroll_yguard the siblingtest_journal_enter_jumps_to_log_positionalready uses.@pytest.mark.flaky(reruns=2, reruns_delay=1), matching the two sibling journal-jump tests, for the orthogonal exclusive-poll-supersede stall theuntildocstring documents.Testing
Ran the target 5× with flaky reruns disabled (
-p no:flaky) → 5/5 green, so the wait fix alone is deterministic; the marker is belt-and-suspenders. Ruff check + format clean. No other test in the suite uses the bare-scroll-end wait pattern.Summary by CodeRabbit