test(Checkbox): mode switching tests (#1825)#2001
Conversation
|
📝 WalkthroughWalkthroughAdds a new test file for the ChangesCheckbox Controlled/Uncontrolled Switch Tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/ui/Checkbox/tests/Checkbox.controlledSwitch.test.tsx (2)
24-27: ⚡ Quick winAssert controlled-mode immutability after click.
After switching to controlled with
checked={false}, add an assertion thataria-checkedremainsfalseafter click (until parent rerender). This closes a regression gap where internal state might incorrectly mutate in controlled mode.Suggested assertion
expect(button).toHaveAttribute('aria-checked', 'false'); fireEvent.click(button); expect(onCheckedChange).toHaveBeenCalledWith(true); + expect(button).toHaveAttribute('aria-checked', 'false');🤖 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/Checkbox/tests/Checkbox.controlledSwitch.test.tsx` around lines 24 - 27, The test verifies that onCheckedChange is called with the correct value but does not assert that the aria-checked attribute remains 'false' after the click event in controlled mode. Add an assertion after the fireEvent.click(button) call to verify that aria-checked stays 'false' (before parent rerender), ensuring the Checkbox component does not mutate its internal state inappropriately when operating in controlled mode. This assertion should go after the click but before or alongside the onCheckedChange expectation to confirm immutability behavior.
14-15: ⚡ Quick winUse role-based queries instead of
container.querySelector('button').These selectors are brittle and produce weaker failure messages. Prefer
screen.getByRole('checkbox')(orgetByRolefrom render result) so the tests bind to the accessibility contract rather than DOM shape.Suggested update
-import { render, fireEvent } from '`@testing-library/react`'; +import { render, fireEvent, screen } from '`@testing-library/react`'; ... - const button = container.querySelector('button')!; + const button = screen.getByRole('checkbox'); ... - const button = container.querySelector('button')!; + const button = screen.getByRole('checkbox');Also applies to: 36-37
🤖 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/Checkbox/tests/Checkbox.controlledSwitch.test.tsx` around lines 14 - 15, The test code uses brittle DOM selectors like container.querySelector('button') which produce weak failure messages and don't bind to the accessibility contract. Replace container.querySelector('button') with screen.getByRole('checkbox') in the fireEvent.click calls to use role-based queries instead. This makes the tests more resilient to DOM structure changes and provides better failure messages by ensuring tests interact with elements based on their accessibility roles.
🤖 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/components/ui/Checkbox/tests/Checkbox.controlledSwitch.test.tsx`:
- Around line 24-27: The test verifies that onCheckedChange is called with the
correct value but does not assert that the aria-checked attribute remains
'false' after the click event in controlled mode. Add an assertion after the
fireEvent.click(button) call to verify that aria-checked stays 'false' (before
parent rerender), ensuring the Checkbox component does not mutate its internal
state inappropriately when operating in controlled mode. This assertion should
go after the click but before or alongside the onCheckedChange expectation to
confirm immutability behavior.
- Around line 14-15: The test code uses brittle DOM selectors like
container.querySelector('button') which produce weak failure messages and don't
bind to the accessibility contract. Replace container.querySelector('button')
with screen.getByRole('checkbox') in the fireEvent.click calls to use role-based
queries instead. This makes the tests more resilient to DOM structure changes
and provides better failure messages by ensuring tests interact with elements
based on their accessibility roles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7b6fec6-830d-4aca-86fa-f7ba14ed8419
📒 Files selected for processing (1)
src/components/ui/Checkbox/tests/Checkbox.controlledSwitch.test.tsx
CoverageThis report compares the PR with the base branch. "Δ" shows how the PR affects each metric.
Coverage improved or stayed the same. Great job! Run |
1 participant
Summary
Addresses #1825
Test plan
Summary by CodeRabbit