feat(server): add SSR test for renderHook #799

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

Jump to bottom

Merged

mpeyper merged 5 commits into testing-library:main from jsjoeio:jsjoeio-add-test

Jun 18, 2022

Merged

feat(server): add SSR test for renderHook #799

mpeyper merged 5 commits into testing-library:main from jsjoeio:jsjoeio-add-test

Jun 18, 2022

Conversation

jsjoeio

Copy link

Contributor

@jsjoeio jsjoeio commented Feb 27, 2022 •

edited

Loading

What:

This PR adds a new test based on the work done in #607 to fix #605. Partially related to #796

Why:

I noticed a test wasn't added when that bug was fixed. I decided to add a test so we could prevent regressions.

How:

A new test was added which runs in a Node environment in Jest.

Checklist:

Documentation updated
Tests
Ready to be merged
Added myself to contributors table

Joe added 2 commits

February 27, 2022 05:02


 refactor(server/pure): remove unnecessary type annotation

5247d27


 feat: add ssr.test.ts

93d927d

This adds a new test to verify that renderHook can be called in an
SSR-like environment based on the changes implemented in testing-library#607.

@netlify

Copy link

netlify bot commented Feb 27, 2022 •

edited

Loading

❌ Deploy Preview for react-hooks-testing-library failed.

Name	Link
🔨 Latest commit	`9d35e1c`
🔍 Latest deploy log	https://app.netlify.com/sites/react-hooks-testing-library/deploys/62a39be0c64b2b000889682f


 chore: update contributors table

59c752f

@codecov

Copy link

codecov bot commented Feb 27, 2022 •

edited

Loading

Codecov Report

Merging #799 (9d35e1c) into main (efd262c) will not change coverage.
The diff coverage is 100.00%.

@@ Coverage Diff @@
## main #799 +/- ##
=========================================
 Coverage 100.00% 100.00% 
=========================================
 Files 15 15 
 Lines 246 245 -1 
 Branches 34 34 
=========================================
- Hits 246 245 -1

Impacted Files	Coverage Δ
src/server/pure.ts	`100.00% <100.00%> (ø)`

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 efd262c...9d35e1c. Read the comment docs.

jsjoeio

jsjoeio commented

Feb 27, 2022

View reviewed changes

src/__tests__/ssr.test.ts

Comment on lines +1 to +3

/**

* @jest-environment node

Copy link

Contributor Author

Choose a reason for hiding this comment

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

It would be nice if we could have sone server only tests run in the node environment without jsdom to make sure we don't break it in the future, but I'm not sure how we would do that for just some tests?

✍🏼 This technique is handy for running specific tests in a Node environment.

See docs.

src/server/pure.ts

@@ -13,7 +13,7 @@ function createServerRenderer<TProps, TResult>(

) {

let renderProps: TProps | undefined

let container: HTMLDivElement | undefined

let serverOutput: string = ''

let serverOutput = ''

Copy link

Contributor Author

Choose a reason for hiding this comment

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

🧹 Since we give serverOutput a default value of '', TS can infer string type. The type annotation here is not needed.

@jsjoeio

Copy link

Contributor Author

jsjoeio commented Feb 27, 2022

 Warning: It looks like you called ReactTestUtils.act(...) in a non-browser environment. If you're using TestRenderer for your tests, you should call ReactTestRenderer.act(...) instead of ReactTestUtils.act(...).

I looked at src/__tests__/autoDetectRenderer.test.ts because it has a reactTestRenderer but it looks custom 🤔 Any tips on how to fix this?

@jsjoeio jsjoeio marked this pull request as ready for review

February 27, 2022 05:22

@jsjoeio

Copy link

Contributor Author

jsjoeio commented Mar 4, 2022

Hey @mpeyper - looks like the test is failing. Would love to get your insights to fix this 🙏

@mpeyper

Copy link

Member

mpeyper commented Mar 5, 2022

The test looks fine, I’ll see if I can get to the bottom of it.

mpeyper

mpeyper reviewed

Jun 7, 2022

View reviewed changes

src/server/pure.ts

@@ -13,7 +13,7 @@ function createServerRenderer<TProps, TResult>(

) {

let renderProps: TProps | undefined

let container: HTMLDivElement | undefined

let serverOutput: string = ''

let serverOutput = ''

const testHarness = createTestHarness(rendererProps, wrapper, false)

return {

Copy link

Member

Choose a reason for hiding this comment

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

github won't let me comment on the actual line, but the error in the test is being caused by the act call on line 22. For some reason, when you use react-dom@16.9.0 this tried to use the document which throws an error when testing in the node environment, which doesn't occur in newer versions.

You can replicate this by running:

npm run install:react-16.9.0
npm test -- --no-watch src/__tests__/ssr.test.ts

In the context of server rendering, the act call is not really necessary here. Dropping it out still passes all of our tests and allows your new test to pass with the older version:

 render(props?: TProps) {
 renderProps = props
 try {
 serverOutput = ReactDOMServer.renderToString(testHarness(props))
 } catch (e: unknown) {
 rendererProps.setError(e as Error)
 }
 },

Copy link

Contributor Author

@jsjoeio jsjoeio Jun 7, 2022

Choose a reason for hiding this comment

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

Wow, I don't think I would have figured that out on my own 😅 I'll test this out and make the necessary changes. Thanks a bunch!

jsjoeio added 2 commits

June 10, 2022 19:29

@jsjoeio


 refactor: remove act call in render in sever/pure

c37e8a4

@mpeyper explained how this `act` call in server rendering is not really
necessary so we can remove it.

@jsjoeio


 Merge branch 'main' into jsjoeio-add-test

9d35e1c

@jsjoeio

Copy link

Contributor Author

jsjoeio commented Jun 10, 2022

Well, that worked! Of course now Netlify is failing lol

12:34:31 PM: error Error in "/opt/build/repo/node_modules/gatsby-plugin-mdx/gatsby-node.js": globalThis is not defined

@mpeyper

Copy link

Member

mpeyper commented Jun 10, 2022 •

edited

Loading

Yeah, the Netlify build had been failing for a while and I’m not sure how to fix it. I’ve mostly been ignoring it with maintenance on this library wrapping up soon anyway.

This all looks fine to me and will try to get it merged next time I’m at my computer.