Router tests #160
Router tests #160
Conversation
| ) | ||
| }) | ||
|
|
||
| it('should render Suspense with fallback loader', () => { |
There was a problem hiding this comment.
Let's be careful here that we're not writing tests for React / React Router core features. That is outside the scope of our project
There was a problem hiding this comment.
I wanted to test if the "loading..." text is rendered when suspended (mostly to get 100 coverage for BaseLayout) but we can remove it if unneeded
There was a problem hiding this comment.
Oh, 100% coverage isn't something that necessarily brings better quality. Usually the best benefit is 80-90%.
Anyway, the test is easier to read understand now so I think it's good to leave in.
src/routes/__test__/Router.test.tsx
Outdated
| </TestWrapper> | ||
| ) | ||
|
|
||
| // Should render without errors |
There was a problem hiding this comment.
It's good we're ensuring the base layout is working. Can we add assertions on the other parts of the router?
If it's cumbersome, feel free to pause and we can discuss if it's better to focus on E2E tests to verify the router works well.
There was a problem hiding this comment.
looking at it now, i think you are right, we should just focus on testing routing behavior with E2E tests 🙏
| const data = mockFetchData() | ||
| if (data instanceof Promise) { | ||
| // eslint-disable-next-line @typescript-eslint/only-throw-error | ||
| throw data |
There was a problem hiding this comment.
Why are we throwing a Promise? This seems odd
There was a problem hiding this comment.
this is a "hack" that I learned from my coworker and the old data fetching strategy we used for our products in the past. basically, throwing the Promise (pending data) triggers the suspense and shows the fallback, then re-renders the component when the Promise has resolved.
i've changed it to use lazy instead as it can also trigger the suspense.
in other news, the "throw a Promise" hack to trigger the suspense will be deprecated in a future version of react
|
This PR is stale. Consider a pair-programming session to help move it forward. |
There was a problem hiding this comment.
kept this unit test lean, i know coverage isn't everything but having this test satisfies 100% coverage for Router.tsx, definitely better to test routing behavior in E2E
| </CustomThemeProvider> | ||
| // Create a lazy component for testing loading state | ||
| const MockLazyChildComponent = lazy(() => | ||
| new Promise(() => {}) // Never resolves, so Suspense will show fallback |
sirbully
force-pushed
the
image-card-test
branch
from
4e87c9c to
f62acc7
Compare
Resolves #99
What changed 🧐
Tests for
BaseLayoutandRouterHow did you test it? 🧪
Mocked child components and assert component behavior