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

Custom renderHook implementation for testing custom hooks #5381

Merged
merged 14 commits into from
May 17, 2022

Conversation

cjreimer
Copy link
Contributor

@cjreimer cjreimer commented May 2, 2022

Status:

  • Base code written
  • Polishing / typescript
  • Testing (decision made to eliminate testing)
  • Documentation

The purpose of this PR is to facilitate testing of custom react hooks within redwood. @testing-library/react-hooks has a renderHook function appropriate for this. However, when calling renderHook, it does not by default have the MockProviders wrapper and thus any typical redwood testing features, such as mocking of graphQL queries and mutations, fails. It is possible to add this manually, but it is already added automatically for the Redwood user for the similar render function

Redwood currently addresses the render function for testing well by providing a customRender function that includes the required wrapper and aliasing it to be called render.

As it took me a bit to chase this all down when I encountered it, this PR proposes a similar implementation for renderHook. A 'customRenderHook' function with the required wrapper is created and it is aliased to 'renderHook'. Note that this does require the addition of @testing-library/react-hooks to package.json.

I would love to get feedback to confirm we would like to add this to Redwood before I complete this PR. If we decide not to add this, then documentation on how to do this manually would be appropriate.

@netlify
Copy link

netlify bot commented May 2, 2022

Deploy Preview for redwoodjs-docs ready!

Name Link
🔨 Latest commit fac2ef4
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/6283bb93fcf29d0009eac573
😎 Deploy Preview https://deploy-preview-5381--redwoodjs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@Tobbe
Copy link
Member

Tobbe commented May 2, 2022

This looks great to me Curtis 🙂

I would love to get feedback to confirm we would like to add this to Redwood before I complete this PR.

I'm all for it, but let me check with the team

@Tobbe
Copy link
Member

Tobbe commented May 2, 2022

@cjreimer We all agree. This would be a nice addition to RW. Please go-ahead 🙂

@cjreimer
Copy link
Contributor Author

cjreimer commented May 2, 2022

Great, thanks @Tobbe!

@cjreimer
Copy link
Contributor Author

cjreimer commented May 3, 2022

OK, I've made progress, but I could use some help from someone more familiar with the redwood jest configuration between packages. I've tried to set up a test that is similar to what I'm putting in the documentation, but I'm getting the following error:

