test(component): refactor Counter component tests #658
Conversation
There was a problem hiding this comment.
Hey @bc-alexsaiannyi!
Thanks for this PR, looking good, I just added a few comments, let me know what you think about them.
🙇
|
|
||
| const counter = getByDisplayValue('5') as HTMLInputElement; | ||
| const counter = screen.getByRole<HTMLInputElement>('textbox'); |
There was a problem hiding this comment.
I don't think the type (HTMLInputElement) is necessary anywhere in the file, consider removing it.
| const item1 = screen.getByTestId<HTMLInputElement>('item1'); | ||
| const item2 = screen.getByTestId<HTMLInputElement>('item2'); |
There was a problem hiding this comment.
No need to cast them to HTMLInputElement, all HTMLElements have ids
bc-alexsaiannyi
force-pushed
the
bigdesign-635-counter
branch
from
739d0b6
to
623ce36
Compare
|
Hey @icatalina. I've added changes. Thank you for going through and adding all your suggestions. |
bc-alexsaiannyi
force-pushed
the
bigdesign-635-counter
branch
from
623ce36
to
e425427
Compare
| expect(ref.current).toBe(counter); | ||
| render(<Counter value={5} onCountChange={jest.fn()} ref={ref} />); | ||
|
|
||
| const input = screen.getByRole<HTMLInputElement>('textbox'); |
There was a problem hiding this comment.
Do we need to cast to HTMLInputElement?
| const { container } = render(counterMock({ max: 20, ...requiredAttributes })); | ||
| render(<CounterMock value={5} />); | ||
|
|
||
| const input = screen.getByRole<HTMLInputElement>('textbox'); |
There was a problem hiding this comment.
No need to cast to HTMLInputElement:
|
|
||
| expect(container.querySelector('[class*="StyledError"]')).not.toBeInTheDocument(); | ||
| expect(screen.queryByText('Error')).not.toBeInTheDocument(); |
There was a problem hiding this comment.
Why do we swap between error and 'Error'?
bc-alexsaiannyi
force-pushed
the
bigdesign-635-counter
branch
from
e425427
to
911de93
Compare
|
I'm going to close this since I already did most of it in #1187 |
This PR is aimed to refactor Counter component's tests according to best practices. We take into account recommendations from Kent C. Dodds article. For instance, using
containerforquery-ing elements has been removed. Instead, we replace it withscreen.Why?
the changes are based on GitHub issue and PRs will be open for each component separately.
Testing/Proof
The tests have been successfully run locally.