-
Notifications
You must be signed in to change notification settings - Fork 7
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
chore: init testing #2
Conversation
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.
As a side not it's worth talking about the granularity of the tests.
tests/theme.test.tsx
Outdated
import { ThemeToggle } from '@/features/theme/components/theme-toggle' | ||
import userEvent from '@testing-library/user-event' | ||
|
||
describe('when using the default system light theme', () => { |
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.
I personally like the tests living next to the component. I better matches the feature oriented structure. Thoughts on this?
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.
I don't mind to be honest. I moved the test files next to the components.
@@ -10,7 +10,7 @@ export function ThemeToggle() { | |||
return ( | |||
<DropdownMenu> | |||
<DropdownMenuTrigger asChild> | |||
<Button variant="default" size="icon"> | |||
<Button variant="default" size="icon" data-testid="theme-toggle-menu"> |
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.
It's generally better to use roles and aria attributes, rather than a testid, as it makes the markup more accessible.
<Button variant="default" size="icon" data-testid="theme-toggle-menu"> | |
<Button variant="default" size="icon" aria-label="Toggle Theme"> |
then the selector in the test would be something like:
await screen.findByRole('button', { name: "Toggle Theme" })
It might be also worth creating a constants.ts
which holds these label values to shared between tests and the component.
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.
Nice, TIL that the name
in findByRole
is the accessible name.
No description provided.