Week 2 tests #233
Week 2 tests #233
Conversation
|
I am yet to checkout this branch and run the app. But one thing I would like to point out is that not using |
|
I understand. I've actually replaced |
There was a problem hiding this comment.
I have left a few suggestions as inline comments to improve some of the tests.
I would encourage to avoid writing tests where the only assertion is expect(text).toBeInTheDocument() especially where text is not a result of a specific DOM query. For example the Sidebar test for version number. Feel free to sprinkle as many data-testids as you want and use them to query in such simple tests.
| renderComponent( | ||
| <NavItem | ||
| key={idx} | ||
| pathName="/dashboard" |
There was a problem hiding this comment.
Why is /dashboard hard coded here?
| expect(screen.getByText('Test paragraph')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| test('NavItem renders Navigation List correctly in the Sidebar', async () => { |
There was a problem hiding this comment.
Can we not directly test by rendering <Sidebar> and checking if it has all the items in navigationPath and if each of them navigates to the right page?
| const runButton = screen.getByText(English['Dashboard.Overview.Run']) | ||
| expect(runButton).toBeInTheDocument() | ||
| fireEvent.click(runButton) | ||
| expect(onRunTest).toHaveBeenCalledTimes(1) |
There was a problem hiding this comment.
If we are only testing if the callback was called, then we should be testing the hook useRunTest also.
| jest.clearAllMocks() | ||
| }) | ||
|
|
||
| test('All the Test Cards are mounted', async () => { |
There was a problem hiding this comment.
This test is relevant only if we are testing by rendering pages/dashboard/index.js and checking if all there are cards for each item in testList. Otherwise we are manually mounting all the cards in the test setup itself.
| /> | ||
| ) | ||
|
|
||
| const testCard = screen.getByTestId('card') |
There was a problem hiding this comment.
Also test if testCard has visible content that matches what's in websiteDetails
|
I have removed some tests which I felt were not relevant at all. I have only kept the ones which I felt added confidence in the component. |
I have modified the
Sidebar.jscomponent to make use ofuseRouter()fromnext/router. There were some bugs I faced on simulating click interaction. On reading about the issues, I came to know thatnext/linkcomponent that we use currently has some bugs and is not a great API, having inconsistencies in its implementation.References:
vercel/next.js#16864 (comment)
vercel/next.js#1490 (comment)