Skip to content

Add waitForNextUpdate promise for testing async updates #11

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

Merged
merged 4 commits into from
Mar 14, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .babelrc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"@babel/react"
],
"plugins": [
"@babel/plugin-transform-runtime",
"@babel/proposal-object-rest-spread",
["module-resolver", { "alias": { "src": "./src" } }],
"@babel/transform-modules-commonjs"
Expand Down
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,9 @@ const useTheme = (initialTheme) => {
}
return useMemo(() => ({ ...themes[theme], toggleTheme }), [theme])
}
```

```js
// useTheme.test.js
import { renderHook, cleanup, act } from 'react-hooks-testing-library'

Expand Down Expand Up @@ -152,6 +154,7 @@ Renders a test component that will call the provided `callback`, including any h

- `result` (`object`)
- `current` (`any`) - the return value of the `callback` function
- `waitForNextUpdate` (`function`) - returns a `Promise` that resolves the next time the hook renders, commonly when state is updated as the result of a asynchronous action.
- `rerender` (`function([newProps])`) - function to rerender the test component including any hooks called in the `callback` function. If `newProps` are passed, the will replace the `initialProps` passed the the `callback` function for future renders.
- `unmount` (`function()`) - function to unmount the test component, commonly used to trigger cleanup effects for `useEffect` hooks.

