Testing custom hook throwing error for incorrect arguments

[dhruv-m-patel]

• react-hooks-testing-library version: 0.3.7
• react-testing-library version: N/A (I haven't installed this package in my repo for testing)
• react version: 16.8
• node version: 8.11.2
• npm (or yarn) version: 5.6.0

Relevant code or config:

const usCustomHook = (argA, argB) => {
  if (!argA || typeof argA !== 'function') {
    throw new Error('argA must be a function');
  }

  if (!argB || typeof argB !== 'string' || !argB.length) {
    throw new Error('argB must be a string');
  }

  ..... // code for the custom hook
}

What you did:

I am trying to implement a custom hook adding validation for the arguments to match the expectations. undefined arguments should result in throwing error.

What happened:

The custom hook itself works as expected, however when testing above using jest passes tests but prints errors in console:

Tests:

test('Not providing valid argA should fail', () => {
    try {
      renderHook(() => useCustomHook(undefined, 'invalid data'));
    } catch (e) {
      expect(e.message).toBe('argA must be a function');
    }

    try {
      renderHook(() => useCustomHook('foo', 'invalid data'));
    } catch (e) {
      expect(e.message).toBe('argA must be a function');
    }
  });

  test('Not providing valid argB should fail', () => {
    try {
      renderHook(() => useCustomHook(() => {}, undefined));
    } catch (e) {
      expect(e.message).toBe('argB must be a string');
    }

    try {
      renderHook(() => useCustomHook(() => {}, 12345));
    } catch (e) {
      expect(e.message).toBe('argB must be a string');
    }
  });

Error it prints in console with tests passing:

console.error node_modules/jsdom/lib/jsdom/virtual-console.js:29
    Error: Uncaught [Error: argA must be a function]

console.error node_modules/react-dom/cjs/react-dom.development.js:17071
    The above error occurred in the <TestHook> component:
        in TestHook
    
    Consider adding an error boundary to your tree to customize error handling behavior.
    Visit https://fb.me/react-error-boundaries to learn more about error boundaries.

console.error node_modules/jsdom/lib/jsdom/virtual-console.js:29
    Error: Uncaught [Error: argA must be a function]

console.error node_modules/react-dom/cjs/react-dom.development.js:17071
    The above error occurred in the <TestHook> component:
        in TestHook
    
    Consider adding an error boundary to your tree to customize error handling behavior.
    Visit https://fb.me/react-error-boundaries to learn more about error boundaries.

console.error node_modules/jsdom/lib/jsdom/virtual-console.js:29
    Error: Uncaught [Error: argB must be a string]

console.error node_modules/react-dom/cjs/react-dom.development.js:17071
    The above error occurred in the <TestHook> component:
        in TestHook
    
    Consider adding an error boundary to your tree to customize error handling behavior.
    Visit https://fb.me/react-error-boundaries to learn more about error boundaries.

console.error node_modules/jsdom/lib/jsdom/virtual-console.js:29
    Error: Uncaught [Error: argB must be a string]

console.error node_modules/react-dom/cjs/react-dom.development.js:17071
    The above error occurred in the <TestHook> component:
        in TestHook
    
    Consider adding an error boundary to your tree to customize error handling behavior.
    Visit https://fb.me/react-error-boundaries to learn more about error boundaries.

Reproduction:

CodeSandBox link: https://codesandbox.io/s/183om7054

Problem description:

I believe it would be ideal not to print the jsdom errors since the errors are catched and error message is verified using jest.

[mpeyper]

Hi @dhruv-m-patel,

The issue here is that the error is actually being thrown in the React render lifecycle, so it is never raised inside your test. You can see this by changing one of the tests to be:

    try {
      renderHook(() => useError(undefined, 'invalid data'))
      fail('should never get here')
    } catch (e) {
      expect(e.message).toBe('argA must be a function')
    }

Now your test fails with the error having an unexpected message ('should never get here'). You will actually find that your tests were only passing because they never hit the catch blocks and consequently never called expect.

Obviously, this is less than ideal when wanting to test error cases, but it does cause us a bit of an issue of how to actually surface the error in these cases. The render operation is outsourced to react-testing-library and the callback pattern does not lend itself well to synchronous errors (when you pass a function as a parameter, there is no guarantee when or how it will be called, if it is even called at all, so relying on a try/catch around the outer function call not always a sure thing).

I'm open to suggestions, but I propose that we extend the result object to have a new field added, error that contains any thrown value that occurred during the hook callback passed to renderHook. It would be undefined if nothing was thrown or if a renderHook did throw, but a subsequent rerender did not. Similarly, the current field would not be undefined if the renderHook callback threw when it was called.

With this change your test would look something like:

test('Not providing valid argA should fail', () => {
  const { result } = renderHook(() => useCustomHook(undefined, 'invalid data'))
  expect(result.error.message).toBe('argA must be a function')
})

How does this sound?

[dhruv-m-patel]

I like the idea of enhancing the renderHook to always return a result with error. 👍

[dhruv-m-patel]

@mpeyper, to clarify I would imagine renderHook would return result with error set to undefined when there was no error, but in case of error it would contain the error representation.

[mpeyper]

I'm about to jump on a flight, so I might take a look at this while in the air.

Quick question, would you expect waitForNextUpdate to resolve or reject if the hook threw, given the synchronous version (e.g. your example) does not throw?

[dhruv-m-patel]

Since the hook first validates the types of arguments and throws errors, I would say reject not resolve. If the args don't match the hook's expectations, the hook won't be usable and hence there won't be a next update on it.

[mpeyper]

So I made a quick spike of this on my flight and it raised more questions than it answered.

Essentially, I'm unable to find a common approach between the synchronous and asynchronous that feels right.

The synchronous test looks something like

test('should capture error', () => {
  const { result } = renderHook(() => useError())

  expect(result.error).toEqual(Error('some error'))
})

But the asynchronous test looks like

test('should capture async error', async () => {
  const { waitForNextUpdate } = renderHook(() => useAsyncError())

  try {
    await waitForNextUpdate()
    fail('expected error to be thrown')
  } catch (e) {
    expect(e).toEqual(Error('some error'))
  }
})

which is the opposite of what I previously said about the use of try/catch not being a good fit for the callback pattern.

So the obvious solution to this is to go back on what I said and make the first test also use try/catch

test('should capture error', () => {
  try {
    renderHook(() => useError())
  } catch (e) {
    expect(e).toEqual(Error('some error'))
  }
})

This seems ok on the surface, as both versions conform to the same pattern and work more or less as one would expect, but now consider the case where the first render is fine, but a rerender throws

test('should capture error' on rerender, () => {
  const { result, rerender } = renderHook(() => useError())

  expect(result.current.someValue).toEqual('some value')

  try {
    rerender()
  } catch (e) {
    expect(e).toEqual(Error('some error'))
  }
 
  // is using result valid here? 
  // what about calling rerender again?
  // should the component wrapper unmount at this point?
})

Ok, so what if we go the other way and remove the try/catch from the async test?

test('should capture async error', async () => {
  const { result, waitForNextUpdate } = renderHook(() => useAsyncError())

  await waitForNextUpdate() // the assumption here is the the promise resolves instead of rejects so that assertions can be made on `result`

  expect(result.error).toEqual(Error('some error'))
})

Again, this seems to satify my common approach hang up, but there is another issue with it (which also applies to the synchronous version). These tests are fine when you are asserting and expected failure, but what happens when we are asserting an expected result, but an error was thrown instead?

test('should capture error', () => {
  const { result } = renderHook(() => useError())

  expect(result.current.someValue).toEqual('someValue') // Uncaught TypeError: Cannot read property 'someValue' of undefined
})

This is an incredibly unhelpful error to see because useError threw an exception.

After discussing this issue with a colleague, they suggested perhaps using a getter on result.current to throw the exception, which works for this case

test('should capture error', () => {
  const { result } = renderHook(() => useError())

  expect(result.current.someValue).toEqual('someValue') // Error: some error
})

but then the test asserting an expected error start to become a bit funky

test('should capture error', () => {
  const { result } = renderHook(() => useError())

  try {
    result.current
  } catch (e) {
    expect(e).toEqual(Error('some error'))
  }
})

So my final thought is to actually do both. Use a getter to throw the error if the current value is not valid AND set an error value on the result for assert expected exceptions. My only hangup is that this seems like there are 2 prescribed ways to actually access the hook errors, but I think it's justified as it's actually solving 2 different problems (asserting errors v.s. providing useful information when value assertions fail).

Does any of that make sense? Any other thoughts, ideas or comments for anyone? Am I overthinking this?

[mpeyper]

Given my previous ramblings, these are the current tests I have passing:

import { useState, useEffect } from 'react'
import { renderHook } from 'src'

describe('error hook tests', () => {
  function useError(throwError) {
    if (throwError) {
      throw new Error('expected')
    }
    return true
  }

  const somePromise = () => Promise.resolve()

  function useAsyncError(throwError) {
    const [value, setValue] = useState()
    useEffect(() => {
      somePromise().then(() => {
        setValue(throwError)
      })
    }, [throwError])
    return useError(value)
  }

  test('should raise error', () => {
    const { result } = renderHook(() => useError(true))

    expect(() => {
      expect(result.current).not.toBe(undefined)
    }).toThrow(Error('expected'))
  })

  test('should capture error', () => {
    const { result } = renderHook(() => useError(true))

    expect(result.error).toEqual(Error('expected'))
  })

  test('should not capture error', () => {
    const { result } = renderHook(() => useError(false))

    expect(result.current).not.toBe(undefined)
    expect(result.error).toBe(undefined)
  })

  test('should reset error', () => {
    const { result, rerender } = renderHook((throwError) => useError(throwError), {
      initialProps: true
    })

    expect(result.error).not.toBe(undefined)

    rerender(false)

    expect(result.current).not.toBe(undefined)
    expect(result.error).toBe(undefined)
  })

  test('should raise async error', async () => {
    const { result, waitForNextUpdate } = renderHook(() => useAsyncError(true))

    await waitForNextUpdate()

    expect(() => {
      expect(result.current).not.toBe(undefined)
    }).toThrow(Error('expected'))
  })

  test('should capture async error', async () => {
    const { result, waitForNextUpdate } = renderHook(() => useAsyncError(true))

    await waitForNextUpdate()

    expect(result.error).toEqual(Error('expected'))
  })

  test('should not capture async error', async () => {
    const { result, waitForNextUpdate } = renderHook(() => useAsyncError(false))

    await waitForNextUpdate()

    expect(result.current).not.toBe(undefined)
    expect(result.error).toBe(undefined)
  })

  test('should reset async error', async () => {
    const { result, waitForNextUpdate, rerender } = renderHook(
      (throwError) => useAsyncError(throwError),
      {
        initialProps: true
      }
    )

    await waitForNextUpdate()

    expect(result.error).not.toBe(undefined)

    rerender(false)

    await waitForNextUpdate()

    expect(result.current).not.toBe(undefined)
    expect(result.error).toBe(undefined)
  })
})

[dhruv-m-patel]

It is great to see result.error being set with Error('message') when capturing the error in both sync and async version of tests. Thanks for taking time to work on this issue.

[perymimon]

So what happened with that? Does it merge to the final release?
Because when I'm checking my code with throw expected it throw on the first line of
const {result, rerender} = renderHook((args=[]) => useHook(...args))