Switch test #786
Switch test #786
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/ui/Switch/tests/Switch.test.tsx (2)
13-23:⚠️ Potential issueFix role inconsistency and improve controlled mode coverage.
The test uses 'switch' role while other tests use 'checkbox' role. Additionally, it's missing important controlled mode scenarios.
This issue was previously identified. Please refer to the earlier review comment about improving controlled mode test coverage, which suggests:
- Using consistent role ('checkbox')
- Adding test for checked prop sync
25-41:⚠️ Potential issueFix incorrect prop combination in uncontrolled mode tests.
The tests incorrectly mix controlled (
checked) and uncontrolled (defaultChecked) props, which can lead to unexpected behavior.This issue was previously identified. Please refer to the earlier review comment about fixing uncontrolled mode tests, which provides a complete solution for properly testing uncontrolled behavior.
🧹 Nitpick comments (1)
src/components/ui/Switch/tests/Switch.test.tsx (1)
5-43: Fix formatting issues.Please address the following formatting issues:
- Remove extra blank lines after the describe block start (line 5-7)
- Remove extra blank lines before the describe block end (line 41-43)
- Add newline at end of file
describe('Switch Component', () => { - test('renders correctly', () => { // ... tests ... }) - }) +🧰 Tools
🪛 eslint
[error] 5-7: Block must not be padded by blank lines.
(padded-blocks)
[error] 41-43: Block must not be padded by blank lines.
(padded-blocks)
[error] 43-43: Newline required at end of file but not found.
(eol-last)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/components/ui/Switch/tests/Switch.test.tsx(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/ui/Switch/tests/Switch.test.tsx
[error] 5-7: Block must not be padded by blank lines.
(padded-blocks)
[error] 41-43: Block must not be padded by blank lines.
(padded-blocks)
[error] 43-43: Newline required at end of file but not found.
(eol-last)
🔇 Additional comments (1)
src/components/ui/Switch/tests/Switch.test.tsx (1)
1-5: LGTM! Clean and standard test setup.The imports and test suite setup follow best practices.
| test('renders correctly', () => { | ||
| render(<Switch checked={true} onChange={() => {}} color='' />) | ||
| const inputElement = screen.getByRole('checkbox') | ||
| expect(inputElement).toBeInTheDocument(); | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the basic render test with meaningful assertions.
The current test only verifies component presence. Consider adding:
- Accessibility attributes verification
- Default state checks
- Meaningful color prop value
test('renders correctly', () => {
- render(<Switch checked={true} onChange={() => {}} color='' />)
- const inputElement = screen.getByRole('checkbox')
- expect(inputElement).toBeInTheDocument();
+ render(<Switch checked={true} onChange={() => {}} color='primary' />)
+ const inputElement = screen.getByRole('checkbox')
+ expect(inputElement).toBeInTheDocument()
+ expect(inputElement).toHaveAttribute('aria-checked', 'true')
+ expect(inputElement).toHaveAttribute('type', 'checkbox')
+ expect(inputElement).toBeEnabled()
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('renders correctly', () => { | |
| render(<Switch checked={true} onChange={() => {}} color='' />) | |
| const inputElement = screen.getByRole('checkbox') | |
| expect(inputElement).toBeInTheDocument(); | |
| }) | |
| test('renders correctly', () => { | |
| render(<Switch checked={true} onChange={() => {}} color='primary' />) | |
| const inputElement = screen.getByRole('checkbox') | |
| expect(inputElement).toBeInTheDocument() | |
| expect(inputElement).toHaveAttribute('aria-checked', 'true') | |
| expect(inputElement).toHaveAttribute('type', 'checkbox') | |
| expect(inputElement).toBeEnabled() | |
| }) |
| }) | ||
|
|
||
| test('renders in controlled mode correctly', () => { | ||
| const handleChange = jest.fn(); |
There was a problem hiding this comment.
for controlled mode, create a state with useState and pass it to the component
when switch is toggled, check the state variable
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/ui/Switch/stories/Switch.stories.js (1)
39-45: Simplify uncontrolled component implementation and fix formatting.The uncontrolled story can be simplified by removing the unnecessary onChange handler. Also, there are some formatting issues to address.
export const Uncontrolled = () => { - - return <SandboxEditor> - <Switch defaultChecked ={true} onChange={() => {}}/> - - </SandboxEditor> + return <SandboxEditor> + <Switch defaultChecked={true} /> + </SandboxEditor> } +🧰 Tools
🪛 eslint
[error] 39-41: Block must not be padded by blank lines.
(padded-blocks)
[error] 45-45: Newline required at end of file but not found.
(eol-last)
src/components/ui/Switch/tests/Switch.test.tsx (1)
25-42: Clean up uncontrolled mode tests.The tests can be simplified by removing unnecessary onChange handlers and fixing formatting.
-test('renders in uncontrolled mode correctly with defaultChecked', () => { +describe('uncontrolled mode', () => { + test('initializes with defaultChecked', () => { + render(<Switch defaultChecked={true} />) + const checkbox = screen.getByRole('checkbox') + expect(checkbox).toBeChecked() + }) - render(<Switch defaultChecked={true} onChange={() => {}}/>) - const checkbox = screen.getByRole('checkbox') - expect(checkbox).toBeChecked() - }) - - test('toggles state independently',() => { - render(<Switch defaultChecked={false} onChange={() => {}} />) - const checkbox = screen.getByRole('checkbox') - expect(checkbox).not.toBeChecked() + test('toggles state independently', () => { + render(<Switch defaultChecked={false} />) + const checkbox = screen.getByRole('checkbox') + expect(checkbox).not.toBeChecked() - fireEvent.click(checkbox) - expect(checkbox).toBeChecked() - - fireEvent.click(checkbox) - expect(checkbox).not.toBeChecked() - }) + fireEvent.click(checkbox) + expect(checkbox).toBeChecked() + + fireEvent.click(checkbox) + expect(checkbox).not.toBeChecked() + }) +})🧰 Tools
🪛 eslint
[error] 25-27: Block must not be padded by blank lines.
(padded-blocks)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ui/Switch/Switch.tsx(2 hunks)src/components/ui/Switch/stories/Switch.stories.js(3 hunks)src/components/ui/Switch/tests/Switch.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/Switch/Switch.tsx
🧰 Additional context used
🪛 eslint
src/components/ui/Switch/stories/Switch.stories.js
[error] 39-41: Block must not be padded by blank lines.
(padded-blocks)
[error] 45-45: Newline required at end of file but not found.
(eol-last)
src/components/ui/Switch/tests/Switch.test.tsx
[error] 5-7: Block must not be padded by blank lines.
(padded-blocks)
[error] 25-27: Block must not be padded by blank lines.
(padded-blocks)
[error] 42-44: Block must not be padded by blank lines.
(padded-blocks)
[error] 44-44: Newline required at end of file but not found.
(eol-last)
🔇 Additional comments (4)
src/components/ui/Switch/stories/Switch.stories.js (2)
Line range hint
13-23: Fix controlled/uncontrolled component pattern mixing.The component is still mixing controlled and uncontrolled patterns by using both
defaultCheckedand state management. This can lead to unexpected behavior.- const [isChecked, setIsChecked] = useState(true) - - const handleChange = (state) => { - setIsChecked(state); - }; - return <SandboxEditor className="flex flex-col gap-2"> - {variants.map((variant, index) => ( - <Switch defaultChecked={args} key={index} variant={variant} onChange={handleChange} {...args} /> - ))} + return <SandboxEditor className="flex flex-col gap-2"> + {variants.map((variant, index) => ( + <Switch key={index} variant={variant} {...args} /> + ))}
28-37: LGTM! Well-implemented controlled component pattern.The controlled story demonstrates the correct usage pattern with proper state management and onChange handling.
src/components/ui/Switch/tests/Switch.test.tsx (2)
7-11: Enhance basic render test with accessibility checks.The test should verify accessibility attributes and meaningful props.
test('renders correctly', () => { - render(<Switch checked={true} onChange={() => {}} />) - const inputElement = screen.getByRole('checkbox') - expect(inputElement).toBeInTheDocument(); + render(<Switch checked={true} onChange={() => {}} color="primary" />) + const inputElement = screen.getByRole('checkbox') + expect(inputElement).toBeInTheDocument() + expect(inputElement).toHaveAttribute('aria-checked', 'true') + expect(inputElement).toHaveAttribute('type', 'checkbox') + expect(inputElement).toBeEnabled() })
13-23: Improve controlled mode test implementation.The test needs improvements in role consistency and state management verification.
-test('renders in controlled mode correctly', () => { +describe('controlled mode', () => { + test('calls onChange with correct values', () => { const handleChange = jest.fn(); render(<Switch checked={true} onChange={handleChange} />) - const switchElement = screen.getByRole('switch') + const switchElement = screen.getByRole('checkbox') fireEvent.click(switchElement) expect(handleChange).toHaveBeenCalledWith(false) fireEvent.click(switchElement) expect(handleChange).toHaveBeenCalledWith(true) - }) + }) + + test('stays in sync with state', () => { + const TestComponent = () => { + const [checked, setChecked] = React.useState(true) + return <Switch checked={checked} onChange={setChecked} /> + } + render(<TestComponent />) + const switchElement = screen.getByRole('checkbox') + + expect(switchElement).toBeChecked() + fireEvent.click(switchElement) + expect(switchElement).not.toBeChecked() + }) +})
kotAPI
added
the
automerge
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
kodiakhq
bot
removed
the
automerge
kotAPI
added
the
automerge
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
kodiakhq
bot
removed
the
automerge
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/ui/Switch/stories/Switch.stories.js (2)
31-33: Simplify the toggle handler.The toggle handler can be written more concisely.
- const handleToggle = () => { - setChecked((prev) => !prev) - } + const handleToggle = () => setChecked(prev => !prev);
39-44: Clean up the Uncontrolled story implementation.Several minor improvements can be made:
- Remove unnecessary padding
- Fix prop spacing
- Remove empty onChange handler since it's not needed
export const Uncontrolled = () => { - - return <SandboxEditor> - <Switch defaultChecked ={true} onChange={() => {}}/> - - </SandboxEditor> + return ( + <SandboxEditor> + <Switch defaultChecked={true} /> + </SandboxEditor> + ); }🧰 Tools
🪛 eslint
[error] 39-41: Block must not be padded by blank lines.
(padded-blocks)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ui/Switch/Switch.tsx(3 hunks)src/components/ui/Switch/stories/Switch.stories.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/Switch/Switch.tsx
🧰 Additional context used
🪛 eslint
src/components/ui/Switch/stories/Switch.stories.js
[error] 39-41: Block must not be padded by blank lines.
(padded-blocks)
🔇 Additional comments (1)
src/components/ui/Switch/stories/Switch.stories.js (1)
Line range hint
21-23: Fix incorrect prop passing and component control pattern.The Switch components have several issues:
defaultChecked={args}is incorrect - args is the full props object- Mixing controlled and uncontrolled patterns with
defaultCheckedandonChangeApply this fix:
{variants.map((variant, index) => ( - <Switch defaultChecked={args} key={index} variant={variant} onChange={handleChange} {...args}/> + <Switch checked={isChecked} key={index} variant={variant} onChange={handleChange} {...args}/> ))}
kotAPI
added
the
automerge
2 participants
Refactor and add test cases for switch component
Summary by CodeRabbit
Documentation
New Features
Tests
Refactor