Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ryan953 wants to merge 1 commit into
base: master
Choose a base branch
from
+6 −2
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions static/app/views/explore/logs/useLogsQuery.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,7 +753,9 @@ describe('Virtual Streaming Integration (Auto Refresh Behaviour)', () => {

// With auto refresh enabled, virtual streaming should be active
// All data should be visible initially since all the mocked timestamps are well behind `now()`
expect(result.current.data).toHaveLength(3);
await waitFor(() => {
expect(result.current.data).toHaveLength(3);
});
Comment on lines +756 to +758
Copy link
Copy Markdown
Contributor

@sentry sentry Bot May 5, 2026

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

@ryan953 ryan953 May 5, 2026

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 •
edited
Loading

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 

});

it('should not apply virtual streaming when auto refresh is disabled', async () => {
Expand Down Expand Up @@ -836,7 +838,9 @@ describe('Virtual Streaming Integration (Auto Refresh Behaviour)', () => {
expect(result.current.isPending).toBe(false);
});

expect(result.current.data).toHaveLength(3);
await waitFor(() => {
expect(result.current.data).toHaveLength(3);
});
expect(initialMock).toHaveBeenCalled();

// Fetch next page
Expand Down
Loading