Expand Down
70 changes: 15 additions & 55 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 2 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,15 @@
"contributors:add": "all-contributors add"
},
"dependencies": {
"@babel/runtime": "^7.3.4",
"react-testing-library": "^6.0.0"
},
"devDependencies": {
"@babel/cli": "^7.2.3",
"@babel/core": "^7.3.4",
"@babel/plugin-proposal-object-rest-spread": "^7.3.4",
"@babel/plugin-transform-modules-commonjs": "^7.2.0",
"@babel/plugin-transform-runtime": "^7.3.4",
"@babel/preset-env": "^7.3.4",
"@babel/preset-react": "^7.0.0",
"@types/react": "^16.8.5",
Expand All @@ -44,16 +46,13 @@
"eslint": "^5.14.1",
"eslint-config-prettier": "^4.0.0",
"eslint-plugin-prettier": "^3.0.1",
"fetch-mock": "^7.3.1",
"husky": "^1.3.1",
"jest": "^24.1.0",
"lint-staged": "^8.1.4",
"node-fetch": "^2.3.0",
"prettier": "^1.16.4",
"prettier-eslint": "^8.8.2",
"prettier-eslint-cli": "^4.7.1",
"react": "^16.8.3",
"react-async": "^5.1.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops! these must have snuck into master when I was updating the README a few days ago. Nonetheless, they are not required.

"react-dom": "^16.8.3",
"typescript": "^3.3.3333",
"typings-tester": "^0.3.2"
Expand Down
7 changes: 7 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,17 @@ function TestHook({ callback, hookProps, children }) {
function renderHook(callback, { initialProps, ...options } = {}) {
const result = { current: null }
const hookProps = { current: initialProps }
const resolvers = []
const waitForNextUpdate = () =>
new Promise((resolve) => {
resolvers.push(resolve)
})

const toRender = () => (
<TestHook callback={callback} hookProps={hookProps.current}>
{(res) => {
result.current = res
resolvers.splice(0, resolvers.length).forEach((resolve) => resolve())
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we pass res to resolve? Like this resolvers.splice(0, resolvers.length).forEach((resolve) => resolve(res))
Would it allow us to rename nextUpdate to nextValue and do assertions directly on it?
Like expect(async nextValue()).toEqual('Bob')

Copy link
Member Author

Choose a reason for hiding this comment

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

I did start out with that, but decided that, as there is already a way of accessing the value, it might be confusing to use different variables before and after the await fold

test('something', async () => {
  const { result, nextValue } = renderHook(() => useSomeHook())

  expect(result.current).toBe({/* ... */})

  expect(await nextValue()).toBe({/* ... */})
})

It also falls down when you want to assert a nexted value

test('something', async () => {
  const { result, nextValue } = renderHook(() => useSomeHook())

  expect(result.current.someProp).toBe({/* ... */})

  // doesn't work
  // expect(await nextValue().someProp).toBe({/* ... */})

  const next = await nextValue()
  expect(next.someProp).toBe({/* ... */})
})

or multiple assertions

test('something', async () => {
  const { result, nextValue } = renderHook(() => useSomeHook())

  expect(result.current).not.toContain({/* ... */})
  expect(result.current).toHaveLength(2)

  // fine
  // expect(await nextValue()).toContain({/* ... */})
  // hangs until the test fails
  // expect(await nextValue()).toHaveLength(3)

  const next = await nextValue()
  expect(next).toContain({/* ... */})
  expect(next).toHaveLength(3)
})

It also raises questions about whether it should fire if the result.current is set to the same value as it was already set to. Consider a hook that has some internal state that updated asyncronously, triggering a side effect , but the hook returns a static result or no result at all itself (e.g. a useRedux hook that returns the static store, but a dispatch triggers some internal state to be updated). If it didnt trigger on every render, this would once again be harder to test.

If we wanted to go this way, I think result.nextValue() feels better to me for what it represents, but right now I still feel as the nextUpdate approach in this PR is more consistent, cleaner in more (common, in my opinion) use cases and requires fewer additional variables in complex tests.

Copy link
Contributor

@ab18556 ab18556 Mar 12, 2019

Choose a reason for hiding this comment

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

I do agree with you. Anyway, we can access the current result right after we awaited so it doesn't really matter.

About the method's name, I am wondering if it wouldn't be better to have something like the async methods in react-testing-library.

I am talking about the wait, waitForElement and waitForDomChange methods.

Maybe a name like waitForNextUpdate or waitForNextResult would provide a better description?

Copy link
Member Author

@mpeyper mpeyper Mar 13, 2019

Choose a reason for hiding this comment

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

waitForNextUpdate

I like that. waitForNextResult is fine but there are legitimate cases where a custom hook may not have a result at all (i.e. no return value), but waitForNextUpdate covers both nicely.

@ab18556 if you want to submit a PR adding yourself as a contributor (review, ideas, others?), I'll happily merge it.

Copy link
Contributor

@ab18556 ab18556 Mar 14, 2019

Choose a reason for hiding this comment

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

I don't feel i did much, but since it was my first contribution to a public project, I am happy to do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

You had more input on this PR than anyone else, and as a library author and maintainer, having anyone actually challenging and validating your work is invaluable and a fantastic contribution you can make (second only to updating the documentation 😉).

}}
</TestHook>
)
Expand All @@ -22,6 +28,7 @@ function renderHook(callback, { initialProps, ...options } = {}) {

return {
result,
waitForNextUpdate,
unmount,
rerender: (newProps = hookProps.current) => {
hookProps.current = newProps
Expand Down
60 changes: 60 additions & 0 deletions test/asyncHook.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import { useState, useEffect } from 'react'
import { renderHook, cleanup } from 'src'

describe('async hook tests', () => {
const getSomeName = () => Promise.resolve('Betty')

const useName = (prefix) => {
const [name, setName] = useState('nobody')

useEffect(() => {
getSomeName().then((theName) => {
setName(prefix ? `${prefix} ${theName}` : theName)
})
}, [prefix])

return name
}

afterEach(cleanup)

test('should wait for next update', async () => {
const { result, waitForNextUpdate } = renderHook(() => useName())

expect(result.current).toBe('nobody')

await waitForNextUpdate()

expect(result.current).toBe('Betty')
})

test('should wait for multiple updates', async () => {
const { result, waitForNextUpdate, rerender } = renderHook(({ prefix }) => useName(prefix), {
initialProps: { prefix: 'Mrs.' }
})

expect(result.current).toBe('nobody')

await waitForNextUpdate()

expect(result.current).toBe('Mrs. Betty')

rerender({ prefix: 'Ms.' })

await waitForNextUpdate()

expect(result.current).toBe('Ms. Betty')
})

test('should resolve all when updating', async () => {
const { result, waitForNextUpdate } = renderHook(({ prefix }) => useName(prefix), {
initialProps: { prefix: 'Mrs.' }
})

expect(result.current).toBe('nobody')

await Promise.all([waitForNextUpdate(), waitForNextUpdate(), waitForNextUpdate()])

expect(result.current).toBe('Mrs. Betty')
})
})
9 changes: 9 additions & 0 deletions test/typescript/renderHook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,12 @@ function checkTypesWhenHookReturnsVoid() {
const _unmount: () => boolean = unmount
const _rerender: () => void = rerender
}

async function checkTypesForWaitForNextUpdate() {
const { waitForNextUpdate } = renderHook(() => {})

await waitForNextUpdate()

// check type
const _waitForNextUpdate: () => Promise<void> = waitForNextUpdate
}
1 change: 1 addition & 0 deletions typings/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export function renderHook<P, R>(
readonly result: {
current: R
}
readonly waitForNextUpdate: () => Promise<void>
readonly unmount: RenderResult['unmount']
readonly rerender: (hookProps?: P) => void
}
Expand Down