Skip to content
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

Global cleanup #199

Merged
merged 4 commits into from
Oct 16, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 21 additions & 0 deletions docs/api-reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ route: '/reference/api'

- [`renderHook`](/reference/api#renderhook)
- [`act`](/reference/api#act)
- [`cleanup`](/reference/api#cleanup)

---

Expand Down Expand Up @@ -102,3 +103,23 @@ A function to unmount the test component. This is commonly used to trigger clean

This is the same [`act` function](https://reactjs.org/docs/test-utils.html#act) that is exported by
`react-test-renderer`.

---

## `cleanup`

```js
function cleanup: Promise<void>
```

Unmounts any rendered hooks rendered with `renderHook`, ensuring all effects have been flushed.

> Please note that this is done automatically if the testing framework you're using supports the
> `afterEach` global (like mocha, Jest, and Jasmine). If not, you will need to do manual cleanups
> after each test.
Comment on lines +117 to +119
Copy link

Choose a reason for hiding this comment

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

❓ Do you happen to have any reference or example of a testing library auto adding a global afterEach?

I think I like this, i'm just curious about community best practices and such.

Copy link
Member Author

Choose a reason for hiding this comment

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

The registration of this is lifted almost verbatim from @testing-library/react-testing-library#430

I don't think this is a commonly used pattern (yet).

>
> Setting the `RHTL_SKIP_AUTO_CLEANUP` environment variable to `true` before the
> `@testing-library/react-hooks` is imported will disable this feature.

The `cleanup` function should be called after each test to ensure that previously rendered hooks
will not have any unintended side-effects on the following tests.
26 changes: 26 additions & 0 deletions src/cleanup.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { act } from 'react-test-renderer'

let cleanupCallbacks = []

async function cleanup() {
await act(async () => {})
cleanupCallbacks.forEach((cb) => cb())
cleanupCallbacks = []
}

function addCleanup(callback) {
cleanupCallbacks.push(callback)
}

function removeCleanup(callback) {
cleanupCallbacks = cleanupCallbacks.filter((cb) => cb !== callback)
}

// Automatically registers cleanup in supported testing frameworks
if (typeof afterEach === 'function' && !process.env.RHTL_SKIP_AUTO_CLEANUP) {
Copy link

@hartzis hartzis Oct 14, 2019

Choose a reason for hiding this comment

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

📓 process.env.RHTL_SKIP_AUTO_CLEANUP only needs to be "truthy" and not true to skip.

Do we want this to explicitly be the string "true"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Again, this is implemented as per @testing-library/react-testing-library

Copy link

Choose a reason for hiding this comment

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

Cool. Nice to see sharing the pattern from RTL for dev familiarity.

afterEach(async () => {
await cleanup()
})
}

export { cleanup, addCleanup, removeCleanup }
18 changes: 12 additions & 6 deletions src/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React, { Suspense } from 'react'
import { act, create } from 'react-test-renderer'
import { cleanup, addCleanup, removeCleanup } from './cleanup'

function TestHook({ callback, hookProps, onError, children }) {
try {
Expand Down Expand Up @@ -73,6 +74,15 @@ function renderHook(callback, { initialProps, wrapper } = {}) {
})
const { unmount, update } = testRenderer

function unmountHook() {
act(() => {
removeCleanup(unmountHook)
unmount()
})
}

addCleanup(unmountHook)

let waitingForNextUpdate = null
const resolveOnNextUpdate = (resolve) => {
addResolver((...args) => {
Expand All @@ -93,12 +103,8 @@ function renderHook(callback, { initialProps, wrapper } = {}) {
update(toRender())
})
},
unmount: () => {
act(() => {
unmount()
})
}
unmount: unmountHook
}
}

export { renderHook, act }
export { renderHook, cleanup, act }
28 changes: 28 additions & 0 deletions test/autoCleanup.disabled.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { useEffect } from 'react'

// This verifies that if RHTL_SKIP_AUTO_CLEANUP is set
// then we DON'T auto-wire up the afterEach for folks
describe('skip auto cleanup (disabled) tests', () => {
let cleanupCalled = false
let renderHook

beforeAll(() => {
process.env.RHTL_SKIP_AUTO_CLEANUP = 'true'
renderHook = require('src').renderHook
})

test('first', () => {
const hookWithCleanup = () => {
useEffect(() => {
return () => {
cleanupCalled = true
}
})
}
renderHook(() => hookWithCleanup())
})

test('second', () => {
expect(cleanupCalled).toBe(false)
})
})
28 changes: 28 additions & 0 deletions test/autoCleanup.noAfterEach.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { useEffect } from 'react'

// This verifies that if RHTL_SKIP_AUTO_CLEANUP is set
// then we DON'T auto-wire up the afterEach for folks
describe('skip auto cleanup (no afterEach) tests', () => {
let cleanupCalled = false
let renderHook

beforeAll(() => {
afterEach = false
renderHook = require('src').renderHook
})

test('first', () => {
const hookWithCleanup = () => {
useEffect(() => {
return () => {
cleanupCalled = true
}
})
}
renderHook(() => hookWithCleanup())
})

test('second', () => {
expect(cleanupCalled).toBe(false)
Copy link

Choose a reason for hiding this comment

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

❓ Is this test supposed to check true, expect(cleanupCalled).toBe(true)?

It appears to be a copy of the above test, autoCleanup.disabled.test.js, but without the process.env setup.

You may need to clear the require cache and reset the process.env before running?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. In the beforeAll am overriding afterEach to not be a function to test the case where the testing framework does not support it. I was very worried about this, but I tested thoroughly and jest brings it back for the next test.

Happy to hear ideas for better ways to test this case.

Copy link

Choose a reason for hiding this comment

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

welp jest is pretty damn cool.

})
})
24 changes: 24 additions & 0 deletions test/autoCleanup.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { useEffect } from 'react'
import { renderHook } from 'src'

// This verifies that by importing RHTL in an
// environment which supports afterEach (like jest)
// we'll get automatic cleanup between tests.
describe('auto cleanup tests', () => {
let cleanupCalled = false

test('first', () => {
const hookWithCleanup = () => {
useEffect(() => {
return () => {
cleanupCalled = true
}
})
}
renderHook(() => hookWithCleanup())
})

test('second', () => {
expect(cleanupCalled).toBe(true)
})
})
41 changes: 41 additions & 0 deletions test/cleanup.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { useEffect } from 'react'
import { renderHook, cleanup } from 'src'

describe('cleanup tests', () => {
test('should flush effects on cleanup', async () => {
let cleanupCalled = false

const hookWithCleanup = () => {
useEffect(() => {
return () => {
cleanupCalled = true
}
})
}

renderHook(() => hookWithCleanup())

await cleanup()

expect(cleanupCalled).toBe(true)
})

test('should cleanup all rendered hooks', async () => {
let cleanupCalled = []
const hookWithCleanup = (id) => {
useEffect(() => {
return () => {
cleanupCalled[id] = true
}
})
}

renderHook(() => hookWithCleanup(1))
renderHook(() => hookWithCleanup(2))

await cleanup()

expect(cleanupCalled[1]).toBe(true)
expect(cleanupCalled[2]).toBe(true)
})
})