Add toTree() method to stack and fiber TestRenderer #8931

lelandrichardson · 2017-02-05T18:53:33Z

This PR adds a toTree() method to ReactTestRenderer. This method is a result of the discussions from enzymejs/enzyme#742. Related discussion also in #8808.

This API will help us accomplish the goal of enzyme not needing to refer to any react internals at all.

This PR isn't complete, but I think it's a good start. I was hoping for some feedback on how to get this to the finish line. The main things I can think of are:

Need to add flow type definitions for this new code
Should we add documentation for this method?
Should we try to write toJSON() as a function that uses toTree() under the hood in order to reduce amount of code?
is toTree() the right name?
Is the code here correct? I wasn't quite sure w/ the fiber renderer if I should be using .child or .progressedChild or .return, etc.
For some reason when I was using Jest/Jasmine's .toEqual matcher, it was succeeding at times where it shouldn't, which really concerned me. I ended up changing expect(foo).toEqual(bar) to expect(prettyFormat(foo)).toEqual(prettyFormat(bar) in order to make sure things were working properly.
The way I'm nulling out instance on the nodes in tests is kind of wonky
Should we or should we not include children in the props object of these elements? I think we should now that I think about it more, but we might have to do more hacks to make the tests easier again.

aweary

This looks great @lelandrichardson, I'm really excited to hopefullly get this in! Left some feedback/questions. Also I think we need to add toTree to the ReactTestEmptyComponent

aweary · 2017-02-05T20:51:48Z

src/renderers/shared/stack/reconciler/ReactCompositeComponent.js

+      instance: this._instance,
+      rendered: this._renderedComponent.toTree(),
+    };
+  },


Historically we've avoided adding/using test-renderer specific APIs to ReactCompositeComponent (reference: #7649 (comment)) Could we derive this tree from the instance inside the stack test renderer like you did with Fiber?

Maybe it could be abstracted into a module like ReactInstanceToTree or something like that?

hmm. This makes sense as a rule. I'm not sure what the cleanest way to do this is but I'll give it a shot

aweary · 2017-02-05T20:53:59Z

src/renderers/testing/ReactTestRendererFiber.js

+    default:
+      throw new Error(
+        `toTree() does not yet know how to handle nodes with tag=${node.tag}.`
+      );


We'll want to use the invariant module for error handling to remain consistent.

You can just do invariant(false, '[message]')

aweary · 2017-02-05T20:56:55Z

src/renderers/testing/stack/ReactTestMount.js

