Skip to content

Conversation

@joshblack
Copy link
Member

@joshblack joshblack commented Nov 17, 2025

Closes https://github.com/github/primer/issues/6140

Changelog

New

Changed

  • Update or refactor tests to work in React 18/19

Removed

Rollout strategy

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

This is a change to our internal test suite

@changeset-bot
Copy link

changeset-bot bot commented Nov 17, 2025

⚠️ No Changeset found

Latest commit: daa5c82

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 the skip changeset This change does not need a changelog label Nov 17, 2025
@github-actions github-actions bot added staff Author is a staff member integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Nov 17, 2025
@github-actions
Copy link
Contributor

👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the integration-tests: skipped manually label to skip these checks.

@github-actions github-actions bot temporarily deployed to storybook-preview-7209 November 19, 2025 18:31 Inactive
@joshblack joshblack marked this pull request as ready for review November 20, 2025 16:03
@joshblack joshblack requested a review from a team as a code owner November 20, 2025 16:03
Copilot finished reviewing on behalf of joshblack November 20, 2025 16:06
Copy link
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

This PR updates the test suites to ensure compatibility with React 19 by removing obsolete test patterns and migrating from snapshot testing to targeted behavioral assertions. The changes primarily focus on removing console.error mocking (which React 19 handles differently), replacing snapshot tests with specific assertions, and removing the continue-on-error flag from CI for React 19 tests.

Key Changes

  • Removed console.error mocking: React 19 no longer requires tests that use expect().toThrow() to mock console.error, so all vi.spyOn(console, 'error') code has been removed from error-testing scenarios
  • Migrated from snapshots to targeted assertions: Replaced toMatchSnapshot() calls with specific behavioral checks like toHaveAttribute(), toBeInTheDocument(), and getByRole() queries to make tests more maintainable and explicit
  • CI workflow update: Removed the continue-on-error: ${{ matrix.react-version == 'react-19' }} flag, indicating that React 19 tests are now expected to pass reliably

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/react/src/experimental/UnderlinePanels/UnderlinePanels.test.tsx Removed console.error mocking from three error-throwing test cases
packages/react/src/UnderlineNav/UnderlineNav.test.tsx Removed console.error mocking from error-throwing test for multiple aria-current items
packages/react/src/TooltipV2/__tests__/Tooltip.test.tsx Removed console.error mocking and unused vi import from disabled trigger error test
packages/react/src/TextInput/TextInput.test.tsx Changed import path, replaced snapshot tests with targeted assertions for size/block/visual/action rendering, removed loading indicator snapshot test
packages/react/src/TextInput/__snapshots__/TextInput.test.tsx.snap Removed numerous snapshot entries that are no longer used (renders, size variants, visuals, actions, loading indicators)
packages/react/src/NavList/NavList.test.tsx Removed snapshot-based tests for simple list rendering, groups rendering, and SubNav active state styling
packages/react/src/NavList/__snapshots__/NavList.test.tsx.snap Removed all snapshot entries for NavList tests
packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx Removed snapshot test for open state rendering
packages/react/src/AnchoredOverlay/__snapshots__/AnchoredOverlay.test.tsx.snap Removed snapshot entry for AnchoredOverlay open state
packages/react/src/ActionList/Heading.test.tsx Removed console.error mocking and unused vi import from ActionMenu context error test
packages/react/src/ActionList/Group.test.tsx Removed console.error mocking from two error-throwing test cases for GroupHeading validation
.github/workflows/ci.yml Removed continue-on-error flag for React 19 test runs, indicating tests now fully support React 19

Copy link
Member

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, could you elaborate on why snapshots need to be removed/don't work anymore?

@joshblack
Copy link
Member Author

@francinelucca the big thing with the snapshots were that the ids collided (in React 19 the way useId() generates ids has changed). They also weren't that helpful so I either rewrote them if it made sense or removed them if they already had a visual alternative for the test

@joshblack joshblack added this pull request to the merge queue Nov 20, 2025
Merged via the queue into main with commit f21a50c Nov 20, 2025
49 checks passed
@joshblack joshblack deleted the test/update-react-19-suite branch November 20, 2025 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm skip changeset This change does not need a changelog staff Author is a staff member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants