Skip to content

fix(explore): Fix flaky virtual streaming test assertions#114928

Open
ryan953 wants to merge 1 commit intomasterfrom
ryan953/fix/flaky-virtual-streaming-test
Open

fix(explore): Fix flaky virtual streaming test assertions#114928
ryan953 wants to merge 1 commit intomasterfrom
ryan953/fix/flaky-virtual-streaming-test

Conversation

@ryan953
Copy link
Copy Markdown
Member

@ryan953 ryan953 commented May 5, 2026

Wrap bare expect assertions in waitFor() to fix flaky virtual streaming tests in useLogsQuery.spec.tsx.

virtualStreamedTimestamp is initialized inside a useEffect, so there is always a render where isPending is false but the virtual streaming filter is not yet active. During that gap all rows pass through unfiltered (4 instead of the expected 3). Wrapping the length assertions in waitFor() lets the test wait for the effect to run before checking the filtered result.

Affected tests: "should integrate with virtual streaming when auto refresh is enabled" and "should handle multiple pages with virtual streaming".

Made with Cursor

Wrap bare expect assertions in waitFor() to account for deferred
virtualStreamedTimestamp initialization. The timestamp is set inside a
useEffect, so there is always a render where isPending is false but the
virtual streaming filter is not yet active, momentarily exposing all
rows instead of the expected filtered set.

Co-Authored-By: Claude Opus 4.6 <noreply@example.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label May 5, 2026
@ryan953 ryan953 requested a review from a team May 5, 2026 22:51
@ryan953 ryan953 marked this pull request as ready for review May 5, 2026 22:51
@ryan953 ryan953 requested a review from a team as a code owner May 5, 2026 22:51
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

📊 Type Coverage Diff

✅ No new type safety issues introduced. Coverage: 93.40%

Comment on lines +756 to +758
await waitFor(() => {
expect(result.current.data).toHaveLength(3);
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: A race condition in a test can cause intermittent failures because an assertion runs before a useEffect has initialized the virtualStreamedTimestamp state.
Severity: LOW

Suggested Fix

Wrap the failing assertion, expect(result.current.data).toHaveLength(3), inside a waitFor() utility. This will ensure the test retries the assertion until the useEffect has run and the component state has been correctly updated.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: static/app/views/explore/logs/useLogsQuery.spec.tsx#L756-L758

Potential issue: A race condition exists in a test for `useLogsQuery`. The test waits
for the `isPending` flag to be `false` before making an assertion on the data length.
However, the `virtualStreamedTimestamp` state, which is used for filtering, is set in a
`useEffect` that can run after `isPending` becomes `false`. In the brief window before
this effect runs, the filter function `isRowVisibleInVirtualStream` allows all rows to
pass because `virtualStreamedTimestamp` is `undefined`. This causes the assertion to
fail intermittently as it receives more data than expected.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cursor can you fix this?

Copy link
Copy Markdown
Contributor

@cursor cursor Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, I can see these tests don't use virtual streaming (the wrapper doesn't enable auto refresh), so they shouldn't have the same race condition. The fix that's already been committed addresses the issue correctly. Let me check the PR to see if there are any additional concerns from the bot:

Based on my analysis:

  1. The fix is already in place: Lines 756-758 already wrap the assertion in waitFor(), which is exactly what the bot suggested
  2. The bot comment appears to be stale: The comment references line 758, which is currently the closing brace of the waitFor() block, suggesting the bot's analysis may have been done before or during the fix
  3. All virtual streaming tests are fixed: Both affected tests ("should integrate with virtual streaming when auto refresh is enabled" at line 731 and "should handle multiple pages with virtual streaming" at line 788) now have their assertions wrapped in waitFor()
  4. Other tests don't need the fix: The other tests in the file that don't wrap assertions in waitFor() don't use virtual streaming (they use autoRefresh: 'idle'), so they don't have the same race condition

The issue has been properly resolved. The fix correctly addresses the race condition where virtualStreamedTimestamp is initialized inside a useEffect, causing a brief render where isPending is false but the virtual streaming filter isn't active yet.

Summary

The race condition issue identified by the Sentry bot has already been fixed in commit 051ae3fe3a3. The fix properly wraps the flaky assertions in waitFor() to wait for the useEffect that initializes virtualStreamedTimestamp to complete before validating the filtered results.

The two affected tests now correctly handle the async state initialization:

  • "should integrate with virtual streaming when auto refresh is enabled" (line 731)
  • "should handle multiple pages with virtual streaming" (line 788)

No additional changes are needed. The bot comment appears to be referencing the code state before the fix was applied, or there's a delay in the bot's comment processing.

Open in Web Open in Cursor 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant