feat(cleanup): automatically cleanup if afterEach is detected #430

kentcdodds · 2019-08-08T21:54:08Z

What:

automatically call cleanup (with async act) if afterEach is detected.

Why:

Closes #428

This effectively means that 90% of users wont even have to know about cleanup at all. Which is a huge improvement.

How:

You can disable this with the RTL_SKIP_CLEANUP environment variable if you so choose, but it's recommended to have cleanup work this way.

Checklist:

Documentation added to the
docs site Let's get this reviewed/merged first, then we can start working on the docs.
Tests
Typescript definitions updated N/A
Ready to be merged

kentcdodds · 2019-08-08T21:54:18Z

cc @threepointone, I'd love your input here.

kentcdodds · 2019-08-08T21:57:45Z

Here's the one issue that I think people will bump up against. If people write their tests like this:

const {getByText} = render(<Counter />)
const counterButton = getByText(/^count/i)

test('the counter is initialized to the initialCount', () => {
  expect(counterButton).toHaveTextContent('0')
})

test('when clicked, the counter increments the click', () => {
  fireEvent.click(counterButton)
  expect(counterButton).toHaveTextContent('1')
})

Or similar, then they'll be in trouble because we'll auto-cleanup for them and their tests will break in a way that's really confusing.

They should definitely not be writing their tests like that, but I know a lot of people do put each assertion in its own it, which is really pointless, but they do it and tests will fail. So we'll need to have a very helpful changelog describing how to use the workaround RTL_SKIP_CLEANUP environment variable and why they should fix their tests to be isolated.

kentcdodds · 2019-08-08T21:58:01Z

FWIW, I have this blog post written already: https://kentcdodds.com/blog/test-isolation-with-react

codecov · 2019-08-08T22:02:21Z

Codecov Report

Merging #430 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #430   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      4    +1     
  Lines          91     95    +4     
  Branches       12     13    +1     
=====================================
+ Hits           91     95    +4

Impacted Files	Coverage Δ
src/pure.js	`100% <100%> (ø)`
src/index.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f17920...c231511. Read the comment docs.

threepointone · 2019-08-08T22:06:52Z

Bit dead today because of the release, so I'll be a while before responding haha. But I don't think you should merge this any time soon, at least not without thinking about it for a day or so. Seems like too little to warrant a major version bump.

kentcdodds · 2019-08-08T22:06:41Z

src/__tests__/act.js

@@ -1,7 +1,5 @@
 import React from 'react'
-import {render, cleanup, fireEvent} from '../'
-
-afterEach(cleanup)


Several of the tests were manually wiring up cleanup even though they didn't need to before (and they certainly don't need to now), so I fixed those.

There a bunch of docs with afterEach(cleanup) too

I made these unrelated polish changes to master so this PR could be slimmer.

alexkrolick · 2019-08-09T05:22:38Z

I know a lot of people do put each assertion in its own it

I suspect that most of these people don't have cleanup configured in most of their tests at all 😬, unless they manually do cleanup in the afterAll (after) hook in each describe(). Easy to miss and fragile.

bcarroll22 · 2019-08-09T14:49:16Z

🤔 I think I'm a fan of this model as a major release. Weighing the tradeoffs personally, this seems like it will save a lot more bugs than it will create. I've seen people get burned multiple times by not importing and running cleanup properly.

If the primary downside is that some test suites will have to be refactored during the upgrade to be better isolated, that seems reasonable.

You can disable this with the RTL_SKIP_CLEANUP environment variable if you so choose, but it's recommended to have cleanup work this way. Closes #428

kentcdodds · 2019-08-09T15:05:29Z

After considering this a lot, I've determined that we should go forward with this change.

The Testing Library family of tools has a primary goal: To enable people to write tests which give them more confidence and are easier to maintain. The kinds of tests that this change could break do not give people more confidence nor are they easy to maintain. And the current requirement to have to wire up cleanup makes using react testing library harder (which is contrary to the goals of the library).

This is worth a breaking change IMO. The "churn" will be a minimum and will only negatively impact people who are not using the library as documented anyway. And for people who have been doing things correctly by wiring up the afterEach(cleanup) properly will be able to upgrade and they don't even have to remove their wiring if they don't want to. It will still work either way.

So in a few hours I will be merging this unless there's good reason to not do so.

DaveStein · 2019-08-09T15:32:36Z

Looks like this would be an auto-solve for testing-library/react-hooks-testing-library#76. I was confused when I looked at your PR, but then saw that cleanup essentially ends up calling ReactDOM.unmountComponentAtNode(container), which is what is exposed simply as unmount to public API.

threepointone · 2019-08-09T15:34:20Z

How will this work with concurrent mode (.createRoot()/.unmount())? or do you not factor that in right now?

