Skip to content

test: update TreeView tests to address flakiness#7818

Open
joshblack wants to merge 9 commits intomainfrom
test/update-treeview-tests
Open

test: update TreeView tests to address flakiness#7818
joshblack wants to merge 9 commits intomainfrom
test/update-treeview-tests

Conversation

@joshblack
Copy link
Copy Markdown
Member

Update our TreeView tests to avoid flakiness issues we're seeing in CI

Changelog

New

Changed

  • Update tests in TreeView
    • Remove renderWithTheme and use render directly
    • Use screen.* instead of helpers returned by render
    • Wrap transitions in act()
    • Update coordination in timers so that we no longer see act() warnings

Removed

Rollout strategy

  • None; if selected, include a brief description as to why

This is a change to our internal test setup

@joshblack joshblack requested review from Copilot May 7, 2026 19:20
@joshblack joshblack requested a review from a team as a code owner May 7, 2026 19:20
@joshblack joshblack requested a review from jonrohan May 7, 2026 19:20
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 7, 2026

⚠️ No Changeset found

Latest commit: 647d12c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@joshblack joshblack added skip changeset This change does not need a changelog integration-tests: skipped manually Changes in this PR do not require an integration test labels May 7, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates TreeView browser tests to reduce CI flakiness and eliminate act() warnings by modernizing Testing Library usage and improving async coordination.

Changes:

  • Refactors tests to use render(...) directly and screen.* queries consistently.
  • Wraps more user interactions and focus/keyboard transitions in act() and replaces timer-driven assertions with waitFor(...) where appropriate.
  • Updates package-lock.json to reflect the current workspace package versions for @primer/react and @primer/styled-react.
Show a summary per file
File Description
packages/react/src/TreeView/TreeView.test.tsx Refactors TreeView tests to reduce flakiness (screen-based queries, better act() usage, and improved async handling).
package-lock.json Updates lockfile entries to match the current workspace package versions.

Copilot's findings

Comments suppressed due to low confidence (2)

packages/react/src/TreeView/TreeView.test.tsx:883

  • In this Home-key test, parent3 is queried with name "Parent 2" and the inline comments say ArrowDown is pressed twice to reach “parent 3”, but the test only presses ArrowDown once. This is confusing and makes the intent unclear—either query/rename to Parent 2 and update comments, or actually move focus to Parent 3 (and press ArrowDown twice) to match the description.
      const parent1 = screen.getByRole('treeitem', {name: 'Parent 1'})
      const parent3 = screen.getByRole('treeitem', {name: 'Parent 2'})

      act(() => {
        // Focus first item
        parent1.focus()
      })

      // Press ↓ 2 times to move focus to parent 3
      fireEvent.keyDown(document.activeElement || document.body, {key: 'ArrowDown'})

      // Parent 3 should be focused
      expect(parent3).toHaveFocus()

packages/react/src/TreeView/TreeView.test.tsx:1577

  • The error dialog content string "Opps" looks like a typo; consider correcting it to "Oops" so the test reads clearly.
          Parent
          <TreeView.SubTree>
            <TreeView.ErrorDialog>Opps</TreeView.ErrorDialog>
            <TreeView.Item id="child">Child</TreeView.Item>
  • Files reviewed: 1/2 changed files
  • Comments generated: 1

Comment on lines 1652 to 1655
await waitFor(() => {
expect(treeitem).toHaveAttribute('aria-expanded', 'true')
expect(screen.getByLabelText('No items found')).toBeInTheDocument()
})
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.

Updated in 765fece: this test now uses fake timers and advances time deterministically by 1000ms before assertions, removing the waitFor race against a real timeout.

Copy link
Copy Markdown
Contributor

Copilot AI commented May 7, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • example.com
    • Triggering command: /home/REDACTED/.cache/ms-playwright/chromium_headless_shell-1194/chrome-linux/headless_shell /home/REDACTED/.cache/ms-playwright/chromium_headless_shell-1194/chrome-linux/headless_shell --disable-field-trial-config --disable-REDACTED-networking --disable-REDACTED-timer-throttling --disable-REDACTEDing-occluded-windows --disable-back-forward-cache --disable-breakpad --disable-client-side-phishing-detection --disable-component-extensions-with-REDACTED-pages --disable-component-update --no-default-browser-check --disable-default-apps --disable-dev-shm-usage --disable-extensions --disable-features=AcceptCHFrame,AvoidUnnecessaryBeforeUnloadCheckSync,DestroyProfileOnBrowserClose,DialMediaRouteProvider,GlobalMediaControls,HttpsUpgrades,LensOverlay,MediaRouter,PaintHolding,ThirdPartyStoragePartitioning,Translate,AutoDeElevate,RenderD (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@github-actions github-actions Bot requested a deployment to storybook-preview-7818 May 7, 2026 22:02 Abandoned
@primer
Copy link
Copy Markdown
Contributor

primer Bot commented May 7, 2026

🤖 Lint issues have been automatically fixed and committed to this PR.

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

Labels

integration-tests: skipped manually Changes in this PR do not require an integration test skip changeset This change does not need a changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants