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

Shallow renderer and test utils bundles #9426

Merged
merged 37 commits into from
Apr 19, 2017

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Apr 14, 2017

Adds new bundles introduced with React 15.5 release to master (and 16 alpha):

react-dom/test-utils

This new bundle contains what used to be react-addons-test-utils. This bundle shares things from react-dom rather than duplicates them.

A temporary createRenderer method has been left behind as a way to access the new shallow renderer. This is for the ReactNative release cycle only and should be going away before the final release.

react-test-renderer/shallow

This new shallow renderer is almost entirely stand-alone (in that it doesn't use the React reconciler or scheduler). The only touch points are ReactElement and prop/context validation. This renderer is stack and fiber compatible.

PS I used __SECRET_INTERNALS please don't fire me 😛

@bvaughn bvaughn changed the title New (fiber-only) shallow renderer New (fiber-only) shallow renderer [WIP] Apr 14, 2017
@bvaughn bvaughn force-pushed the react-dom-test-utils-shallow-renderer branch 3 times, most recently from d98cfee to fb05fc9 Compare April 14, 2017 17:22
enumerable: false,
writable: false,
value: owner,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Related past discussion #6355

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you verify with @sebmarkbage he has no concerns here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seb and I chatted about this idea in person. He didn't seem to, but I'll let him comment here (or I can follow up with him in person on Monday). Sure!

@bvaughn bvaughn force-pushed the react-dom-test-utils-shallow-renderer branch from a5b7aff to b8c35d8 Compare April 14, 2017 20:25
@bvaughn bvaughn changed the base branch from react-dom-test-utils to master April 14, 2017 22:19
@bvaughn bvaughn changed the title New (fiber-only) shallow renderer [WIP] Shallow renderer and test utils bundles Apr 14, 2017
@bvaughn
Copy link
Contributor Author

bvaughn commented Apr 14, 2017

Adding a few more folks who may be interested in looking over this change


if (process.env.NODE_ENV === 'production') {
throw Error('shallow renderer is not available in production mode.');
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also throw for the main test renderer entry point then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or should we create prod versions of all of them? I don't see benefit but somebody might come up with a use case. Maybe just worth keeping in mind.

Copy link
Contributor Author

@bvaughn bvaughn Apr 15, 2017

Choose a reason for hiding this comment

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

Or should we create prod versions of all of them?

I don't see the value of this, and it would cause problems with __DEV__ only things like the non-enumerable _owner attribute.

I'll throw in the main test renderer too, sure.

Choose a reason for hiding this comment

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

@gaearon @bvaughn I'm having a problem with these lines: I'm using enzyme and it uses shallow rendering, for tests, while NODE_ENV is set to production. So my tests are failing because of this.

"gzip": 55887
},
"react-test-utils.development.js (NODE_DEV)": {
"size": 510240,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this file was deleted so the stat is probably not needed?

enumerable: false,
writable: false,
value: owner,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you verify with @sebmarkbage he has no concerns here?

const tree = this._renderer.toTree();
if (tree && tree.rendered) {
// If we created a context-wrapper then skip over it.
const element = tree.type.childContextTypes
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems fragile. What if it isn't our context but the root element just happens to be a context provider?

To be honest I'm not a fan of second undocumented context argument. Our other APIs don't take it. I think it's remaining from the time context was owner-based and we might as well deprecate it and tell people to use their own context providers explicitly. But it's probably annoying to do in this PR.

Let's just be stricter with checking here.


// Convert the rendered output to a ReactElement.
// This supports .toEqual() comparison for test elements.
return React.createElement(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this re-trigger any validation? Would people see duplicate messages?

Copy link
Contributor Author

@bvaughn bvaughn Apr 16, 2017

Choose a reason for hiding this comment

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

This would only re-trigger if the ReactElement returned from the top-level render was a class component with propTypes. In that case, it still wouldn't show a duplicate message because checkPropTypes tracks loggedTypeFailures and avoids re-logging duplicates.

Do you think this is reasonable? Should I look for a way of bypassing checkPropTypes for this case (or some other approach)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about the key warning?
It’s probably fine, just making sure you’re aware of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Key warning will only log once as well.

Yup, much appreciated for the double check. 😄


getRenderOutput() {
if (this._renderer) {
const tree = this._renderer.toTree();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will likely make all shallow tests slower. Would be good to measure it on some mid size codebase to check the impact. It might be better to add some way of bailing out if it's too bad.

@@ -12,23 +12,27 @@
'use strict';

Copy link
Collaborator

Choose a reason for hiding this comment

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

While we're at it can we move src/test to a more appropriate place (considering the package structure)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

class SomeComponent extends React.Component {
render() {
// Shallow renderer only implemented for Fiber in 16+
if (ReactDOMFeatureFlags.useFiber) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit confused by this comment. Can we branch on require instead? Otherwise it looks like we stopped testing shallow renderer in Stack at all. However the Stack version is what we're currently running in www.

Copy link
Contributor Author

@bvaughn bvaughn Apr 15, 2017

Choose a reason for hiding this comment

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

Otherwise it looks like we stopped testing shallow renderer in Stack at all.

This new shallow renderer is fiber only. Seemed like too much effort to create a new one that supports stack and works with our new bundles- just to throw it away. (I mentioned this in the description but maybe it wasn't prominent enough.)

I'm not sure how to address www yet. I'll give it some more thought. 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh. I didn't realize we deleted the Stack based one. Our usual strategy is to keep both until we’ve migrated everything that uses the old one (e.g. www). This lets us revert to using the old one if the new one has issues. Otherwise once this is merged, we have no way but to fix forward. Which might be fine, but it’s blocking any future syncs, and I don’t know if this will pass www tests or not.

Usually we keep both, and let the tests pick the right one based on the flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, totally understood- and I'm glad you bought it up. I'll figure out a strategy for www tests with stack once the fiber bundles and renderer is solid. 😁

@gaearon
Copy link
Collaborator

gaearon commented Apr 16, 2017

I am also concerned that running toTree() will also make shallow rendering more fragile. For example imagine a leaf component changes to throw in render(). Now your grandparent shallow rendering test fails even though it's meant to be isolated. Am I missing something here?

@bvaughn
Copy link
Contributor Author

bvaughn commented Apr 16, 2017

Great point Dan.

I just realized something silly anyway 😐 The new renderer actually causes render to be called on the full tree because of how the reconciler works. I need to think about this a bit more.

I'll also add a test to verify that the shallow renderer is...shallow. This is embarrassing. 😅

@bvaughn bvaughn changed the title Shallow renderer and test utils bundles Shallow renderer and test utils bundles [WIP] Apr 16, 2017
@gaearon
Copy link
Collaborator

gaearon commented Apr 16, 2017

Yea, this is what I meant by the "bailout" in the comment above. The original PR by @lelandrichardson contained this logic in a more generic way (it allowed "redirecting" types) and I think @sebmarkbage had a strategy for using this for shallow renderer in #8931 (comment). This way it would only render one level deep.

@bvaughn
Copy link
Contributor Author

bvaughn commented Apr 16, 2017

Gotcha! Thanks for pointers. 😄

@bvaughn bvaughn force-pushed the react-dom-test-utils-shallow-renderer branch from bef9f05 to 86bfb52 Compare April 17, 2017 18:37
@bvaughn
Copy link
Contributor Author

bvaughn commented Apr 17, 2017

Gave this some more thought over the weekend and tried a couple of alternate implementations. One was similar to ReactNoopRenderer but required monkey-patching ReactCurrentOwner to work which felt too hacky. The other one (which I have now committed+pushed) is stand-alone. I want to do a little more testing to make sure I didn't introduce a regression WRT the lifecycle hooks or something similar- but I feel this route is probably better than the previous test-renderer-based approach.

@bvaughn
Copy link
Contributor Author

bvaughn commented Apr 17, 2017

I ran shallow renderer tests in www against the new ShallowRenderer. There aren't that many of them unfortunately (only 64) but all pass except:

  • OCCreditCardForm-test.react.js which uses Enzyme.
  • P2PMemoField.react-test.js which uses internals (renderer._instance._instance). I'll fix this later as part of syncing the new shallow renderer to www.
  • P2PProductItemChatThreadBanner.react-test.js which uses ReactLayeredComponentMixin_DEPRECATED. Specifically this one fails because it calls ReactDOM.renderSubtreeIntoContainer which errors because ReactInstanceMap doesn't contain the shallow instance. I think it's reasonable to not support this test with the new renderer- particularly since we were hoping to decouple the shallow renderer from DOM.

I also tried plugging the new renderer into a local copy of Enzyme but didn't spend much time on it once I realized there was some internal coupling (eg ShallowWrapper.instance = () => this.renderer._instance ? this.renderer._instance._instance : null). I believe the Enzyme team has plans to implement their own, Enzyme-compatible shallow renderer for the next major version so I think it's okay to leave this off for now.

In the meanwhile I've reached out to Leland and made him aware of some of the issues I noticed. I think he's going to take a look at this branch and we'll chat some more about it.

@bvaughn bvaughn changed the title Shallow renderer and test utils bundles [WIP] Shallow renderer and test utils bundles Apr 17, 2017
Brian Vaughn added 4 commits April 17, 2017 22:45
Files now import directly from prop-types/checkPropTypes
* Moved test-utils into react-dom package
* Added test-utils to package.json 'files' list
* Added ReactTestUtilsFBEntry module (though it doesn't really do anything at the moment)
* Replaced ReactDOM references with react-dom to avoid duplicating code
…ific entry point. Throw explicit error if imported in prod mode
@bvaughn
Copy link
Contributor Author

bvaughn commented Apr 18, 2017

(e.g. would be good to run this against at least a few third party projects that use shallow rendering)

All 617 Enzyme unit tests pass for react/react-dom 15 + the new shallow renderer if I make the following patch:

diff --git a/src/ShallowWrapper.js b/src/ShallowWrapper.js
index bd23106..ade4e73 100644
--- a/src/ShallowWrapper.js
+++ b/src/ShallowWrapper.js
@@ -194,7 +194,11 @@ class ShallowWrapper {
     if (this.root !== this) {
       throw new Error('ShallowWrapper::instance() can only be called on the root');
     }
-    return this.renderer._instance ? this.renderer._instance._instance : null;
+    const instance = this.renderer._instance;
+
+    return instance && instance._instance
+      ? instance._instance
+      : instance;
   }
 
   /**

The above patch accounts for the fact that we no longer auto-wrap functional components in a class component.

Enzyme isn't compatible with React 16 yet because there are a lot of hooks into internals. But this improves my confidence about the new shallow renderer and its backwards compatibility (at least from an external POV).

Don't require subclass constructors to pass this along to super
@gaearon
Copy link
Collaborator

gaearon commented Apr 18, 2017

To be clear I don't think it's as important that Enzyme's own tests pass (which may rely on internals too much) since it's bound for major anyway. But we should test it on large projects using shallow rendering (via Enzyme or not).

@bvaughn
Copy link
Contributor Author

bvaughn commented Apr 18, 2017

Uh, right. But Enzyme's own tests seem like a pretty useful metric given their volume. If the new renderer passes all of their tests- that's a good sign, no?

I'm having trouble testing it on projects using shallow renderer because none of them have updated to be React 16 compatible yet.

@koba04
Copy link
Contributor

koba04 commented Apr 19, 2017

What do you think ShallowRender has a new API like getPublicInstance, which returns a public instance?
Currently, Enzyme depends on React internal to get a public instance.

It would be nice if it is accomplished without depending on React internals.
To do that, Enzyme needs some works because Enzyme is using a public instance even if it's a Stateless Functional Components, but it can be fixed.

@bvaughn
Copy link
Contributor Author

bvaughn commented Apr 19, 2017

What do you think ShallowRender has a new API like getPublicInstance, which returns a public instance? Currently, Enzyme depends on React internal...

Yeah, I had to patch ShallowWrapper to test it with the new shallow renderer. :grimace:

AirBnb has plans to rewrite Enzyme internals for the next release though. The idea is for libraries like React (and similar ones- Preact, Inferno, etc) to provide an abstraction layer / adapter that Enzyme consumes rather than reaching into internals. The Enzyme team can then create adapters on their side (eg one for each major version of React) so their core can be cleaned up. (This file has since been deleted, or maybe moved, but I think it explains the general idea.)

That's a bit orthogonal to this PR though probably. At the moment, I don't think they have any plans of actually using this new shallow renderer. It's for non-Enzyme users (including Facebook's non-Enzyme tests).

@bvaughn
Copy link
Contributor Author

bvaughn commented Apr 19, 2017

By the way, Dan- thanks for the unblock approval. I'm going to spend a little more time walking through the 30 failing tests in www and writing down the reasons why they're failing (patching things up if possible) to make our later sync easier. I hope to merge this PR sometime tomorrow.

@gaearon
Copy link
Collaborator

gaearon commented Apr 19, 2017

Sounds great!

@bvaughn
Copy link
Contributor Author

bvaughn commented Apr 19, 2017

All www tests pass now! Added a few more tests for edge-cases I found while running through www tests. Soon as Circle gives it a green check, I'll merge.

@bvaughn
Copy link
Contributor Author

bvaughn commented Apr 19, 2017

For posterity (or future-me) here's the list of mods I made in www while testing:

  1. Add 'prop-types' and 'create-react-class' packages to html/shared/node_modules
  2. Add magicdirs entries for the new bundles and the above NPM packages so Haste could find them.
  3. Patch Enzyme's ShallowWrapper.instance() slightly to account for the fact that the new renderer doesn't wrap function components in a class component.
  4. Noop mock unstable_renderSubtreeIntoContainer since any calls to this method would fail due to dependencies on ReactInstanceMap which isn't used by the new shallow renderer. (This was required for tests like P2PProductItemChatThreadBanner.react-test.js that use ReactLayeredComponentMixin_DEPRECATED.)
  5. Updated ReactDOMServerStack-dev to read checkPropTypes from prop-types/checkPropTypes rather than React.checkPropTypes. (Enzyme invokes this using renderToStaticString.) Also updated ReactARTStack-dev in a similar fashion.
  6. Replaced renderer._instance._instance with renderer.getMountedInstance() in P2PMemoField.react-test
  7. Mock XControllerURIBuilder.prototype.setInt.mockReturnThis() for TestConsoleLoader-test.react.js

I'll submit a WIP diff for the above changes internally as well for when we eventually sync a newer alpha to www.

@bvaughn bvaughn merged commit 66f2097 into master Apr 19, 2017
@bvaughn bvaughn deleted the react-dom-test-utils-shallow-renderer branch April 19, 2017 23:45
@koba04
Copy link
Contributor

koba04 commented Apr 20, 2017

Great work! 🎉

@bvaughn
Copy link
Contributor Author

bvaughn commented Apr 20, 2017

Thanks so much for the helpful info you provided @koba04 !

@gaearon
Copy link
Collaborator

gaearon commented Apr 20, 2017

Great work!

Copy link

@oguzgelal oguzgelal left a comment

Choose a reason for hiding this comment

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

Whoops, meant to add a single line comment


if (process.env.NODE_ENV === 'production') {
throw Error('shallow renderer is not available in production mode.');
} else {

Choose a reason for hiding this comment

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

@gaearon @bvaughn I'm having a problem with these lines: I'm using enzyme and it uses shallow rendering, for tests, while NODE_ENV is set to production. So my tests are failing because of this.

@gaearon
Copy link
Collaborator

gaearon commented Nov 4, 2017

I added prod builds in 16.1. You can try 16.1.0-beta now.

@oguzgelal
Copy link

Hmm. Upgraded to 16.1.0-beta but still having the issue.

@gaearon
Copy link
Collaborator

gaearon commented Nov 4, 2017

This code doesn't exist in it.

https://unpkg.com/[email protected]/shallow.js

If you still see the issue you have not upgraded. Or forgot to update some packages.

@oguzgelal
Copy link

Ah you're right, yes I updated react not react-test-renderer. Thanks!

NMinhNguyen referenced this pull request in enzymejs/react-shallow-renderer Jan 29, 2020
Shallow renderer and test utils bundles

Adds new bundles introduced with React 15.5 release to master (and 16 alpha)

react-dom/test-utils:

This new bundle contains what used to be react-addons-test-utils. This bundle shares things from react-dom rather than duplicates them.

A temporary createRenderer method has been left behind as a way to access the new shallow renderer. This is for the ReactNative release cycle only and should be going away before the final release.

react-test-renderer/shallow:

This new shallow renderer is almost entirely stand-alone (in that it doesn't use the React reconciler or scheduler). The only touch points are ReactElement and prop/context validation. This renderer is stack and fiber compatible.
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.

5 participants