threepointone · 2019-08-09T15:35:29Z

src/index.js

+// this ensures that tests run in isolation from each other
+if (typeof afterEach === 'function' && !process.env.RTL_SKIP_CLEANUP) {
+  afterEach(async () => {
+    await asyncAct(async () => {})


Consider - if ther are any hanging promises after a test is finished, that probably means the user hasn't asserted on the actual stable state of the UI. So this would hide a probable bug.

Maybe this is a similar concern with the async act() in cleanup-after-each, ouch. at least there it's explicit tho.

if there are any hanging promises after a test is finished, that probably means the user hasn't asserted on the actual stable state of the UI.

I'm not sure I understand how this change impacts that at all. This change is doing exactly the same thing that cleanup-after-each is doing. It's just wiring it up automatically.

Maybe this shouldn't be an act(), but it should flush promises anyway. So if there are any stray async updates, the warning will fire, and the dev will account for it in their test.

I'm not sure I understand how this change impacts that at all. This change is doing exactly the same thing that cleanup-after-each is doing. It's just wiring it up automatically.

I'm suggesting it should be removed from that too.

kentcdodds · 2019-08-09T15:47:28Z

How will this work with concurrent mode (.createRoot()/.unmount())

I don't know. I guess we don't factor that in right now. Currently you can only ReactDOM.render anyway, so what we have today works with what you can do anyway. When we add concurrent mode rendering then perhaps we'll have to adjust cleanup to manage unmounting those nodes as well I guess (if it's different?).

kentcdodds · 2019-08-09T15:49:52Z

To be clear: all this change does is make it so people don't have to wire up cleanup-after-each themselves (which they should be doing). If they're doing that already, then this change will make 0 impact on them.

So the only people who will experience any bumps in this transition are the people who are using RTL incorrectly.

threepointone · 2019-08-09T15:50:03Z

If you're going all in on a major update, why do it just for this one feature, and so soon, is my major concern. I think you should think deeply on the implications, and at least bounce around some ideas.

Alternate idea for this 'feature': Custom describe/test helpers? You'd have a lot more freedom there too.

kentcdodds · 2019-08-09T15:57:03Z

I definitely don't want custom describe/test helpers. That increases the API surface/intrusiveness significantly and the number of people who wouldn't use those helpers is huge. We'd basically split the community between people who use those helpers and those who don't. I'm sorry Sunil, but I'm pretty firm on this.

kentcdodds · 2019-08-09T15:58:44Z

Also, I'm not as concerned about publishing major version bumps to my libraries. It's just a number, people using the current version get a great library. People who upgrade have very little work to do to upgrade (less than if we batched a ton of changes for a major version change).

I personally feel like most people wont need to make any changes for this major version change, so upgrading to this version will require very minimal work. And if they don't bother upgrading that's fine too.

threepointone · 2019-08-09T15:58:55Z

Sure, it was a suggestion. I'm trying to point out that you're set on a major, then you can rethink stuff beyond this one feature.

If you're set on making this change, go for it, I'll keep my feedback to myself.

kentcdodds · 2019-08-09T16:00:04Z

you're set on a major, then you can rethink stuff beyond this one feature.

That's a good point.

I'm definitely open to suggestions and feedback. I just thought I had already explained my thoughts around your feedback about the custom describe/test helpers.

kentcdodds · 2019-08-09T16:07:32Z

There's one other breaking change that we could potentially include with this one: testing-library/dom-testing-library#314

I'm just worried that this will be a significant change and there's too much inertia behind the debug function that it would be confusing for people 😬 But I could be wrong. I'll make a PR on DTL right now to see what that would be like.

benlesh · 2019-08-09T16:55:28Z

+1 for major version bumps for anything that might even be remotely breaking.

(I'll go back to lurking now)

eps1lon

Really like this being implemented by default.

I'm not a big fan of having side-effects in modules though but I guess we don't have any other choice.

I would like to have a pure import though (e.g. @testing-library/react/pure) instead of the environment variables. Those are pretty iffy if you want to incrementally adopt isolated tests.

eps1lon · 2019-08-09T18:03:30Z

src/index.js

+// if we're running in a test runner that supports afterEach
+// then we'll automatically run cleanup afterEach test
+// this ensures that tests run in isolation from each other
+if (typeof afterEach === 'function' && !process.env.RTL_SKIP_CLEANUP) {


We should guard against process and process.env not being defined for people running their tests in actual browsers.

Co-Authored-By: Adrià Fontcuberta <afontcu@gmail.com>

kentcdodds · 2019-08-09T18:20:36Z

I'm not a big fan of having side-effects in modules

I feel the same way. I'm fine with the pure approach. I'll refactor this to that.

kentcdodds · 2019-08-09T18:23:36Z

Actually, I think we'll support both. Because lots of people who would need to make this migration would probably prefer the env approach.

kentcdodds · 2019-08-09T18:29:02Z

Ok, both mechanisms are supported. So people who don't want this behavior can either set RTL_SKIP_AUTO_CLEANUP to 'true' (or anything for that matter) OR they can import from @testing-library/react/pure.

But 99% of people wont even know this is a thing (or want it) and that's a huge win.

kentcdodds · 2019-08-09T18:30:47Z

Basically, we're changing the default for the more common case. Before, 99% of people had to wire up the cleanup and 1% didn't bother (and their tests were probably not very good). Now, 99% of people don't have to do anything, 0.9% of people will have to figure out a better way to write tests, and 0.1% of people will have to figure out how to skip out of the auto-cleanup stuff.

Those numbers are made up. It's probably even more favorable to be honest.

kentcdodds · 2019-08-09T19:35:52Z

Ok, this is going to go in the next few minutes.

You can disable this with the RTL_SKIP_CLEANUP environment variable if you so choose, but it's recommended to have cleanup work this way. Closes #428 BREAKING CHANGE: If your tests were not isolated before (and you referenced the same component between tests) then this change will break your tests. You should [keep your tests isolated](https://kentcdodds.com/blog/test-isolation-with-react), but if you're unable/unwilling to do that right away, then you can either run your tests with the environment variable `RTL_SKIP_AUTO_CLEANUP` set to `true` or import `@testing-library/react/pure` instead of `@testing-library/react`.

BREAKING CHANGE: If you were using `debugDOM` before, use `prettyDOM` instead. Note that `debugDOM` is different from `debug` which you get back from `render`. That is unchanged.

kentcdodds · 2019-08-09T19:50:07Z

I'm going to upgrade @testing-library/dom with this PR as well. So we'll get a single release for both breaking changes (because we re-export everything from @testing-library/dom).

kentcdodds · 2019-08-09T19:57:22Z

Here it goooooes!

kentcdodds · 2019-08-09T20:00:06Z

🎉 This PR is included in version 9.0.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

reyronald · 2019-08-10T11:38:09Z

Wow this is great Kent, thank you so much! Totally in favor of this change.

Just wanted to chime in to say that I think the affected users will be higher than 1% or 0.1%. I do contractor work so I see many different codebases from different teams and companies often and the style you described in #430 (comment) is far more common than one would think. I suspect it might be because of the micro-optimization tendencies and that low-performance phobia that some developers have that they think that re-using the container/wrapper element across several tests will make their test suites faster. Or maybe they don't feel comfortable rendering the components each time on each tests because it doesn't feel DRY ?

Regardless, I will not make up any numbers or statistics, and like I said I totally agree with the change, just wanted to provide a different perspective and expectations based on my personal anecdotal experience, just because I can think of many repos that I've worked with this year alone that I know will have test failures.

kentcdodds · 2019-08-10T13:42:47Z

You're probably right @reyronald. Hopefully this change will help people rethink their testing practices.

That said, I decided to make disabling this behavior as easy as enabling the original behavior was: #435

See testing-library/react-testing-library#430

kentcdodds requested review from Gpx and alexkrolick August 8, 2019 21:54

kentcdodds mentioned this pull request Aug 8, 2019

Have render call cleanup #428

Closed

kentcdodds force-pushed the pr/auto-cleanup branch from 55aa167 to 65ff32b Compare August 8, 2019 21:59

mpeyper mentioned this pull request Aug 8, 2019

Pr/cleanup renderhooks testing-library/react-hooks-testing-library#106

Closed

5 tasks

kentcdodds force-pushed the pr/auto-cleanup branch from 65ff32b to a67a3fc Compare August 8, 2019 22:08

kentcdodds commented Aug 8, 2019

View reviewed changes

kentcdodds force-pushed the pr/auto-cleanup branch from a67a3fc to 31b8dfc Compare August 9, 2019 14:59

feat(cleanup): automatically cleanup if afterEach is detected

6862dd1

You can disable this with the RTL_SKIP_CLEANUP environment variable if you so choose, but it's recommended to have cleanup work this way. Closes #428

kentcdodds force-pushed the pr/auto-cleanup branch from 31b8dfc to 6862dd1 Compare August 9, 2019 15:00

threepointone reviewed Aug 9, 2019

View reviewed changes

eps1lon reviewed Aug 9, 2019

View reviewed changes

Update src/__tests__/auto-cleanup-skip.js

c5f9a12

Co-Authored-By: Adrià Fontcuberta <afontcu@gmail.com>

kentcdodds force-pushed the pr/auto-cleanup branch 2 times, most recently from 405f61a to e0deaf7 Compare August 9, 2019 18:28

Kent C. Dodds added 2 commits August 9, 2019 13:46

feat(deps): update @testing-library/dom to 6.0.0

c231511

BREAKING CHANGE: If you were using `debugDOM` before, use `prettyDOM` instead. Note that `debugDOM` is different from `debug` which you get back from `render`. That is unchanged.

kentcdodds force-pushed the pr/auto-cleanup branch from e0deaf7 to c231511 Compare August 9, 2019 19:49

kentcdodds merged commit 1dd6544 into master Aug 9, 2019

kentcdodds deleted the pr/auto-cleanup branch August 9, 2019 19:57

kentcdodds added the released label Aug 9, 2019

afontcu mentioned this pull request Aug 10, 2019

Automatically cleanup if afterEach is detected testing-library/vue-testing-library#77

Closed

kentcdodds mentioned this pull request Aug 10, 2019

feat: add dont-cleanup-after-each.js #435

Merged

4 tasks

kripod added a commit to kripod/react-hooks that referenced this pull request Aug 11, 2019

Remove use of obsolete test cleanup method

111d52b

See testing-library/react-testing-library#430

zainfathoni mentioned this pull request Aug 23, 2019

Integrate jest-axe to enable a11y assertions in unit & integration tests zainfathoni/obel#70

Merged

balazsorban44 mentioned this pull request Sep 7, 2019

Unit tests for components chakra-ui/chakra-ui#7

Closed

kripod added a commit to kripod/react-hooks that referenced this pull request Oct 11, 2019

Remove use of obsolete test cleanup method

4981d6f

See testing-library/react-testing-library#430

mpeyper mentioned this pull request Oct 14, 2019

Global cleanup testing-library/react-hooks-testing-library#199

Merged

3 tasks

newtonry mentioned this pull request Nov 6, 2019

docs: add comment about 'cleanup' in react-testing-library example jestjs/jest#9144

Merged

oliverjam mentioned this pull request Nov 7, 2019

Cleanup done automatically for Jest/Mocha/Jasmine (according to docs) oliverjam/learn-react-testing#12

Closed

eps1lon mentioned this pull request Jan 21, 2020

Re-render renders component from scratch when used in separate tests #573

Closed

feat(cleanup): automatically cleanup if afterEach is detected #430

feat(cleanup): automatically cleanup if afterEach is detected #430

Conversation

kentcdodds commented Aug 8, 2019

kentcdodds commented Aug 8, 2019

kentcdodds commented Aug 8, 2019

kentcdodds commented Aug 8, 2019

codecov bot commented Aug 8, 2019 • edited

Codecov Report

threepointone commented Aug 8, 2019

kentcdodds Aug 8, 2019

Choose a reason for hiding this comment

alexkrolick Aug 9, 2019

Choose a reason for hiding this comment

kentcdodds Aug 9, 2019

Choose a reason for hiding this comment

alexkrolick commented Aug 9, 2019

bcarroll22 commented Aug 9, 2019

kentcdodds commented Aug 9, 2019

DaveStein commented Aug 9, 2019

threepointone commented Aug 9, 2019

threepointone Aug 9, 2019

Choose a reason for hiding this comment

threepointone Aug 9, 2019 • edited

Choose a reason for hiding this comment

kentcdodds Aug 9, 2019 • edited

Choose a reason for hiding this comment

threepointone Aug 9, 2019

Choose a reason for hiding this comment

threepointone Aug 9, 2019

Choose a reason for hiding this comment

kentcdodds commented Aug 9, 2019

kentcdodds commented Aug 9, 2019

threepointone commented Aug 9, 2019

kentcdodds commented Aug 9, 2019

kentcdodds commented Aug 9, 2019 • edited

threepointone commented Aug 9, 2019

kentcdodds commented Aug 9, 2019

kentcdodds commented Aug 9, 2019

benlesh commented Aug 9, 2019

eps1lon left a comment

Choose a reason for hiding this comment

eps1lon Aug 9, 2019

Choose a reason for hiding this comment

kentcdodds commented Aug 9, 2019

kentcdodds commented Aug 9, 2019

kentcdodds commented Aug 9, 2019

kentcdodds commented Aug 9, 2019

kentcdodds commented Aug 9, 2019

kentcdodds commented Aug 9, 2019

kentcdodds commented Aug 9, 2019

kentcdodds commented Aug 9, 2019

reyronald commented Aug 10, 2019

kentcdodds commented Aug 10, 2019

codecov bot commented Aug 8, 2019 •

edited

threepointone Aug 9, 2019 •

edited

kentcdodds Aug 9, 2019 •

edited

kentcdodds commented Aug 9, 2019 •

edited