+  // eslint-disable-next-line no-unused-vars
+  const { children, ...propsWithoutChildren } = element.props;
+  return {
+    nodeType: ReactNodeTypes.getType(element),


ReactNodeTypes returns an integer representing the type, but in the spec we define nodeType as a string. Should we update the spec and standardize the numbering, e.g., 0 always refers to a host node?

I thought about this. I feel like there are two ways of looking at this, and I'm not sure which one is more correct:

we just export a nodeType that makes the most sense based on the data structure available and not relying on the knowledge of the RST node spec.

we export a nodeType that is identified by the RST node spec.

The former is easier in terms of the react implementation and makes the "enzyme adapter" more complicated. The latter makes the enzyme adapter simpler, but means that react has to have some knowledge of this "RST node spec".

I'm open to either. Now that I'm saying it out loud, option 2 seems a bit better to me.

lelandrichardson · 2017-02-05T23:13:01Z

@aweary for ReactTestEmptyComponent, how would I test that it is working since the tests i've written so far are apparently working fine without it?

gaearon · 2017-02-05T23:18:32Z

src/renderers/testing/ReactTestRendererFiber.js

+function toTree(node) {
+  switch (node.tag) {
+    case HostRoot: // 3
+      return toTree(node.progressedChild);


I think you want .child because .progressedChild refers to something different.

for some reason, .node on HostRoot was always null.

What is .node?

gah, sorry. i meant .child.

If i change this line to return toTree(node.child);, this breaks because node.child is null but node.progressedChild is not. :/

That might be a bug either in Fiber or the test renderer. You should definitely not use progressedChild.

ok good to know. Any advice on what i should dive into in order to try and track down why this is happening?

The problem is that you're looking at the wrong root. There's two. We might also want to switch which one createContainer returns.

For now, use stateNode to store the "root".

var root = TestRenderer.createContainer(container).stateNode;

This will not be a Fiber object so every time you use it you need to use root.current to get the current Fiber.

There are two Fibers per root and this will ensure that you get the right one.

aweary · 2017-02-06T04:47:44Z

@aweary for ReactTestEmptyComponent, how would I test that it is working since the tests i've written so far are apparently working fine without it?

@lelandrichardson you should be able to test it by rendering null or false I believe. ReactTestEmptyComponent is set via injection and ReactEmptyComponent uses it.

lelandrichardson · 2017-02-06T18:51:31Z

@sebmarkbage I just added a commit which refactors the fiber test renderer to use stateNode and root.current everywhere. Tests pass. Let me know if it looks right to you

sebmarkbage · 2017-02-06T19:38:47Z

@lelandrichardson Mind rebasing on top of #8934 ?

sebmarkbage · 2017-02-06T19:39:54Z

src/renderers/testing/ReactTestRendererFiber.js

@@ -248,12 +307,12 @@ var ReactTestFiberRenderer = {
      createNodeMock,
      tag: 'CONTAINER',
    };
-    var root = TestRenderer.createContainer(container);
-    TestRenderer.updateContainer(element, root, null, null);
+    var root = TestRenderer.createContainer(container).stateNode;


After #8934 this doesn't need .stateNode anymore.

lelandrichardson · 2017-02-08T16:47:12Z

@sebmarkbage rebased. Any thoughts on what the next steps for this PR might be? We could refactor toJSON to use toTree under the hood perhaps? I'll also add some test cases with null returned value for some components

sebmarkbage · 2017-02-08T17:50:53Z

@lelandrichardson I think just fixing the unit tests would be enough to land this. According to CI you have some failing tests.

We can refactor toJSON as a follow up. Might be harder if there are breaking changes.

lelandrichardson · 2017-02-08T18:04:06Z

OK. Looks like it's just some flow errors. I'll add some flow types to the new code

lelandrichardson · 2017-02-09T17:01:55Z

@sebmarkbage strange. If i click "Details" for circle CI it shows that everything has passed for the latest commit of this branch. Am i missing something?

aweary · 2017-02-09T17:32:19Z

@lelandrichardson you have to re-run the fiber test script and commit those changes since you added a new test toTree() handles null rendering components

lelandrichardson · 2017-02-09T17:50:03Z

@aweary ah, makes sense

lelandrichardson · 2017-02-09T19:02:20Z

@aweary @sebmarkbage tests passing! Let me know if there's anything more i need to do!

sebmarkbage

lgtm

sebmarkbage · 2017-02-09T19:24:58Z

src/renderers/testing/ReactTestRendererFiber.js

@@ -16,8 +16,19 @@
 var ReactFiberReconciler = require('ReactFiberReconciler');
 var ReactGenericBatching = require('ReactGenericBatching');
 var emptyObject = require('emptyObject');
+var ReactTypeOfWork = require('ReactTypeOfWork');


Can you confirm that this works in the npm build? I.e. grunt build puts this file in the npm package. I've had trouble in RN when this isn't reachable from within the npm build?

sorry.... can you explain what you want me to try here? just run npm run build?

Please verify this file actually ends up in build/packages/react-test-renderer after you've run npm run build. Whether it does or not depends on where ReactTypeOfWork is located. Explanation for our folder structure: https://facebook.github.io/react/contributing/codebase-overview.html#shared-code.

In other words, after npm run build try cd-ing into build/packages/react-test-renderer and verify you can require() it without crashing.

I've confirmed that build/packages/react-test-renderer/lib/ReactTypeOfWork.js exists

gaearon · 2017-02-09T20:55:09Z

✨

lelandrichardson · 2017-02-09T21:31:51Z

Really happy to see this land! I'm looking to do a followup PR to add in a isHost configuration option which would allow us to implement something semantically equivalent to the ShallowRenderer. I was not quite sure how to implement something like this on the fiber side. Would any of you have any hints to provide here regarding where I could start? cc @sebmarkbage @gaearon ?

gaearon · 2017-02-09T21:43:54Z

Not sure it's the best way but you'd likely want to make hostConfig available in ReactFiber so that you can delegate to it (hostConfig.isHost) when deciding whether something HostComponent or not. Host config is the object with methods that you pass to ReactFiberReconciler to create the renderer. Does this help?

lelandrichardson · 2017-02-09T21:50:46Z

@gaearon i think that might give me enough to start playing around. Thanks!

sebmarkbage · 2017-02-09T22:10:15Z

What @gaearon said about hostConfig but please don't make a isHost and turn it into a host component in the internals. Instead, make a mockComponent(type : Function) -> Function and then you can make a call to that when we access the type of a custom component. That way any user component can be replaced with any other user component and everything just works as is.

sebmarkbage · 2017-02-09T22:34:12Z

Then in user space you can build isHost:

mockComponent = (component) => {
  if (isHost(component)) {
    const x = (props) => props.children;
    x.displayName = component.displayName || component.name;
    return x;
  }
  return component;
};

This just works because Fiber supports fragments.

gaearon · 2017-02-09T23:39:01Z

This is what I proposed in enzymejs/enzyme#742 (comment), is my understanding right? One thing I'm not sure about is how exactly to model shallow rendering with this (I think my comment actually gets it wrong). For shallow rendering the type is not enough because you want the (composite) root to become expanded but any composite children to get mocked. So either some notion of depth, or element equality, is necessary. WDYT?

lelandrichardson · 2017-02-10T00:24:39Z

@gaearon my understanding was that for shallow rendering we would just make any composite component a "mocked" host component that just renders children. Since children would be elements (of both composite and host types), the hosts would work like normal and the composites would also be mocked, so their corresponding render methods would never get called (only the mocked render would). This should get us the tree we want, I believe.

sebmarkbage · 2017-02-10T00:30:09Z

A composite may not be using children as the prop name for its children.

Additionally, that wouldn't render the very root which is the problem @gaearon is concerned about.

You really want to render null for everything but the first one and then only assert on the props of the top level rendered object.

var isFirst = true;
function mockComponent(c) {
  if (isFirst) {
    isFirst = false;
    return c;
  }
  return function() { return null; };
}

ReactShallowRenderer.render = function(children) {
  isFirst = true;
  Renderer.updateContainer(root, children);
}

We could also just write a shallow renderer from scratch. It's not hard.

gaearon · 2017-02-10T00:37:13Z

Hah, I didn't think about local variables 😄 . This looks like it would work if we forbid passing a fragment or a host component to the shallow renderer (since host could have multiple composite children). Which kinda makes sense anyway.

I feel like mocking without hijacking the module system is generally valuable so I'd like to see this explored within our existing system in Fiber, without a separate rewrite. Although I'm not sure if this is going to have unwanted side effects like calling componentDidMount without the DOM. OTOH unintentional differences from our main composite logic (and related bugs) were also annoying in the past. Need to strike a good balance here.

YERFINKOGOYA

Yrs

facebook-github-bot added GH Review: review-needed CLA Signed labels Feb 5, 2017

aweary suggested changes Feb 5, 2017

View reviewed changes

gaearon reviewed Feb 5, 2017

View reviewed changes

lelandrichardson force-pushed the lmr--test-renderer-to-tree branch from 6129a08 to f847289 Compare February 6, 2017 00:06

lelandrichardson force-pushed the lmr--test-renderer-to-tree branch from a565b14 to c596e19 Compare February 6, 2017 18:55

sebmarkbage reviewed Feb 6, 2017

View reviewed changes

lelandrichardson added 4 commits February 7, 2017 22:04

Add toTree() method to stack and fiber TestRenderer

b7e79d8

Address PR feedback

cf10eb3

Refactor TestRenderer to use correct root

cf8a7a7

Rebase off master and fix root node references

cc80a77

lelandrichardson force-pushed the lmr--test-renderer-to-tree branch from c596e19 to cc80a77 Compare February 8, 2017 06:16

lelandrichardson added 3 commits February 8, 2017 20:46

Add flow types

bf7339d

Add test for null rendering components

43d8f4a

Remove last remaining lint error

878f629

Add missing test

41a557b

sebmarkbage approved these changes Feb 9, 2017

View reviewed changes

sebmarkbage reviewed Feb 9, 2017

View reviewed changes

aweary approved these changes Feb 9, 2017

View reviewed changes

gaearon merged commit 869c779 into facebook:master Feb 9, 2017

lelandrichardson deleted the lmr--test-renderer-to-tree branch February 9, 2017 21:32

lelandrichardson mentioned this pull request Feb 12, 2017

Add createComponentMock option to test renderer #8982

Closed

gaearon mentioned this pull request Apr 16, 2017

Shallow renderer and test utils bundles #9426

Merged

YERFINKOGOYA reviewed Apr 17, 2017

View reviewed changes

aweary mentioned this pull request Apr 25, 2017

Test renderer traversal #7516

Closed

aweary mentioned this pull request May 29, 2017

Convert elements to test objects for toMatchObject in Jest #9747

Closed

Add toTree() method to stack and fiber TestRenderer #8931

Add toTree() method to stack and fiber TestRenderer #8931

Conversation

lelandrichardson commented Feb 5, 2017 • edited Loading

aweary left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lelandrichardson commented Feb 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweary commented Feb 6, 2017

lelandrichardson commented Feb 6, 2017

sebmarkbage commented Feb 6, 2017

sebmarkbage Feb 6, 2017 • edited Loading

Choose a reason for hiding this comment

lelandrichardson commented Feb 8, 2017

sebmarkbage commented Feb 8, 2017 • edited Loading

lelandrichardson commented Feb 8, 2017

lelandrichardson commented Feb 9, 2017

aweary commented Feb 9, 2017 • edited Loading

lelandrichardson commented Feb 9, 2017

lelandrichardson commented Feb 9, 2017

sebmarkbage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Feb 9, 2017

lelandrichardson commented Feb 9, 2017

gaearon commented Feb 9, 2017

lelandrichardson commented Feb 9, 2017

sebmarkbage commented Feb 9, 2017

sebmarkbage commented Feb 9, 2017

gaearon commented Feb 9, 2017

lelandrichardson commented Feb 10, 2017

sebmarkbage commented Feb 10, 2017 • edited Loading

gaearon commented Feb 10, 2017

YERFINKOGOYA left a comment

Choose a reason for hiding this comment

lelandrichardson commented Feb 5, 2017 •

edited

Loading

aweary left a comment •

edited

Loading

sebmarkbage Feb 6, 2017 •

edited

Loading

sebmarkbage commented Feb 8, 2017 •

edited

Loading

aweary commented Feb 9, 2017 •

edited

Loading

sebmarkbage commented Feb 10, 2017 •

edited

Loading