Skip to content

Replace fetch-mock with Mock Service Workers #571

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.

Sign up for GitHub

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

Merged
david-crespo merged 8 commits into from
Jan 11, 2022
Merged

Replace fetch-mock with Mock Service Workers #571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
db711b6
working msw server in one test
david-crespo Jan 10, 2022
6885090
fancy VpcPage test now works with MSW
david-crespo Jan 11, 2022
fc39705
InstanceCreatePage and ProjectCreatePage tests work
david-crespo Jan 11, 2022
0668de7
Merge main into msw
david-crespo Jan 11, 2022
ee6ba83
forgot to delete one more fetch-mock
david-crespo Jan 11, 2022
a30e2bb
make CI pass
david-crespo Jan 11, 2022
d6fa036
move msw into @oxide/api-mocks and thereby fix import paths
david-crespo Jan 11, 2022
80fc66f
:orgId -> :orgName because that's what they are
david-crespo Jan 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/lintBuildTest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ jobs:
- name: Lint
run: yarn lint

- name: Test
run: yarn test

- name: Build
run: yarn build

- name: Build for Nexus
run: yarn build-for-nexus

- name: Test
run: yarn test
Copy link
Collaborator Author

@david-crespo david-crespo Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moving this before build permanently because the two builds take 30 seconds and basically never fail. might as well get test failures sooner

74 changes: 17 additions & 57 deletions app/pages/__tests__/InstanceCreatePage.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,17 @@
import {
fireEvent,
lastPostBody,
renderAppAt,
screen,
waitFor,
} from 'app/test-utils'
import fetchMock from 'fetch-mock'

import { org, project, instance, sessionMe } from '@oxide/api-mocks'
import { fireEvent, renderAppAt, screen, waitFor } from 'app/test-utils'
import { msw, org, project } from '@oxide/api-mocks'

const submitButton = () =>
screen.getByRole('button', { name: 'Create instance' })

const projectUrl = `/api/organizations/${org.name}/projects/${project.name}`
const instancesUrl = `${projectUrl}/instances`
const disksUrl = `${projectUrl}/disks`
const vpcsUrl = `${projectUrl}/vpcs`

const formUrl = `/orgs/${org.name}/projects/${project.name}/instances/new`

const renderPage = () => {
// existing disk modal fetches disks on render even if it's not visible
fetchMock.get('/api/session/me', { status: 200, body: sessionMe })
fetchMock.get(disksUrl, 200)
fetchMock.get(vpcsUrl, 200)
fetchMock.get(projectUrl, 200)
return renderAppAt(formUrl)
}

