Render the whole app in page-level tests #565
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/oxidecomputer/console-ui-storybook/LCe29rjY3e3C14m9W2fTssP5shUn |
|
Preview will be deployed at https://console-git-render-app-at.internal.oxide.computer |
david-crespo
force-pushed
the
render-app-at
branch
from
2e72887 to
034b41e
Compare
david-crespo
force-pushed
the
render-app-at
branch
from
034b41e to
f23f475
Compare
david-crespo
force-pushed
the
render-app-at
branch
from
f23f475 to
97fe39a
Compare
david-crespo
force-pushed
the
render-app-at
branch
from
97fe39a to
d897782
Compare
david-crespo
changed the title
| expect(history.location.pathname).toEqual( | ||
| `/orgs/${org.name}/projects/${project.name}/instances` | ||
| ) | ||
| ) |
There was a problem hiding this comment.
This is a little funny because we actually navigate to the project route, but that redirects to project instances.
david-crespo
force-pushed
the
render-app-at
branch
from
d897782 to
1c856ba
Compare
| await waitFor(() => expect(submit).toBeDisabled()) | ||
| expect(mock.done()).toBeTruthy() | ||
| expect(submit).not.toBeDisabled() | ||
| expect(mock.called(undefined, 'POST')).toBeTruthy() |
There was a problem hiding this comment.
This mock variable refers not only, as you would expect, to the post mock but to all mock requests to this URL, which is really annoying and dumb, and requires me to filter here to only the POST. Making this change made me realize how bad fetch-mock is. If we want to keep mocking fetch (which we may not) I think we should find another library or just wrap global.fetch ourselves, as the linked posts do.
https://github.com/jefflau/jest-fetch-mock
https://benjaminjohnson.me/mocking-fetch
https://www.leighhalliday.com/mock-fetch-jest
There's also Kent C. Dodds's post about why Mock Service Workers is better than mocking fetch. I never found it too convincing, but this PR has definitely made me more amenable to it.
david-crespo
force-pushed
the
render-app-at
branch
from
2b54bee to
e25e4c4
Compare
|
Tried upgrading but it caused weird failures in the "disables submit button" tests that didn't seem worth the time to debug. |
| }) | ||
| const renderPage = () => { | ||
| // fetch projects list for org layout sidebar on project create | ||
| fetchMock.get(projectsUrl, { status: 200, body: { items: [] } }) |
There was a problem hiding this comment.
feels silly to have to do this. definitely a point against the whole approach
david-crespo
force-pushed
the
render-app-at
branch
from
e25e4c4 to
ef3315d
Compare
david-crespo
force-pushed
the
render-app-at
branch
from
2aba869 to
e26a8a2
Compare
david-crespo
force-pushed
the
render-app-at
branch
from
e26a8a2 to
89cd088
Compare
david-crespo
changed the title
| {getServerError(createProject.error, ERROR_CODES)} | ||
| </div> | ||
| </Form> | ||
| </Formik> | ||
| </> | ||
| ) | ||
| } |
There was a problem hiding this comment.
changes to this file and InstanceCreatePage are much less interesting than they look, and they shrink quite a bit with whitespace changes hidden — just moving the contents of the Form component into the page since we no longer need the former for testing purposes
|
|
||
| fireEvent.click(submitButton()) | ||
|
|
||
| await waitFor(() => expect(mock.called()).toBeTruthy()) | ||
| await waitFor(() => expect(mock.done()).toBeTruthy()) |
There was a problem hiding this comment.
turns out these weren't doing what I thought because fetchMock is horrible? I'll see if I can bring them back properly
I've felt our test conventions for pages were a little lacking. I didn't really like the tweaks I made in #535 even if asserting on an
onSuccessspy is a slight improvement on hacky asserts onwindow.location. Turns out asserting onwindow.locationis fine, you just have to usehistory.pushStateto set the current route at the beginning of every test. This is easy if you bake it intorenderAppAt(url)as I have here.Summary of changes
renderAppAt(url)does what it sounds likeInstanceCreatePageandProjectCreatePagetests use itInstanceCreatePageandProjectCreatePagenow that we don't need an innerFormwith props for route params andonSuccessfor testing purposesBad stuff
fetch-mockis still kind of horrible, but that's orthogonal to this change. I'm just using it slightly betterOutdated commentary about spying on `history` that I don't want to delete
I realized we can non-hackily assert on location if we have access to the
historyobject used by React Router. This SO answer says how to do that with aRouter(as opposed to, e.g.,MemoryRouterorBrowserRouter) but in RR 6.1 they added aHistoryRouterthat makes it as simple as<HistoryRouter history={history}>. (Note that in 6.1.1 they changed the export name tounstable_HistoryRouterbecause they expect the API to change, but I don't really care about that — we can easily update when they change it.)So based on that, we would want a helper that renders the component under test inside a
HistoryRouterand returns the history object so we can assert onhistory.location. The problem here is that if we want to use relative routes (we probably do? one could certainly argue otherwise) they will produce weird results unless we're rendering our actual route tree in the tests. And... why not?