Use react-test-renderer instead react-testing-library #40

sgrishchenko · 2019-04-14T19:09:20Z

What:

Feature:
When we test hooks we do not use DOM. Therefore, we can use a react-test-renderer and do not need to clean anything.

Why:

This simplifies the library API.
This reduces the number of dependencies.

How:

Just use react-test-renderer instead react-testing-library

Checklist:

codecov-io · 2019-04-14T19:11:01Z

Codecov Report

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

@@          Coverage Diff          @@
##           master    #40   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          37     44    +7     
  Branches        3      4    +1     
=====================================
+ Hits           37     44    +7

Impacted Files	Coverage Δ
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 c6f376d...9b4f2b5. Read the comment docs.

mpeyper · 2019-04-16T11:20:44Z

Hi @sgrishchenko,

Thanks for taking the time to submit this PR. It's an interesting point you've raised here (i.e. "do we need a dom?") and not one I've put much thought into yet.

At a quick glance, the changes look ok. I'm struggling to think of any particular reason why we need to use react-testing-library, other than it was just pretty cool that it was built on top of it.

I've nver really used react-test-renderer directly myself, having started with using Enzyme to test React components to using react-testing-library, so I'm not really sure what, if any, benefits there are to using one over the other (other than the reason you mentioned in the original post).

The biggest downside I see is that react-testing-library does some of the heavy lifting around act providing wrappers and things that now has to be handled within this library. Your changes show that the amount of work involved isn't huge, but it is something that we will have to maintain and update with future React releases. As react-testing-library is insanely popular, any update are likely to be done there before there even on my radar, so maintaining compatability with future react releases will likely be as simple as bumping the react-testing-library version. This isn't a deal breaker for this change, but is something worth considering.

The other aspect that appears to be a driver for this change is not having to call cleanup. I have yet to see a test actually fail because cleanup was not called, but it was included in our API because you should moreso than you must. If that's a deal breaker for you using this library as-is, I suggest leaving it out and see if it causes you any issues.

As for the additional dependency, I'm not too concerned about having react-testing-library as an extra dependency, especially given:

It is a testing library, so there's not bundle size consideration to worry about

there is an install size concern, it's just not something I usually worry about

Many projects will already have it as a dependency

I'm happy to have the discussion here as to the pros and cons of this change, and I will continue to consider the implications myself. I have a work trip over the next few days and then Easter weekend (which has various public holidays attached in my country), so I may not get back here for a while, but please feel free to leave comments and I'll try to check in periodically.

mpeyper · 2019-04-16T11:30:33Z

Sorry @sgrishchenko, I merged another PR which has caused a conflict in this one that will need be be resolved before it can be merged. Apologies for the extra effort.

sgrishchenko · 2019-04-16T19:54:19Z

As far as I understand, the main idea of the react-testing-library is to use native css-celesters for testing (under the hood). This allows you to build your tests as plausibly as possible, as if you were testing your application in a browser. But this approach imposes a restriction - you need to use the global document object. This is the reason why cleaning is required. Each time you mount a component, the react-testing-library creates a node in the document (mountedContainers is simple js Set):

  if (!container) {
    // default to document.body instead of documentElement to avoid output of potentially-large
    // head elements (such as JSS style blocks) in debug output
    baseElement = document.body
    container = document.body.appendChild(document.createElement('div'))
  }
  // we'll add it to the mounted containers regardless of whether it's actually
  // added to document.body so the cleanup method works regardless of whether
  // they're passing us a custom container or not.
  mountedContainers.add(container)

and clenup mountedContainers after each test (all code copy-pasted from react-testing-library):

const mountedContainers = new Set()

// ...

function cleanup() {
  mountedContainers.forEach(cleanupAtContainer)
}

react-testing-library use ReactDOM renderer for it and for using react-testing-library you need have jsdom environment
react-test-renderer is just an alternative render (like react-native), using it you get all the same virtual house (not real). Using it, all operations are performed in memory. You do not need to have a global state anymore. In addition, you do not need to set up any test environment (like jsdom), which will allow more users to use your library.
react-testing-library is the official React package. This means that it is well supported and you will receive updates more quickly than from react-testing-library (because the library itself depends from React).
About act wrappers: all that makes the library is a bunch of garbage on the provision of polifiling if you have an old version of React. Now React delivers the official act and all this is no longer necessary.

Recap:

no clenup - it will works because react-test-renderer dont use document
maintaining - should be simpler because we use React insted using library that used React
act wrapper - just legacy and polifiling

I hope I have given sufficiently strong arguments to prove that there is no inexpedience in the react-testing-library. If users need DOM, they can always use the react-testing-library directly.

…nderer # Conflicts: # src/index.js

mpeyper · 2019-04-18T01:36:13Z

This makes a lot of sense, but I think you misunderstood my point about ongoing maintenance cost.

For example, the line to actually render the component in your code looks like

   let testRenderer
  act(() => {
    testRenderer = create(toRender())
  })
  const { unmount, update } = testRenderer

wheras it used to be

const { unmount, rerender: rerenderComponent } = render(toRender(), options)

The fact that we must now wrap the call in act ourselves, rather than inheriting it from react-testing-library means that if the semantics/syntax around act ever changes, we would need to have to make those changes here, whereas it's likely react-testing-library would have already been updated, so we just need to update our version, assuming it wasn't just picked up by semver for our users.

