-
Notifications
You must be signed in to change notification settings - Fork 0
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: add testing environment #73
base: main
Are you sure you want to change the base?
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.
Initial feeling is that the actual tests introduced here feel better-suited for being done in the actual app as opposed to the emulated environment. think it's fine for now given that we don't have that set up
Also, I'd prefer the tests
directory to be located at the renderer root instead of within its src
directory. see #76 for reasoning, but basically it better follows conventions around node-based projects.
import { render, screen } from '@testing-library/react' | ||
import { expect, test } from 'vitest' | ||
|
||
import { IntlProvider } from '../../contexts/IntlContext' | ||
import { MapLayout } from './_Map' | ||
import { router } from '../../App' |
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.
similar to how we should be working with zustand stores in tests, I think the tests should avoid using this singleton instance and instead create their own instance within the test to avoid potential issues related to shared instances across tests i.e.
import { routeTree } from '../../routeTree.gen'
describe('tabs navigate to correct routes', () => {
const router = createRouter({ routeTree })
render(<RouterProvider router={router} />, { wrapper: Wrapper })
// rest of tests...
expect(title).toBeVisible() | ||
}) | ||
|
||
test('Clicking "Settings" propogates "/Tab 2" to the navigator', () => { |
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.
test('Clicking "Settings" propogates "/Tab 2" to the navigator', () => { | |
test('Clicking "Settings" propagates "/Tab 2" to the navigator', () => { |
import { render, waitFor } from '@testing-library/react' | ||
import { describe, expect, test } from 'vitest' | ||
|
||
import { router } from '../App' |
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.
same comment about creating these router instances within the tests instead of using the singleton instance
export const ReuseableProviderWrapper = ({ | ||
children, | ||
}: { | ||
children: ReactNode | ||
}) => { | ||
return ( | ||
<ThemeProvider theme={theme}> | ||
<CssBaseline /> | ||
<IntlProvider>{children}</IntlProvider> | ||
</ThemeProvider> | ||
) | ||
} |
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 think abstracting this is necessary for the time being. would suggest just doing duplicate work of setting up the providers in the relevant test helper for now
import { createMapeoServer } from '@comapeo/ipc/dist/server.js' | ||
import { KeyManager } from '@mapeo/crypto' | ||
import Fastify from 'fastify' | ||
// @ts-expect-error - no types |
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.
if you add the following to the compilerOptions
in the project root tsconfig.json
, it should fix this issue:
"baseUrl": "./",
"paths": {
"*": ["node_modules/@digidem/types/vendor/*/index.d.ts"]
}
/
route and_Map
route