-
Notifications
You must be signed in to change notification settings - Fork 615
Update vitest config and TreeView tests to Vitest #6236
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.
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
Conversation
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
There was a problem hiding this 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 migrates existing Jest tests to Vitest for the TreeView
suite and refines the Vitest setup for both Node and browser environments, including style transformations and stricter console logging checks.
- Convert
TreeView
tests from Jest to Vitest (imports, timers, userEvent) - Update Vitest configs to include shared setup files, PostCSS, and browser CSS
- Introduce
vitest-fail-on-console
in common setup and remove legacy cleanup
Reviewed Changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
packages/react/vitest.config.mts | Added shared setup.ts for node-based tests |
packages/react/vitest.config.browser.mts | Configured PostCSS, scoped class names, and browser setup files |
packages/react/src/hooks/tests/useAnchoredPosition.test.tsx | Switched to async/waitFor and Vitest imports |
packages/react/src/TreeView/TreeView.test.tsx | Fully migrated TreeView tests to Vitest; timers, userEvent , act |
packages/react/config/vitest/setup.ts | Replaced manual cleanup with vitest-fail-on-console |
packages/react/config/vitest/browser/setup.ts | Added global CSS imports and DOM cleanup for browser tests |
packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx | Converted tests to async/userEvent and Vitest imports |
Comments suppressed due to low confidence (5)
packages/react/src/TreeView/TreeView.test.tsx:211
- This test body is entirely commented out, so no assertions run. Either remove or re-enable the test logic to ensure this behavior is actually verified.
it('should include `aria-expanded` when a SubTree contains content', async () => {
packages/react/src/TreeView/TreeView.test.tsx:1467
- You replaced an async
findByRole
with a synchronousgetByRole
for an element that appears during an async state. This may flake if the loading node isn't present immediately. Consider usingfindByRole
orwaitFor
.
const loadingItem = getByRole('treeitem', {name: 'Loading...'})
packages/react/src/TreeView/TreeView.test.tsx:1530
vi.waitFor
isn't imported; Vitest doesn’t exposewaitFor
onvi
by default. ImportwaitFor
from@testing-library/react
(or from Vitest if supported) to use it.
await vi.waitFor(() => {
packages/react/config/vitest/setup.ts:3
- You removed the
cleanup()
call for React testing in the node setup. Without callingcleanup
after each test, rendered DOM nodes may leak between tests. Re-introducecleanup()
in anafterEach
hook.
const failConsoleMessages = process.env.CI === 'true'
packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx:1
act
should be imported from@testing-library/react
(orreact-dom/test-utils
), not fromreact
itself. Please correct the import to ensure the testing helper works correctly.
import {act, useCallback, useState} from 'react'
@@ -18,16 +18,19 @@ const Component = ({callback}: {callback: (hookReturnValue: ReturnType<typeof us | |||
) | |||
} | |||
|
|||
it('should should return a position', () => { | |||
it('should should return a position', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The test description has a duplicated word: should should
. Please remove the extra should
so it reads should return a position
.
it('should should return a position', async () => { | |
it('should return a position', async () => { |
Copilot uses AI. Check for mistakes.
await act(async () => { | ||
await user.click(button) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Wrapping user.click
in act
is usually unnecessary with @testing-library/user-event
, which already handles React act
internally. You can simplify to await user.click(button)
.
await act(async () => { | |
await user.click(button) | |
}) | |
await user.click(button) |
Copilot uses AI. Check for mistakes.
expect(button).toHaveFocus() | ||
|
||
// Move focus to tree | ||
const item1 = getByRole('treeitem', {name: /Item 1/}) | ||
const toggle = item1.querySelector('.PRIVATE_TreeView-item-toggle') | ||
await user.click(toggle!) | ||
const toggle = item1.querySelector('.PRIVATE_TreeView-item-toggle') as HTMLElement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Querying by an internal class name is brittle. Consider exposing a data-testid or using an accessible query (e.g., getByRole
or getByLabelText
) to select the toggle element.
Copilot uses AI. Check for mistakes.
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Updates our TreeView tests from Jest to Vitest. This PR also includes some changes I needed to land in order to convert the tests, namely:
console.*
method is logged (helps to preventact()
warnings from sneaking in)Changelog
New
Changed
Removed
Rollout strategy
This is a change to our internal test setup and does not impact our public API.