In the grand scheme of things, these cases are going to be very rare and the work required will likely not be large, so I don't think it's a good reason not to move forward with this change, as I think the benefits you described vastly outweights this negagative (which is the only one I've been able to think of).

The only additional thing to update is in the README where we have a bit of verbage that says:

The react-hooks-testing-library is built on top of the wonderful react-testing-library...

which would not be incorrect. I'd like it to still reference the inspiration this library took from the api and values of react-testing-library, but we cannot claim to be built on top of it anymore. Perhaps something along the lines of:

The react-hooks-testing-library allows you to create a simple test harness for React hooks that handles running them within the body of a function component, as well as providing various useful utility functions for updating the inputs and retrieving the outputs of your amazing custom hook.

Similarly to react-testing-library, which this library draws much of it's inspiration from, it aims to provide a testing experience as close as possible to natively using your hook from within a real component.

And of course, please add yourself as a contributor (code, test and ideas... feel free to suggest your own list if you think these don't fit or you feel something else is applicable - options).

…nderer # Conflicts: # .all-contributorsrc # README.md # package.json

sgrishchenko · 2019-04-20T16:46:00Z

I corrected the documentation, added myself to the list of contributors, and resolved conflicts. Now pull request is in an updated state.

mpeyper · 2019-04-21T10:43:43Z

Sweet. Merging this. I'll get a major bump out for this soon.

bcarroll22 · 2019-04-28T05:01:22Z

I know this issue is closed, just wanted to say that this is amazing 👍

Because of this PR I can now leverage react-hooks-testing-library in native-testing-library as well 💯 thanks so much for putting in this work!

mpeyper · 2019-04-28T07:47:58Z

That's great! I didn't even think of that benefit when reviewing this.

mpeyper · 2019-04-28T13:52:22Z

@bcarroll22, I just realized that you're actually a maintainer of native-testing-library and not just a user of both.

For my own curiosity, are you planning on essentially just re-exporting renderHook and act from this library, putting a layer around it, or removing your implementation all together and just link to us in your docs ala react-testing-library?

As someone who has played around with the API a bit, I'd also be very curious to get your opinions on #56.

bcarroll22 · 2019-04-28T14:27:15Z

After seeing this is merged I intend to just point to this library from the docs and remove our whole implementation. It’s already done as part of the 3.0.0 pull request that’s currently open, so I’m really excited about it! There’s no need for me to put a wrapper around it at all, this fully works now for us.

I’ll take a look at 56 as well!

kentcdodds · 2019-04-29T01:58:05Z

This is fantastic

use react-test-renderer instead react-testing-library

c223e6e

unmount wrapped to act, docs updated

bfa2bed

sgrishchenko added 2 commits April 16, 2019 22:57

Merge remote-tracking branch 'upstream/master' into use-react-test-re…

67ac9bd

…nderer # Conflicts: # src/index.js

fix tests after merge

aeadb01

sgrishchenko added 2 commits April 20, 2019 18:33

fix docs and run all-contributors

19617af

Merge remote-tracking branch 'upstream/master' into use-react-test-re…

9b4f2b5

…nderer # Conflicts: # .all-contributorsrc # README.md # package.json

mpeyper merged commit a31fa5d into testing-library:master Apr 21, 2019

This was referenced Apr 24, 2019

Improve documentation #19

Closed

Add docs to testing-library-docs #15

Open

Updated react hooks example page to match react-hooks-testing-library's documentation testing-library/testing-library-docs#64

Open

mpeyper mentioned this pull request Sep 15, 2019

Warning about using different versions of act() when wait()ing on an async effect #173

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use react-test-renderer instead react-testing-library #40

Use react-test-renderer instead react-testing-library #40

sgrishchenko commented Apr 14, 2019 •

edited

Loading

codecov-io commented Apr 14, 2019 •

edited

Loading

mpeyper commented Apr 16, 2019

mpeyper commented Apr 16, 2019

sgrishchenko commented Apr 16, 2019 •

edited

Loading

mpeyper commented Apr 18, 2019 •

edited

Loading

sgrishchenko commented Apr 20, 2019

mpeyper commented Apr 21, 2019

bcarroll22 commented Apr 28, 2019

mpeyper commented Apr 28, 2019

mpeyper commented Apr 28, 2019 •

edited

Loading

bcarroll22 commented Apr 28, 2019

kentcdodds commented Apr 29, 2019

Use react-test-renderer instead react-testing-library #40

Use react-test-renderer instead react-testing-library #40

Conversation

sgrishchenko commented Apr 14, 2019 • edited Loading

codecov-io commented Apr 14, 2019 • edited Loading

Codecov Report

mpeyper commented Apr 16, 2019

mpeyper commented Apr 16, 2019

sgrishchenko commented Apr 16, 2019 • edited Loading

mpeyper commented Apr 18, 2019 • edited Loading

sgrishchenko commented Apr 20, 2019

mpeyper commented Apr 21, 2019

bcarroll22 commented Apr 28, 2019

mpeyper commented Apr 28, 2019

mpeyper commented Apr 28, 2019 • edited Loading

bcarroll22 commented Apr 28, 2019

kentcdodds commented Apr 29, 2019

sgrishchenko commented Apr 14, 2019 •

edited

Loading

codecov-io commented Apr 14, 2019 •

edited

Loading

sgrishchenko commented Apr 16, 2019 •

edited

Loading

mpeyper commented Apr 18, 2019 •

edited

Loading

mpeyper commented Apr 28, 2019 •

edited

Loading