describe('InstanceCreatePage', () => {
it('disables submit button on submit', async () => {
fetchMock.post(instancesUrl, 201)
renderPage()
renderAppAt(formUrl)

const submit = submitButton()
expect(submit).not.toBeDisabled()
Expand All @@ -42,11 +22,10 @@ describe('InstanceCreatePage', () => {
})

it('shows specific message for known server error code', async () => {
fetchMock.post(instancesUrl, {
status: 400,
body: { error_code: 'ObjectAlreadyExists' },
msw.override('post', instancesUrl, 400, {
error_code: 'ObjectAlreadyExists',
})
renderPage()
renderAppAt(formUrl)
Copy link
Collaborator Author

@david-crespo david-crespo Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is clean enough, but we could also hard code a special instance name like "dupe" in the handler and have it return this error when you post an instance with that name. Or probably even better, once we have a fake instances list, we can have it actually detect a duplicate name.


fireEvent.click(submitButton())

Expand All @@ -58,11 +37,8 @@ describe('InstanceCreatePage', () => {
})

it('shows generic message for unknown server error', async () => {
fetchMock.post(instancesUrl, {
status: 400,
body: { error_code: 'UnknownCode' },
})
renderPage()
msw.override('post', instancesUrl, 400, { error_code: 'UnknownCode' })
renderAppAt(formUrl)

fireEvent.click(submitButton())

Expand All @@ -71,38 +47,22 @@ describe('InstanceCreatePage', () => {
expect(window.location.pathname).toEqual(formUrl)
})

it('posts form on submit', async () => {
const mock = fetchMock.post(instancesUrl, 201)
renderPage()

fireEvent.change(screen.getByLabelText('Choose a name'), {
target: { value: 'new-instance' },
})
fireEvent.click(screen.getByLabelText(/6 CPUs/))
fireEvent.click(submitButton())

await waitFor(() =>
expect(lastPostBody(mock)).toEqual({
name: 'new-instance',
description: 'An instance in project: mock-project',
hostname: '',
ncpus: 6,
memory: 25769803776,
})
)
})

it('navigates to project instances page on success', async () => {
const mock = fetchMock.post(instancesUrl, { status: 201, body: instance })
renderPage()
renderAppAt(formUrl)

const instancesPage = `/orgs/${org.name}/projects/${project.name}/instances`
expect(window.location.pathname).not.toEqual(instancesPage)

// TODO: once MSW data layer is in place, uncomment these and assert that
// the name shows up in the list of instances

// fireEvent.change(screen.getByLabelText('Choose a name'), {
// target: { value: 'new-instance' },
// })
// fireEvent.click(screen.getByLabelText(/6 CPUs/))

fireEvent.click(submitButton())

await waitFor(() => expect(mock.called(instancesUrl)).toBeTruthy())
await waitFor(() => expect(mock.done()).toBeTruthy())
await waitFor(() => expect(window.location.pathname).toEqual(instancesPage))
})
})
61 changes: 8 additions & 53 deletions app/pages/__tests__/ProjectCreatePage.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,7 @@
import {
fireEvent,
lastPostBody,
renderAppAt,
screen,
waitFor,
} from 'app/test-utils'
import fetchMock from 'fetch-mock'

import { org, project, projects, sessionMe } from '@oxide/api-mocks'
import { fireEvent, renderAppAt, screen, waitFor } from 'app/test-utils'
import { msw, org, project } from '@oxide/api-mocks'

const projectsUrl = `/api/organizations/${org.name}/projects`
const projectUrl = `${projectsUrl}/${project.name}`
const instancesUrl = `${projectUrl}/instances?limit=10`

const submitButton = () =>
screen.getByRole('button', { name: 'Create project' })
Expand All @@ -24,21 +14,13 @@ function enterName(value: string) {
const formUrl = `/orgs/${org.name}/projects/new`

const renderPage = () => {
// fetch projects list for org layout sidebar on project create
fetchMock.get('/api/session/me', { status: 200, body: sessionMe })
fetchMock.get(projectsUrl, { status: 200, body: projects })
const result = renderAppAt(formUrl)
enterName('valid-name')
return result
}

describe('ProjectCreatePage', () => {
afterEach(() => {
fetchMock.reset()
})

it('disables submit button on submit', async () => {
fetchMock.post(projectsUrl, { status: 201 })
renderPage()

const submit = submitButton()
Expand All @@ -50,9 +32,8 @@ describe('ProjectCreatePage', () => {
})

it('shows message for known error code in project create code map', async () => {
fetchMock.post(projectsUrl, {
status: 400,
body: { error_code: 'ObjectAlreadyExists' },
msw.override('post', projectsUrl, 400, {
error_code: 'ObjectAlreadyExists',
})
renderPage()

Expand All @@ -66,10 +47,7 @@ describe('ProjectCreatePage', () => {
})

it('shows message for known error code in global code map', async () => {
fetchMock.post(projectsUrl, {
status: 403,
body: { error_code: 'Forbidden' },
})
msw.override('post', projectsUrl, 403, { error_code: 'Forbidden' })
renderPage()

fireEvent.click(submitButton())
Expand All @@ -90,10 +68,7 @@ describe('ProjectCreatePage', () => {
})

it('shows generic message for unknown server error', async () => {
fetchMock.post(projectsUrl, {
status: 400,
body: { error_code: 'UnknownCode' },
})
msw.override('post', projectsUrl, 400, { error_code: 'UnknownCode' })
renderPage()

fireEvent.click(submitButton())
Expand All @@ -103,35 +78,15 @@ describe('ProjectCreatePage', () => {
expect(window.location.pathname).toEqual(formUrl)
})

it('posts form on submit', async () => {
const mock = fetchMock.post(projectsUrl, { status: 201 })
renderPage()

fireEvent.click(submitButton())

await waitFor(() =>
expect(lastPostBody(mock)).toEqual({
name: 'valid-name',
description: '',
})
)
})
Copy link
Collaborator Author

@david-crespo david-crespo Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of miss these tests, but I get that they're an implementation detail. The more correct MSW way of doing it is to have MSW actually add the posted thing to a mock database and then assert that it shows up in the UI.

Copy link
Contributor

@just-be-dev just-be-dev Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is where that data library would come in handy.


it('navigates to project instances page on success', async () => {
fetchMock.post(projectsUrl, {
status: 201,
body: project,
})
fetchMock.get(projectUrl, { status: 200 })
// instances fetch after success
fetchMock.get(instancesUrl, { status: 200, body: { items: [] } })

renderPage()
const projectPath = `/orgs/${org.name}/projects/${project.name}/instances`
expect(window.location.pathname).not.toEqual(projectPath)

fireEvent.click(submitButton())

await waitFor(() => expect(window.location.pathname).toEqual(projectPath))

// TODO: navigate to projects page so you can see the project in the list?
})
})
56 changes: 16 additions & 40 deletions app/pages/project/networking/VpcPage/VpcPage.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,17 @@
import {
fireEvent,
lastPostBody,
renderAppAt,
screen,
userEvent,
waitForElementToBeRemoved,
} from 'app/test-utils'
import fetchMock from 'fetch-mock'
import {
org,
project,
vpc,
vpcSubnet,
vpcSubnet2,
vpcSubnets,
} from '@oxide/api-mocks'
import { msw, org, project, vpcSubnet, vpcSubnet2 } from '@oxide/api-mocks'

const vpcUrl = `/api/organizations/${org.name}/projects/${project.name}/vpcs/default`
const subnetsUrl = `${vpcUrl}/subnets`
const getSubnetsUrl = `${subnetsUrl}?limit=10`
const subnetsUrl = `/api/organizations/${org.name}/projects/${project.name}/vpcs/default/subnets`

describe('VpcPage', () => {
describe('subnets tab', () => {
it('creating a subnet works', async () => {
fetchMock.get('/api/session/me', 200)
fetchMock.get(vpcUrl, { status: 200, body: vpc })
fetchMock.getOnce(getSubnetsUrl, { status: 200, body: vpcSubnets })
const postMock = fetchMock.postOnce(subnetsUrl, {
status: 201,
body: vpcSubnet2,
})

renderAppAt('/orgs/mock-org/projects/mock-project/vpcs/default')
screen.getByText('Subnets')

Expand All @@ -52,35 +33,30 @@ describe('VpcPage', () => {
const name = screen.getByRole('textbox', { name: 'Name' })
userEvent.type(name, 'mock-subnet-2')

// override the subnets GET to include both subnets
fetchMock.getOnce(
getSubnetsUrl,
{
status: 200,
body: { items: [vpcSubnet, vpcSubnet2] },
},
{ overwriteRoutes: true }
)
// this is temporary, a workaround for the fact that the mock server
// doesn't have a persistence layer yet
msw.override('get', subnetsUrl, 200, { items: [vpcSubnet, vpcSubnet2] })

// submit the form
fireEvent.click(screen.getByRole('button', { name: 'Create subnet' }))

// wait for modal to close
await waitForElementToBeRemoved(() =>
screen.queryByRole('dialog', { name: 'Create subnet' })
await waitForElementToBeRemoved(
() => screen.queryByRole('dialog', { name: 'Create subnet' }),
// fails in CI without a longer timeout (default 1000). boo
{ timeout: 2000 }
)

// it posted the form
expect(lastPostBody(postMock)).toEqual({
ipv4Block: '1.1.1.2/24',
ipv6Block: null,
name: 'mock-subnet-2',
description: '',
})
// TODO: before, we asserted what body the form posted. MSW strongly
// discourages this because it's testing implementation details, but I
// can't shake the feeling that I want it. But it might feel better after
// the MSW mock is more sophisticated and actually handles a create by
// inserting the thing in the list of subnets. Then our assertion that it
// showed up in the list actually does check what was posted.
Comment on lines +50 to +55
Copy link
Contributor

@just-be-dev just-be-dev Jan 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that testing the body of the post is testing implementation details. Having the data layer plugged in means we could test the results of the post which feels like a more valuable test to me. The important part here is that we don't actually want the test to have internal details about the shape of the form. If we assert against the shape of the body then we're tightly coupling the test to the forms internal representation which would be best avoided.

Copy link
Collaborator Author

@david-crespo david-crespo Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s a good point, and it’s even stronger when you consider more complex inputs than text fields. With a text field, the internal representation of the value is just the string you typed (usually). But with a multi-select the internal representation is more opaque.


// table should refetch and now include second subnet
screen.getByRole('cell', { name: vpcSubnet.identity.name })
screen.getByRole('cell', { name: vpcSubnet2.identity.name })
await screen.findByRole('cell', { name: vpcSubnet2.identity.name })
})
})
})
5 changes: 1 addition & 4 deletions app/routes.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,13 @@

import { matchRoutes } from 'react-router'
import { renderAppAt } from './test-utils'
import fetchMock from 'fetch-mock'

import { projects, sessionMe } from '@oxide/api-mocks'
import { projects } from '@oxide/api-mocks'

import { getRouteConfig } from './routes'

describe('routes', () => {
it('should render successfully', async () => {
fetchMock.get('/api/session/me', { status: 200, body: sessionMe })
fetchMock.get('/api/organizations/maze-war/projects', projects)
const { findAllByText } = renderAppAt('/')
await findAllByText(projects.items[0].name)
})
Expand Down
9 changes: 4 additions & 5 deletions app/test-utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import React from 'react'
import { BrowserRouter } from 'react-router-dom'
import { render } from '@testing-library/react'
import { QueryClient, QueryClientProvider } from 'react-query'
import type { FetchMockStatic } from 'fetch-mock'
import { routes } from './routes'

const queryClient = new QueryClient({
Expand All @@ -13,6 +12,10 @@ const queryClient = new QueryClient({
},
})

// this is necessary to prevent requests left in flight at the end of a test from
// coming back during another test and triggering whatever they would trigger
afterEach(() => queryClient.clear())

const customRender = (ui: React.ReactElement) =>
render(ui, {
wrapper: ({ children }) => (
Expand All @@ -33,10 +36,6 @@ export function renderAppAt(url: string) {
})
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
export const lastPostBody = (mock: FetchMockStatic): any =>
JSON.parse(mock.lastOptions(undefined, 'POST')?.body as unknown as string)

export * from '@testing-library/react'
export { default as userEvent } from '@testing-library/user-event'
export { customRender as render }
Loading