Create test to select#78
Conversation
📝 WalkthroughWalkthroughAdds a theme-aware RTL render helper and a Vitest test suite for SGLSelect covering dropdown open, option selection, Escape dismissal, and changing selections. ChangesSGLSelect Test Suite
Possibly Related PRs
Suggested Reviewers
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/components/UI/Select/SGLSelect.test.tsx (1)
7-9: 💤 Low valueConsider removing redundant cleanup.
React Testing Library automatically performs cleanup after each test when used with Vitest, making this
afterEachcall unnecessary.♻️ Simplify by removing redundant cleanup
-afterEach(() => { - cleanup() -}) - describe('SGLSelect', () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/UI/Select/SGLSelect.test.tsx` around lines 7 - 9, The afterEach block calling cleanup() is redundant because React Testing Library with Vitest auto-cleans between tests; remove the afterEach(() => { cleanup() }) block from the SGLSelect.test.tsx file (the explicit cleanup call) so tests rely on the test runner's automatic teardown and avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/UI/Select/SGLSelect.test.tsx`:
- Line 25: Remove the trailing whitespace and extra newline after the test block
closing token shown as "})" in SGLSelect.test.tsx; replace the "}) " with a
clean "})" (no spaces or extra blank line) so the file passes Prettier
formatting; ensure there are no other trailing spaces in the file.
- Around line 28-37: The test currently renders <SGLSelect options={options}
value="" /> so asserting the combobox does not contain "Banana" cannot verify
Escape behavior; update the test to render with a non-empty initial value (e.g.,
value="Banana") or with a controlled value variable set to "Banana", open the
dropdown via fireEvent.mouseDown(select), send Escape with
fireEvent.keyDown(select, { key: 'Escape' }), and then assert the combobox still
contains "Banana" (or that the controlled value didn't change) to confirm the
original value is preserved after pressing Escape; use the existing SGLSelect
component and the select element obtained by screen.getByRole('combobox') to
locate the element.
- Around line 39-57: The third test case "should change the selected value when
selecting a different option" in SGLSelect.test.tsx has incorrect 2-space
indentation causing Prettier errors; update the block that renders <SGLSelect
options={options} />, the queries (screen.getByRole/getByText), fireEvent calls,
and expect assertions to use the project's 4-space indentation for all nested
lines so the file passes formatting checks and the test remains functionally
unchanged.
---
Nitpick comments:
In `@src/components/UI/Select/SGLSelect.test.tsx`:
- Around line 7-9: The afterEach block calling cleanup() is redundant because
React Testing Library with Vitest auto-cleans between tests; remove the
afterEach(() => { cleanup() }) block from the SGLSelect.test.tsx file (the
explicit cleanup call) so tests rely on the test runner's automatic teardown and
avoid duplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb047916-8e1d-41f3-9e30-26a14225cb21
📒 Files selected for processing (1)
src/components/UI/Select/SGLSelect.test.tsx
| it('should not select any item when dropdown is opened and closed', () => { | ||
| render(<SGLSelect options={options} value="" />) | ||
|
|
||
| const select = screen.getByRole('combobox') | ||
|
|
||
| fireEvent.mouseDown(select) | ||
| fireEvent.keyDown(select, { key: 'Escape' }) | ||
|
|
||
| expect(select).not.toHaveTextContent('Banana') | ||
| }) |
There was a problem hiding this comment.
Strengthen the Escape key test assertion.
The current assertion doesn't properly verify Escape behavior. Since value="" is set initially, the combobox would not contain "Banana" regardless of whether Escape works correctly. The test should verify that the original value is preserved after pressing Escape.
🧪 Improved test for Escape key behavior
- it('should not select any item when dropdown is opened and closed', () => {
- render(<SGLSelect options={options} value="" />)
+ it('should preserve the selected value when Escape is pressed', () => {
+ render(<SGLSelect options={options} />)
const select = screen.getByRole('combobox')
+
+ // Initially should show first option (Apple) due to defaultValue
+ expect(select).toHaveTextContent('Apple')
fireEvent.mouseDown(select)
fireEvent.keyDown(select, { key: 'Escape' })
- expect(select).not.toHaveTextContent('Banana')
+ // After Escape, should still show Apple
+ expect(select).toHaveTextContent('Apple')
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/UI/Select/SGLSelect.test.tsx` around lines 28 - 37, The test
currently renders <SGLSelect options={options} value="" /> so asserting the
combobox does not contain "Banana" cannot verify Escape behavior; update the
test to render with a non-empty initial value (e.g., value="Banana") or with a
controlled value variable set to "Banana", open the dropdown via
fireEvent.mouseDown(select), send Escape with fireEvent.keyDown(select, { key:
'Escape' }), and then assert the combobox still contains "Banana" (or that the
controlled value didn't change) to confirm the original value is preserved after
pressing Escape; use the existing SGLSelect component and the select element
obtained by screen.getByRole('combobox') to locate the element.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
Did you checked yourself locally? your tests are failing:
Before you pushed the code did you tests passed?
|
@Tamir198 Fixed the failing tests. I verified locally and all tests are now passing. |
|
|
||
| it('should select the relevant item', () => { | ||
| render( | ||
| <ThemeProvider theme={theme}> |
There was a problem hiding this comment.
This can be extracted into general helper component, we can put it inside src/tests/....
There was a problem hiding this comment.
Do you want me to create a shared test helper under src/tests and move the ThemeProvider wrapper there?
There was a problem hiding this comment.
Done, I extracted the ThemeProvider wrapper into a shared helper under src/tests and updated the tests to use it.
| it('should not select any item when dropdown is opened and closed', () => { | ||
| render(<SGLSelect options={options} value="" />) | ||
|
|
||
| const select = screen.getByRole('combobox') | ||
|
|
||
| fireEvent.mouseDown(select) | ||
| fireEvent.keyDown(select, { key: 'Escape' }) | ||
|
|
||
| expect(select).not.toHaveTextContent('Banana') | ||
| }) |
|
|
||
| import { theme } from '../theme' | ||
|
|
||
| export const renderWithTheme = (ui: ReactElement) => |
There was a problem hiding this comment.
React got a prop for this called children
There was a problem hiding this comment.
Do you mean using a wrapper component with children and passing it to render as wrapper?
There was a problem hiding this comment.
There was a problem hiding this comment.
Done, updated renderWithTheme to use a wrapper component with children.
| it('should not select any item when dropdown is opened and closed', () => { | ||
| render(<SGLSelect options={options} value="" />) | ||
|
|
||
| const select = screen.getByRole('combobox') | ||
|
|
||
| fireEvent.mouseDown(select) | ||
| fireEvent.keyDown(select, { key: 'Escape' }) | ||
|
|
||
| expect(select).not.toHaveTextContent('Banana') | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tests/renderWithTheme.tsx (1)
7-11: ⚡ Quick winResolve the Fast Refresh lint warning by inlining the wrapper.
Line 7 defines a top-level component in a file exporting a non-component utility, which triggers the react-refresh warning. Inlining the wrapper inside
renderWithThemekeeps behavior unchanged and removes the warning.Suggested diff
-import type { PropsWithChildren, ReactElement } from 'react' +import type { PropsWithChildren, ReactElement } from 'react' @@ -const Wrapper = ({ children }: PropsWithChildren) => ( - <ThemeProvider theme={theme}>{children}</ThemeProvider> -) - -export const renderWithTheme = (ui: ReactElement) => render(ui, { wrapper: Wrapper }) +export const renderWithTheme = (ui: ReactElement) => { + const wrapper = ({ children }: PropsWithChildren) => ( + <ThemeProvider theme={theme}>{children}</ThemeProvider> + ) + return render(ui, { wrapper }) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/renderWithTheme.tsx` around lines 7 - 11, The top-level Wrapper component causes a Fast Refresh lint warning; remove the exported Wrapper and inline it inside renderWithTheme by replacing the external Wrapper reference with an inline wrapper function passed to render (e.g., wrapper: ({ children }) => <ThemeProvider theme={theme}>{children}</ThemeProvider>); update renderWithTheme to accept the same ReactElement input and call render(ui, { wrapper: inlineWrapper }) so ThemeProvider and theme usage remains unchanged while eliminating the top-level component (references: Wrapper, renderWithTheme, ThemeProvider, theme).Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/tests/renderWithTheme.tsx`:
- Around line 7-11: The top-level Wrapper component causes a Fast Refresh lint
warning; remove the exported Wrapper and inline it inside renderWithTheme by
replacing the external Wrapper reference with an inline wrapper function passed
to render (e.g., wrapper: ({ children }) => <ThemeProvider
theme={theme}>{children}</ThemeProvider>); update renderWithTheme to accept the
same ReactElement input and call render(ui, { wrapper: inlineWrapper }) so
ThemeProvider and theme usage remains unchanged while eliminating the top-level
component (references: Wrapper, renderWithTheme, ThemeProvider, theme).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e00265d4-2c36-4af4-9de6-282ec38ace1b
📒 Files selected for processing (1)
src/tests/renderWithTheme.tsx
| <ThemeProvider theme={theme}>{children}</ThemeProvider> | ||
| ) | ||
|
|
||
| export const renderWithTheme = (ui: ReactElement) => render(ui, { wrapper: Wrapper }) |
There was a problem hiding this comment.
You can just export the Wrapper component. Also rename it to more indicative name
| it('should not select any item when dropdown is opened and closed', () => { | ||
| renderWithTheme(<SGLSelect options={options} value="" />) | ||
|
|
||
| const select = screen.getByRole('combobox') | ||
|
|
||
| fireEvent.mouseDown(select) | ||
| fireEvent.keyDown(select, { key: 'Escape' }) | ||
|
|
||
| expect(select).not.toHaveTextContent('Banana') | ||
| }) |
There was a problem hiding this comment.
As @Tamir198 and the coderabbit said, you have a logic flaw. If you wont click anything the value obviously wont be "Banana". Consider to change the test for trying to click "Banana" option while not appears.
3 participants
Description
Please include a summary of the changes and the related issue.
Related Issue(s)
Fixes # (issue number)
Checklist:
Screenshots (if appropriate):
Summary by CodeRabbit