react
react copied to clipboard
facebook
ReactTestInstance to expose toJSON() method for focused snapshots
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.
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 [email protected]. Thanks!
Comparing: ea04a486a7c27771f7eea896565c0980927ed3b4...6deb332bb4d037caca7b2f3ab2395725ea9c96ea
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
Generated by :no_entry_sign: dangerJS against 6deb332bb4d037caca7b2f3ab2395725ea9c96ea
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!
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 :-)
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.
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 (https://github.com/facebook/react/pull/18607) which aren't used anywhere yet but I think they should also (?) work on RN.
@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.
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.
@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 👍🏼
@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?
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
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!
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[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
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.
Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!