Coverage #49
Coverage #49
Conversation
There was a problem hiding this comment.
Moved to DrawerContents.test.tsx
There was a problem hiding this comment.
Moved to SideDrawer.test.tsx
| @@ -21,3 +21,5 @@ i18next.use(initReactI18next).init({ | |||
| // set returnNull to false (and also in the i18next.d.ts options) | |||
| // returnNull: false, | |||
| }); | |||
|
|
|||
| export default i18next; | |||
There was a problem hiding this comment.
Read the docs and they exported this
There was a problem hiding this comment.
Yeah I think you are right about this, and it solves an issue I faced when I was working on tests. Good find.
| }); | ||
| expect(heading).toBeVisible(); | ||
|
|
||
| expect(await screen.findAllByRole('heading')).toHaveLength(5); |
There was a problem hiding this comment.
I lowered jsdom 1 level down since heading was not being recognized as a selector
There was a problem hiding this comment.
Thanks for taking a look at these changes and identifying a lot of opportunities for improvement.
It's better to keep code reviews small (under 200 LoC is best) and focused around a single change, so I'd like you to divide this into parts and resubmit it:
- vitest configuration improvements
- eslint styling changes (Code formatting #51 )
- adding new tests (may include test file renames)
We can talk about refactoring the components after the tests are in place. (Are you familiar with pindown tests? I didn't look at the commit order) However, I would caution against refactoring too much, unless we have clear reasons. If you'd like to chat or do some pair programming drop me a line.
| @@ -1,22 +1,22 @@ | |||
| import { FC } from 'react' | |||
There was a problem hiding this comment.
Let's separate stylistic changes from logical changes to make the review size smaller.
I noticed recently there is inconsistent semicolon usage. We can talk about whether we want the semi rule on or off, I've heard good arguments for both cases.
| const Header: FC = () => { | ||
| const theme = useTheme() |
There was a problem hiding this comment.
Not really sure why we are rewriting this. Prefer the original way
There was a problem hiding this comment.
Sorry, my formatter adds semicolons :orz: will remove these
There was a problem hiding this comment.
Here I'm referring to the refactor from lines 9-20
There was a problem hiding this comment.
The test coverage could not reach inner = (<MobileHeader />) for some reason so I rewrote it but let me check it again
| @@ -1,14 +0,0 @@ | |||
| import { describe, expect, it } from 'vitest' | |||
There was a problem hiding this comment.
Was this a delete or a rename? Looking now, the title seems redundant.
git mv may be helpful in making the diff show the rename
There was a problem hiding this comment.
Ah, after fixing the vitest config, it is now okay not to import describe, expect, it, etc from vitest 🙇♀️
| @@ -21,3 +21,5 @@ i18next.use(initReactI18next).init({ | |||
| // set returnNull to false (and also in the i18next.d.ts options) | |||
| // returnNull: false, | |||
| }); | |||
|
|
|||
| export default i18next; | |||
There was a problem hiding this comment.
There's a lot of good things you're noticing, though they are out of scope.
Better to have one PR for one task, and keep 'em small 🤏🏻
| } |
There was a problem hiding this comment.
Please confirm that you have the .editorconfig plugin installed for your editor: https://editorconfig.org/
(I'll be sure to add this to the readme to make it clearer)
There was a problem hiding this comment.
Oh I might be looking at this backwards... looks like you are following the correct formatting
| const AllTheProviders = ({ children }: React.PropsWithChildren) => { | ||
| return ( | ||
| <CustomThemeProvider> | ||
| <CssBaseline /> | ||
| <MemoryRouter>{children}</MemoryRouter> | ||
| </CustomThemeProvider> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
moved AllTheProviders inside customRender so there is no need to do // eslint-disable-next-line react-refresh/only-export-components
| options?: Omit<RenderOptions, 'wrapper'>, | ||
| ) => render(ui, { wrapper: AllTheProviders, ...options }) | ||
| return { | ||
| user: userEvent.setup(), |
There was a problem hiding this comment.
added userEvent.setup() here so we don't manually write this in every test case that needs it
| plugins: [react()], | ||
| resolve: { | ||
| alias: { | ||
| '@': path.resolve(__dirname, 'src') | ||
| } | ||
| }, |
There was a problem hiding this comment.
used mergeConfig and the config defined in vite.config.ts so we only need to change the vite config in one place and have it reflect here too
| @@ -1,4 +1,3 @@ | |||
| import { describe, expect, it } from 'vitest' | |||
There was a problem hiding this comment.
It's ok not to import this 👍
|
Will resubmit! |
Resolves #6
What changed 🧐
Please list the changes that are in this PR
Header,LocalToggle,SideDrawer,DrawerContents.customRenderto includeuserEvent.setupso it does not need to be setup manually in each test case.jsdomsince theheadingselector is not working inv24.0.0.routesand the scope of the issue only mentionscomponentsso I didn't include it in this PR.💡 I used
getByRoleandfindByRolea lot in the tests. Fromreact-testing-librarydocs:How did you test it? 🧪
Please describe new unit tests, E2E tests, or manual testing steps.
Run
npm run test.