@redwoodjs/testing:  FAIL  src/web/__tests__/renderHook.test.tsx
@redwoodjs/testing:   ● Test suite failed to run
@redwoodjs/testing:     Cannot find module '~__REDWOOD__USER_ROUTES_FOR_MOCK' from 'src/web/MockProviders.tsx'
@redwoodjs/testing:     Require stack:
@redwoodjs/testing:       src/web/MockProviders.tsx
@redwoodjs/testing:       src/web/customRender.tsx
@redwoodjs/testing:       src/web/index.ts
@redwoodjs/testing:       src/web/__tests__/renderHook.test.tsx
@redwoodjs/testing:       19 | const {
@redwoodjs/testing:       20 |   default: UserRouterWithRoutes,
@redwoodjs/testing:     > 21 | } = require('~__REDWOOD__USER_ROUTES_FOR_MOCK')
@redwoodjs/testing:          |     ^
@redwoodjs/testing:       22 |
@redwoodjs/testing:       23 | const fakeUseAuth = (): AuthContextInterface => ({
@redwoodjs/testing:       24 |   loading: false,
@redwoodjs/testing:       at Resolver.resolveModule (../../node_modules/jest-resolve/build/resolver.js:324:11)
@redwoodjs/testing:       at Object.<anonymous> (src/web/MockProviders.tsx:21:5)

Would someone be able to help advise if we can modify our jest configuration to run this test within the testing package?

@virtuoushub
Copy link
Collaborator

Hi @cjreimer, thank you for your contribution thus far!

So what I think is happening is the new test, has a transitive dependency on MockProviders, which has these lines: https://github.com/redwoodjs/redwood/blob/main/packages/testing/src/web/MockProviders.tsx#L19-L21

~__REDWOOD__USER_ROUTES_FOR_MOCK is never mapped in https://github.com/redwoodjs/redwood/blob/main/packages/testing/jest.config.js so this new test has no idea what to do.

I have gotten it a little bit further by using a valid path in jest.config.js, e.g. '~__REDWOOD__USER_ROUTES_FOR_MOCK': '<rootDir>/web/index.js',; however this is not quite correct as we need whatever that module is mapped to be a valid Routes file (e.g. https://github.com/redwoodjs/redwood/blob/main/packages/internal/src/__tests__/fixtures/nestedPages/web/src/Routes.js ). Ideally we have a mock of that type of file for use in testing for exactly what you are trying to do. Unfortunately right now we do not have this.

I am working with the other core team members to figure out how best to advise we unblock you.

@cjreimer
Copy link
Contributor Author

cjreimer commented May 6, 2022

Thanks @virtuoushub! I appreciate the help! I've cleaned up the code and docs, and other than the testing block, the code is ready for review.

I'll note that one other option is to just omit the test. The base code addition is pretty straightforward. I'll leave that up to the core team to decide.

@virtuoushub virtuoushub self-assigned this May 9, 2022
@jtoar jtoar added the release:feature This PR introduces a new feature label May 9, 2022
@jtoar jtoar self-assigned this May 11, 2022
@jtoar
Copy link
Contributor

jtoar commented May 12, 2022

@cjreimer thanks! I ran into this while writing tests for the example store and had to make a new react component just so I could call the hook inside it, so I definitely see the use case here.

I had a quick look and I'll keep reviewing, but have a question: it looks like @testing-library/react also exports a renderHook function: https://github.com/testing-library/react-testing-library/blob/9171163fccf0a7ea43763475ca2980898b4079a5/src/pure.js#L221-L247.

It's in their docs too: https://testing-library.com/docs/react-testing-library/api/#renderhook.

The package you added is in the testing-library organization, and is published under the testing-library namespace, so it's official. But do you know what the difference is, or why we should prefer one over the other?

@cjreimer
Copy link
Contributor Author

Hey @jtoar, great catch! I looked and I believe the answer is in:

https://www.npmjs.com/package/@testing-library/react-hooks

It looks like they are in the process of depreciating the react-hooks package and moving it all to the core @testing-library/react package. I'll look into using the new one in the base package .... much better if it's ready!
Thanks, Dom!

@cjreimer
Copy link
Contributor Author

@jtoar, Sorry, I looked a bit further and they are dropping support for React 17 in version 13 of the @testing-library/react. We are still tied to React 17 in Redwood, so we will have to wait for that change.

See:
https://github.com/testing-library/react-testing-library/releases

I'll make a comment in the code.

@virtuoushub virtuoushub removed their assignment May 13, 2022
Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

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

Thanks @cjreimer, this looks good! Tried it out on the example store and it streamlined writing a test where I was using a custom hook.

I'm with you and @virtuoushub—we can get rid of the test. The logic is virtually the same as the customRender, which we don't have a specific test for, but we do test the MockProviders which are used in the wrapper of both functions. It's important that those work.

@cjreimer
Copy link
Contributor Author

Thanks @jtoar! I appreciate your cleanup and I removed the test. It should be ready to go.

@jtoar
Copy link
Contributor

jtoar commented May 17, 2022

Thanks @cjreimer! Had to make one small last change to get storybook working. We just had to be a little more specific with the import or else @testing-library/react-hooks tries to auto detect which renderer to use and in this case picks the wrong one. All good now though (pending checks)

@cjreimer
Copy link
Contributor Author

Sounds good @jtoar ! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature
Projects
Status: Archived
Development

Successfully merging this pull request may close these issues.

4 participants