Skip to content

Add utility for testing custom hooks #261

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

Closed
kentcdodds opened this issue Jan 7, 2019 · 20 comments
Closed

Add utility for testing custom hooks #261

kentcdodds opened this issue Jan 7, 2019 · 20 comments

Comments

@kentcdodds
Copy link
Member

Describe the feature you'd like:

I think it would be great to add a utility to make testing custom hooks easier. Right now, I recommend making a render prop component out of the hook and testing that component. I think we could do the same from within react-testing-library.

Suggested implementation:

Here's a working implementation/example

import React, {useState} from 'react'
import {render, cleanup} from 'react-testing-library'

afterEach(cleanup)

function useCounter({initialCount = 0, step = 1} = {}) {
  const [count, setCount] = useState(initialCount)
  const increment = () => setCount(c => c + step)
  const decrement = () => setCount(c => c - step)
  return {count, increment, decrement}
}

function testHook(useHook, props) {
  const RenderProp = ({children, ...rest}) => children(useHook(rest))
  const returnVal = {}
  render(
    <RenderProp {...props}>
      {val => {
        // may need some special treatment if the
        // return value is not an object of values...
        Object.assign(returnVal, val)
        return null
      }}
    </RenderProp>,
  )
  return returnVal
}


// how to use this:
test('renders counter', () => {
  const data = testHook(useCounter)
  data.increment()
  expect(data.count).toBe(1)
  data.decrement()
  expect(data.count).toBe(0)
})

Describe alternatives you've considered:

  • Not providing the utility at all.
  • Calling it renderHook rather than testHook, or just about anything else. I'm not sure I like testHook...

Teachability, Documentation, Adoption, Migration Strategy:

One really important consideration is that with render we recommend that people destructure just what they need rather than assigning the return value an actual variable name. With testHook however, if you destructure the return value it can have some pretty confusing issues (relating to the way JavaScript closures work, I explain it in the video I linked above). So all docs will need to call this out specially and all examples should not destructure anything from testHook.

We should also make sure to document the fact that this should typically only be used for hooks that either have a lot of edge cases that need special unit testing or highly reused hooks. Typically custom hooks should just be covered by testing the components that use them.

@gnapse
Copy link
Member

gnapse commented Jan 7, 2019

Sounds good, but I'm still skeptical that is can be advertised as a general solution to test custom hooks.

What about custom hooks that only useEffect and have no return values? Or even if they return something, they're also using a useEffect and possibly interacting with a ref. For instance, I already have some hooks to spy on scroll (they need a ref to the scroll container) and load more if scroll is down enough. Or custom hooks to imperatively re-apply focus to a given element on certain conditions.

Not saying this invalidates the idea, but advertising this as something to test custom hooks in general may raise expectations too much, and the flood of questions about "how do I use renderHook / testHook to test this use case" can become overwhelming. Unless I'm not seeing from your example how could it be applied to these other scenarios.

@kentcdodds
Copy link
Member Author

Those are great points @gnapse. If the hook does side-effects though, even if you're testing a regular component, you're going to have to mock the side-effects and you could do the same with this thing:

// no need to handle the return value
testHook(useSideEffect)
expect(sideEffect).toHaveBeenCalledTimes(1)

So I'm pretty sure this will only make those kinds of things easier as well.

@kentcdodds
Copy link
Member Author

My biggest concern is that people will start testing hooks that are neither complex nor reusable in isolation. That's where docs will need a very big message indicating how this should be used.

@gnapse
Copy link
Member

gnapse commented Jan 7, 2019

I'm sorry but I still do not follow you. What is sideEffect in your code snippet:

// no need to handle the return value
testHook(useSideEffect)
expect(sideEffect).toHaveBeenCalledTimes(1)

The .toHaveBeenCalledTimes expectation suggests is a function, but where did it come from that it gets called inside useSideEffect and then we can test that it was called indeed?

Or perhaps let's work with an actual-but-simplified example. Take this hook, for instance:

function useRefocus(ref, values) {
  useEffect(() => {
    if (ref.current && typeof ref.current.focus === 'function') {
      ref.current.focus();
    }
  }, values);
}

How do you test not only that it works on the first run, but also provide stimulus to make those values change as if on subsequent renders?

Maybe I'm too much into the "create a test-only component to wrap the hook" way of thinking 😄 and I'm not the target user of this feature.

@kentcdodds
Copy link
Member Author

Good point. That's a good example of a hook that this wouldn't really serve well I think...

Can you think of a way that we could simplify testing a hook like that as well? What's the typical way that you test that kind of hook?

@gnapse
Copy link
Member

gnapse commented Jan 7, 2019

I'll try to think about it, but some additional thoughts. This surfaces yet another problem with the suggested implementation. In it, you already hit the issue of what to do if the returned value from the hook is not an object (like what if it's an array like useState does). But also it assumes the hook receives all its arguments as a single object rest. But useEffect does not. And neither does your useLocalStorageState from youtube that I watched the other day.

Actually, I think that one would be a very good basis for a proof-of-concept. It has effects and state, receives more than one arg and returns an array instead of an object. Plus it's as reusable and test-worthy as it can be. If we can make it work with that one, I'd say we're good to go. I'll see if I can carve some time, unless you beat me to it, which you probably will 😄.

PS: How does react test the built-in hooks BTW? 🤔

@j-f1
Copy link

j-f1 commented Jan 7, 2019

How about this API?

const counter = testHook(() => useCounter())

For simple hooks like this one, you can just pass the hook function, but it also makes it easy to pass custom parameters.

@alexkrolick
Copy link
Collaborator

alexkrolick commented Jan 7, 2019

Can't find it in the docs so it's probably in the RFC, but the React team said they would provide a way to test hooks in react-test-utils.

Without that or a testHook helper here, using jest.spy or jest.fn mocks could work:

test('my hook', () => {
  const check = jest.fn()
  function HookComp() {
    const [x, setX] = myHook()
    check(x, setX)
    return null
  }
  render(<HookComp />)
  expect(check).toHaveBeenCalledWith(y)
  // ...
}

It seems like there is a lot of variance in use cases and capabilities for hooks

@kentcdodds
Copy link
Member Author

I'm going to close this until we start seeing common enough patterns to warrant a utility.

@donavon
Copy link
Contributor

donavon commented Jan 21, 2019

Hello all. Kent pointed me to this issue and I thought I'd join in. I had a discussion with @kentcdodds on some tests that I was writing and he pointed me to his YouTube video on testing hooks.

Based on his work, I published a general purpose package called react-proxy-hook which initially only supported objects as return values from Hooks. I've modified it to support any return value type.

You use it by passing in your Hook. I return a "proxy hook" that you can use—a HOH (Higher Order Hook) if you will. Example:

import { proxyHook, cleanup } from 'react-proxy-hook';
import useCounter from '../use-counter';

const useCounterProxy = proxyHook(useCounter);

afterEach(cleanup);

test('useCounter', () => {
  const counterData = useCounterProxy();
  counterData.increment();
  expect(counterData.count).toBe(1);
  counterData.decrement();
  expect(counterData.count).toBe(0);
});

I've done a lot of refactoring of some tests in this PR and as you can see, this method turned out extremely useful for code readability.

The way I see it, this could go one of a few ways.

  1. Kent implements some form of testHook or proxyHook (whatever the name) in the core library itself.
  2. There is a place in the world for this in a separate package (like mine).

My package was never meant to last and was really only a POC. I'd much prefer to see this backed into react-testing-library directly, but if not, I'll keep using it.

I know there is the "you can't destructure" caveat, but it's not too bad with object and a small price to pay for the massive benefit that it provides.

With arrays, it's a little odd, but that too makes sense.

const returnedVal = useFooProxy();
expect(returnedVal[0]).toBe(0);

And you must use something like value (as I implemented it) with other types (i.e. string, numbers, etc).

const returnedVal = useFooProxy();
expect(returnedVal[0]).toBe(0);

Just my 2¢

@alexkrolick
Copy link
Collaborator

alexkrolick commented Jan 21, 2019

@donavon I like the idea. It's a clean API.

My concern is the caveat you mentioned - you can't destructure the result, which looks different than the standard way of consuming the hook so it will likely cause some people to struggle with a strange error. This is especially true for array results, which are "idiomatic" if you mimic the built-in hook APIs.

It also does obscure the fact that hooks only work inside component renders 🤔

A more explicit API could look like this:

import {HookTester, render} from 'react-testing-library'
import myCustomHook from '../my-custom-hook'

test('my hook', () => {
  let count, setCount
  render(
    <HookTester
      hook={myCustomHook}
      handleResult={result => ([count, setCount] = result)}
    />
  )

  expect(count).toBe(0)
  setCount(2)
  expect(count).toBe(2)
}
Yes, you can update values with destructuring
% node
> let x, y
undefined
> [x, y] = [1, () => x++]
[ 1, [Function] ]
> y()
1
> x
2

Note: the created [] or {} must be used inline or returned (implicitly is OK like in the REPL)



Implementation of HookTester

function HookTester({hook, handleResult}) {
  handleResult(hook())
  return null
}

@kentcdodds
Copy link
Member Author

Interesting idea Alex! Hmm... What does everyone else think? I think I like it.

@donavon
Copy link
Contributor

donavon commented Jan 22, 2019

Interesting. I like the idea. But IMO there's too much boilerplate in the test itself. What if we abstracted some of it away like this (below), passing the hook, the args to pass to the hook, and the callback handler.

This also eliminates the need to import React as there is no JSX in the test itself.

import { cleanup, hookTester } from 'react-testing-library'
import useCounter from '../use-counter'

afterEach(cleanup)

test('useCounter', () => {
  let count, increment
  hookTester(useCounter, [{initialCount: 2}], result => {
    ;({count, increment} = result)
  })

  expect(count).toBe(2)
  increment()
  expect(count).toBe(3)
})

Implementation of hookTester

import React from 'react'
import {render} from 'react-testing-library'

function HookTester({ hook, args, handler }) {
  handler(hook(...args));
  return null;
}

const hookTester = (hook, args, handler) => {
  render<HookTester hook={hook} args={args} handler={handler} />;
};

export default hookTester;

This method is not without issues either. The weirdities that I see are:

  1. Note that for some reason, I need parens around the destructure statement when destructuring into an object (probably because it looks like a code block) which caused Prettier to enter the ;.

  2. The call to hookTester make it look a lot less like it does when calling the hook in the actual component. We're trading destructing for an odd setup.

I've taken Kent's sample and ran a single test here on CodeSandbox if you'd like to play with it live.

@donavon
Copy link
Contributor

donavon commented Jan 22, 2019

We're over thinking this (also on CodeSandbox)

test('useCounter', () => {
  let count, increment
  hookTester(() => ({count, increment} = useCounter({initialCount: 2})))

  expect(count).toBe(2)
  increment()
  expect(count).toBe(3)
})

Implementation of hookTester

import React from 'react'
import {render} from 'react-testing-library'

function HookTester({callback}) {
  callback()
  return null
}

const hookTester = callback => {
  render(<HookTester callback={callback} />)
}

export default hookTester

Super simple.

@alexkrolick
Copy link
Collaborator

👍 to the single callback especially for passing arguments to the hook

@donavon
Copy link
Contributor

donavon commented Jan 22, 2019

Now... What do we call this thing?

  • hookTester isn't catchy enough. Plus "tester"?
  • renderHook is synonymous with render. (maybe??)
  • testHook (Kent's original above)
  • proxyHook is what I used in my other package.

Even though I'm not a super fan of using "render", I think renderHook will drive the point across that you use it to render (or use) the hook.

p.s. Instead of a renderProp, I think we just invented a hookProp. 😆

@gnapse
Copy link
Member

gnapse commented Jan 22, 2019

testHook seems pretty straightforward and to the point, you can't get it simpler than that

@j-f1
Copy link

j-f1 commented Jan 22, 2019

useHook?

@donavon
Copy link
Contributor

donavon commented Jan 22, 2019

I was going to put useHook in my list, but didn't want to be the one who said it first. It's just wrong.

@donavon donavon mentioned this issue Jan 22, 2019
4 tasks
@donavon
Copy link
Contributor

donavon commented Jan 22, 2019

Moving this along... #274

lucbpz pushed a commit to lucbpz/react-testing-library that referenced this issue Jul 26, 2020
* docs: update README.md

* docs: update .all-contributorsrc

Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants