Skip to content

fix: deflake chat user journey test#7646

Merged
nmgarza5 merged 1 commit intomainfrom
nikg/pw/flaky-test-timeout-bumo
Jan 21, 2026
Merged

fix: deflake chat user journey test#7646
nmgarza5 merged 1 commit intomainfrom
nikg/pw/flaky-test-timeout-bumo

Conversation

@nmgarza5
Copy link
Copy Markdown
Contributor

@nmgarza5 nmgarza5 commented Jan 21, 2026

Description

this test constantly is flaky

targeted changes:
waitForSelector is deprecated and default waitFor is 10 seconds. so updated the action toggle command specifically to use this pattern and switching to some explicit expect.locator().toBeVisible() for some others.

How Has This Been Tested?

ci

Additional Options

  • [Optional] Override Linear Check

Summary by cubic

Deflaked the Default Assistant E2E by replacing deprecated waits with locator-based waits and explicit visibility checks. Uses Playwright’s default 10s auto-wait to reduce CI flakiness.

  • Bug Fixes
    • Replaced page.waitForSelector with locator().waitFor() in default_assistant.spec.ts and tools.ts.
    • Switched to expect(locator).toBeVisible() for the Onyx logo check.
    • Removed custom 5s timeouts to rely on Playwright defaults.

Written for commit dfa0939. Summary will update on new commits.

@nmgarza5 nmgarza5 requested a review from a team as a code owner January 21, 2026 22:15
@nmgarza5 nmgarza5 force-pushed the nikg/pw/flaky-test-timeout-bumo branch from dfa0939 to c0d2bdd Compare January 21, 2026 22:16
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Jan 21, 2026

Greptile Summary

Modernized Playwright test patterns by replacing deprecated waitForSelector with locator().waitFor() and removing explicit 5-second timeouts to allow the default 10-second timeout.

  • Replaced waitForSelector with locator().waitFor() pattern in 3 locations in the user journey test
  • Removed explicit timeout: 5000 parameters in openActionManagement utility function, allowing default 10-second timeout
  • Changes target the flaky "complete user journey with default assistant" test

Note: File still contains several instances of the old waitForSelector pattern at lines 102, 133, 166, 341, 440, 482, and 513 that should be updated for consistency.

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk
  • Changes follow Playwright best practices and should improve test stability. Scope is limited to test code only. Minor inconsistency with incomplete migration of all similar patterns in the same file.
  • No files require special attention

Important Files Changed

Filename Overview
web/tests/e2e/chat/default_assistant.spec.ts Replaced deprecated waitForSelector with modern locator().waitFor() pattern to improve test reliability
web/tests/e2e/utils/tools.ts Removed explicit 5-second timeouts, allowing default 10-second timeout to reduce flakiness

Sequence Diagram

sequenceDiagram
    participant Test as Test Runner
    participant Page as Playwright Page
    participant DOM as Browser DOM
    
    Note over Test,DOM: Old Pattern (waitForSelector)
    Test->>Page: waitForSelector('[data-testid="onyx-logo"]', timeout: 5000)
    Page->>DOM: Poll for element (5s timeout)
    alt Element found within 5s
        DOM-->>Page: Element handle
        Page-->>Test: ElementHandle
        Test->>Test: expect(element).toBeTruthy()
    else Timeout after 5s
        DOM-->>Page: Timeout error
        Page-->>Test: Error thrown
    end
    
    Note over Test,DOM: New Pattern (locator().waitFor())
    Test->>Page: locator('[data-testid="onyx-logo"]').waitFor()
    Page->>DOM: Poll for element (10s default timeout)
    alt Element found within 10s
        DOM-->>Page: Element visible
        Page-->>Test: Promise resolved
    else Timeout after 10s
        DOM-->>Page: Timeout error
        Page-->>Test: Error thrown
    end
Loading

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (2)

  1. web/tests/e2e/chat/default_assistant.spec.ts, line 102-106 (link)

    style: Inconsistent pattern - should also be updated to use locator().waitFor() like the changes in the same file

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  2. web/tests/e2e/chat/default_assistant.spec.ts, line 133-137 (link)

    style: Same pattern should be updated for consistency

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

2 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 2 files

@nmgarza5 nmgarza5 changed the title fix: deflake test chat user journey test fix: deflake chat user journey test Jan 21, 2026
@nmgarza5 nmgarza5 enabled auto-merge January 21, 2026 22:19
@nmgarza5 nmgarza5 added this pull request to the merge queue Jan 21, 2026
Merged via the queue into main with commit 8ca06ef Jan 21, 2026
80 checks passed
@nmgarza5 nmgarza5 deleted the nikg/pw/flaky-test-timeout-bumo branch January 21, 2026 22:39
nmgarza5 added a commit that referenced this pull request Jan 27, 2026
jmelahman pushed a commit that referenced this pull request Jan 27, 2026
jmelahman pushed a commit that referenced this pull request Jan 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants