Fix flaky UnreadIndicators mark-as-unread test#92125
Draft
neil-marcellini wants to merge 1 commit into
Draft
Conversation
… render Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
(Neil's AI agent)
Fixes a flaky CI test:
Unread Indicators › Mark the chat as unread on clicking "Mark as unread" on an item in LHN when the last message of the chat was deleted by another userintests/ui/UnreadIndicatorsTest.tsx.Failure observed on CI
Root cause
The assertion read the rendered LHN synchronously, immediately after a single
await waitForBatchedUpdates():The preceding steps fire several async
Onyx.mergecalls plusmarkCommentAsUnread, which triggers an LHN recompute/re-sort. A singlewaitForBatchedUpdates()does not guarantee the chat row has re-mounted, so the synchronous query can run before the row renders and find0. Other UI assertions in this same file already wrap reads inwaitFor(...)(e.g. lines 285, 453, 487) to avoid this exact race.Fix
Wrap the assertion in
waitFor(...)so it retries until the LHN row renders. This is strictly more tolerant of render timing and never less correct.Fixed Issues
$ #92122
PROPOSAL: N/A (flaky test fix)
For #92118
Tests
(Neil's AI agent)
Evidence of flakiness (before the fix) — the same test, at the same line, failed on two unrelated
mainmerges within ~3 hours, neither of which touches this test or the LHN:The failure is a CI-only timing flake and does not reproduce in local single-file loops (expected for timing flakes). To prove the failure mechanism the fix targets, a throwaway test confirmed that a synchronous
queryAllByLabelTextmisses an element that renders one tick later (reproducingReceived length: 0), while thewaitForversion passes:Proof of stability (after the fix) — ran the full
UnreadIndicatorsTest.tsxfile 25× under heavy CPU load (8 stressors) to simulate CI contention:(Before the fix, the same loop was also green locally — 25/25 — confirming the flake is CI-only, not a local hard failure.)
Reviewer can reproduce:
npx jest tests/ui/UnreadIndicatorsTest.tsx --runInBandOffline tests
N/A — test-only change.
QA Steps
N/A — test-only change. [No QA]
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionScreenshots/Videos
N/A — test-only change.
Made with Cursor