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

ReactTestInstance to expose toJSON() method for focused snapshots #25329

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

mdjastrzebski
Copy link

@mdjastrzebski mdjastrzebski commented Sep 27, 2022

Summary

Allow calling toJSON on any ReactTestInstance in order to be able to make focused snapshots when writing tests using React Test Renderer or React Native Testing Library.

Currently Test Renderer only exposes parameterless toJSON() as a result to ReactTestRenderer.create() function call but that function can only output whole component tree, which frequently result in very long snapshots that are hard to work with.

Additionally, we plan to use this new function to improve implementation of various internal elements from React Native Testing Library, like getByText query algorithm, focused debug() function, etc.

This PR adds toJSON() method to ReactTestInstance class, so that users can generate JSON tree for any node. Implementation works by calling internal toJSON method if given node is host component, otherwise it recursively calls toJSON() on its children and returns collected JSON trees as array.

Resolves #14539

How did you test this change?

I've added additional assertions to all relevant ReactTestRenderer-test.internal.js tests. The new assertions verify that renderer.root.toJSON() output is the same as renderer.toJSON() output. These tests cover different component tree structures and lifecycle changes.

@facebook-github-bot
Copy link

Hi @mdjastrzebski!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@sizebot
Copy link

sizebot commented Sep 27, 2022

Comparing: ea04a48...6deb332

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 135.46 kB 135.46 kB = 43.41 kB 43.41 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 147.72 kB 147.72 kB = 47.17 kB 47.17 kB
facebook-www/ReactDOM-prod.classic.js = 491.70 kB 491.70 kB = 87.49 kB 87.49 kB
facebook-www/ReactDOM-prod.modern.js = 477.00 kB 477.00 kB = 85.24 kB 85.24 kB
facebook-www/ReactDOMForked-prod.classic.js = 491.70 kB 491.70 kB = 87.49 kB 87.49 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.production.min.js +0.39% 92.59 kB 92.95 kB +0.32% 28.54 kB 28.63 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.production.min.js +0.39% 92.61 kB 92.97 kB +0.32% 28.54 kB 28.63 kB
oss-stable-semver/react-test-renderer/umd/react-test-renderer.production.min.js +0.39% 92.84 kB 93.20 kB +0.32% 29.01 kB 29.11 kB
oss-stable/react-test-renderer/umd/react-test-renderer.production.min.js +0.39% 92.86 kB 93.22 kB +0.33% 29.01 kB 29.11 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.production.min.js +0.37% 97.33 kB 97.69 kB +0.32% 29.87 kB 29.96 kB
oss-experimental/react-test-renderer/umd/react-test-renderer.production.min.js +0.37% 97.57 kB 97.93 kB +0.28% 30.28 kB 30.36 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-prod.js +0.26% 280.77 kB 281.50 kB +0.27% 49.73 kB 49.86 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-profiling.js +0.25% 296.35 kB 297.08 kB +0.26% 52.09 kB 52.23 kB

Generated by 🚫 dangerJS against 6deb332

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@mdjastrzebski
Copy link
Author

Pinging so this PR doesn't get closed. I'm looking forward for some feedback on it.

The change is small, it's tested, and would be really useful for Test Renderer users, as indicated in #14539 :-)

@eps1lon
Copy link
Collaborator

eps1lon commented Oct 4, 2022

A test that mimics how you would use, would help. Looks like none of the existing tests really showcase a need for this feature since it's just used on the root.

@gaearon
Copy link
Collaborator

gaearon commented Oct 4, 2022

On the web, we've moved away from recommending the test renderer, and instead recommended to query the native tree. I wonder if instead of expanding the API surface of the Test Renderer, whether we've considered to do something similar on RN. I don't mean literally running Android/iOS, but more like whether we can expose something on the RN tree level and use that as the way forward. We've been adding some test selector APIs (#18607) which aren't used anywhere yet but I think they should also (?) work on RN.

@mdjastrzebski mdjastrzebski force-pushed the feature/test-instance-expose-toJSON branch from 95c52c5 to 43f3333 Compare October 4, 2022 20:19
@mdjastrzebski
Copy link
Author

@eps1lon I've added tests for more complex tree structure with mixed host & composite elements, re-using existing arrangement from traversal tests. I used toEqual matcher instead of toMatchSnapshot/InlineSnapshot to be consistent with existing root-level toJSON() tests.

@gaearon sounds like an interesting idea to consider, yet quite complex one, as its appears to work on lower level of abstraction than Test Renderer. One aspect that bothers me though is the "experimental" status of these queries. Will research more on that, and discuss with other RNTL maintainers.

Meanwhile, I would argue that merging this PR would still benefit the existing RN projects using RNTL/Test Renderer with immediate benefit of focused snapshot testing. Using Test Renderer is currently the main solution for writing component & TL-style integration tests in RN world, with a lot of projects being heavily invested in creating their automated tests. While the cost is relatively minor addition to Test Renderer package.

@gaearon
Copy link
Collaborator

gaearon commented Oct 4, 2022

One aspect that bothers me though is the "experimental" status of these queries

It's implemented. The only reason they're called experimental is because we never deployed or exposed them anywhere so they're just... kind of there.

@thymikee
Copy link
Contributor

@gaearon is there any recommendation of moving from react-test-renderer to test selector API? Some plans to deprecate it maybe? If you could confirm whether this approach works with RN or not, that would be great 👍🏼

@mdjastrzebski
Copy link
Author

@eps1lon I've added more tests for toJSON() method as you requested. Is there anything more I can do/change/implement to move forward this PR?

@mdjastrzebski
Copy link
Author

It's me again, I wanted to check with the React team, if this PR has chances to move forward.

To give you some more context, React Test Renderer is the primary solution used for testing in the React Native ecosystem, so while it might seem a bit odd to use in on the web, there are a lot of RN apps that use it directly or through React Native Testing Library, which I maintain, to provide solid integration & component test coverage. This PR would really bring value to RN space.

I believe that having toJSON() implementation inside React Test Renderer is the best place as it uses the internal fiber APIs, which are encapsulated through TestRenderer and ReactTestInstance types.

@eps1lon, @gaearon pls help

@stevehanson
Copy link

Curious if there has been any movement on this. I'm a React Native developer, and these changes would be a huge benefit to my testing workflow if this were merged and React Native Testing Library were able to take advantage of this. Thanks all!

@brittanyvaldez
Copy link

would also like to see this implemented

Copy link

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Apr 10, 2024
@mdjastrzebski
Copy link
Author

bump

@github-actions github-actions bot removed the Resolution: Stale Automatically closed due to inactivity label Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reactTestInstance.toJSON() serializer
8 participants