Add createComponentMock option to test renderer #8982

lelandrichardson · 2017-02-12T22:52:43Z

This PR is a followup to #8931, and adds a createComponentMock option to the test renderer when using fiber. The function is expected to return the correct type (mocked or not) for the provided fiber. This gets called on functional components and class components, but not hosts.

I've added corresponding tests that show how one could use this property to emulate shallow rendering, as well as what I believe is an even more practical usage, simple mocking of components that have implementations difficult to test.

The implementation of this uses an additional optional mockComponent function that is added to the HostConfig when creating the reconciler. This required that we pass the host config into a couple of additional places in the fiber codebase.

There are still some outstanding questions:

Do we want to allow createComponentMock to return null in the case of not mocking, or keep it as is where it just returns type?
Is mockComponent the right name to use?
createComponentMock gets called with a Fiber. Do we instead want to only pass it type and tag, or some transformation of tag? Right now user land code will have to know if the mock should be a functional component or a class component. Alternatively, we could have two methods, one for functional component mocking, and another for class component mocking.

cc @gaearon @sebmarkbage @aweary @iamdustan

iamdustan · 2017-02-13T12:34:35Z

src/renderers/testing/ReactTestRendererFiber.js

@@ -220,6 +232,9 @@ var defaultTestOptions = {
  createNodeMock: function() {
    return null;
  },
+  createComponentMock: function(component: Fiber) {


Something I was thinking about when doing the initial test renderer implementation is if the createNodeMock had to be part of the HostConfig long-term. I think it did at the time for backwards-compatibility, but ideally could be done without extending the HostConfig. Since this is a new API can you think of a way to have this done entirely in renderer-land?

iamdustan · 2017-02-13T12:43:02Z

src/renderers/shared/fiber/ReactFiberReconciler.js

@@ -100,6 +100,8 @@ export type HostConfig<T, P, I, TI, PI, C, CX, PL> = {
  resetAfterCommit() : void,

  useSyncScheduling ?: boolean,
+
+  mockComponent ?: Function,


I was guided towards making the HostConfig shape strict in a prior change. May be applicable here as well.

See #8628 (comment)

acdlite · 2017-02-13T20:00:46Z

Do we want to allow createComponentMock to return null in the case of not mocking, or keep it as is where it just returns type?

Don't have a strong preference other than consistency with createNodeMock.

Is mockComponent the right name to use?

Seems fine to me.

createComponentMock gets called with a Fiber. Do we instead want to only pass it type and tag, or some transformation of tag? Right now user land code will have to know if the mock should be a functional component or a class component. Alternatively, we could have two methods, one for functional component mocking, and another for class component mocking.

In general I'm not a fan of leaking the Fiber to the public interface. Seems like it's not necessary for the typical use case; if there are use cases that come up in the future we can reconsider then.

The answer might be obvious, but could you explain why it's important to distinguish between function and class mocks? Why are the mocks different?

If it is necessary to distinguish between the two, we should make sure we don't call createComponentMock until IndeterminateComponent resolves to either FunctionalComponent or ClassComponent.

Is the intention to support this in production mode, too? Are there use cases where mocking in production is necessary? Ideally it'd be nice to strip out any mocking code when not in DEV.

acdlite · 2017-02-13T20:04:46Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

@@ -450,7 +457,9 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
      'An indeterminate component should never have mounted. This error is ' +
      'likely caused by a bug in React. Please file an issue.'
    );
-    var fn = workInProgress.type;
+    var fn = hasMockingBehavior
+      ? mockComponent(workInProgress, hostContext.getRootHostContainer())


Would it not be simpler to mock the component just once, on mount?

workInProgress.type = mockComponent(...);

That way this would be the only place we'd ever have to branch on hasMockingBehavior

@acdlite one problem with this option is that then the resulting data structure in toTree() would have type be set to the mock, rather than the original value

toTree() is part of the test renderer, so you could store the original type on the mocked type.

bvaughn · 2017-02-13T20:52:13Z

In general I'm not a fan of leaking the Fiber to the the public interface. Seems like it's not necessary for the typical use case; if there are use cases that come up in the future we can reconsider then.

Agreed 👍

gaearon · 2017-02-13T20:54:30Z

I wonder if this could also be helpful for hot reloading..

acdlite · 2017-02-13T21:08:33Z

@gaearon Ooh yeah, should be!

lelandrichardson · 2017-02-13T23:41:48Z

@acdlite re:

The answer might be obvious, but could you explain why it's important to distinguish between function and class mocks? Why are the mocks different?

They are different because the place in which i'm mocking them, they've already turned into either a ClassComponent or a FunctionalComponent, so the actual mocks need to be the one or the other or else it would cause an exception

acdlite · 2017-02-14T19:17:40Z

so the actual mocks need to be the one or the other or else it would cause an exception

Ok, so if that's the only reason, then that's why I think it would be simpler to override the type with the mocked type, and to store the original type on the mock itself.

How does that sound?

lelandrichardson · 2017-02-14T20:20:10Z

@acdlite yeah, I think that's probably the right approach. Let me take another pass at this! 👍

acdlite · 2017-02-14T20:23:05Z

@lelandrichardson Awesome, thanks!

lelandrichardson · 2017-03-25T07:56:13Z

@acdlite @trueadm I finally got some time to work on this...

I addressed a couple of things:

the Fiber data structure is no longer leaked to the consumer. I am now just passying { type, props } to the createComponentMock function
I am now running mockComponent only once in the beginWork method, and leaving all other functions unchanged. This also allows for users to mock a component as an SFC or class component and have it not affect anything.
I have mocking behavior and code fenced behind __DEV__ checks
Because I am now setting fiber.type to the mocked version of the component, I needed some way to hold onto the original unmocked version. It turns out setting a property on the mock itself is not enough here, as the same mock might end up being used for several different fibers. After thinking about this for a while, it seemed like the most sensible way to deal with this was to instead store a reference to the type on the fiber itself. This seems like a pretty invasive change for what ought to be a non-invasive feature. This is lessened somewhat by the fact that it is a property that will only be there in non __DEV__ environments, but I'm still not super happy about it. If anyone has any suggestions I'd love to hear them.

trueadm · 2017-03-27T11:38:13Z

@lelandrichardson For point 4., rather than adding a property on the fiber, could you not instead use a Map to store the unmocked version, using the fiber itself as the key? Thank you for the work you've put in on this :)

sebmarkbage · 2017-03-27T15:43:36Z

@trueadm The problem with using a fiber as a key in a Map is that there's two fibers, and potentially more. We could rely on there being just two Fibers (currently true) and storing each in a Map, or look up both. However, we might want more of them or throw them away, so it might be even more invasive than just another field.

sebmarkbage · 2017-03-27T16:05:57Z

src/renderers/testing/ReactTestRendererFiber.js

@@ -217,6 +220,29 @@ var TestRenderer = ReactFiberReconciler({
    setTimeout(fn, 0, {timeRemaining: Infinity});
  },

+  mockComponent(component: Fiber, rootContainer: Container) {


We should not be exposing the Fiber data structure to renderers. I have some refactoring ideas where this will break. Instead, pass the individual pieces as arguments.

sebmarkbage · 2017-03-27T16:07:12Z

src/renderers/testing/ReactTestRendererFiber.js

+    );
+    if (mockedFn !== component.type) {
+      component._unmockedType = component.type;
+      component.type = mockedFn;


We should never mutate the Fiber here. We should instead return the mock and let the algorithm initialize it in the appropriate place.

sebmarkbage · 2017-03-27T16:08:07Z

src/renderers/testing/ReactTestRendererFiber.js

+      component.type = mockedFn;
+      // force the fiber to be indeterminate so that users can mock a class component
+      // into a functional component and vice versa
+      component.tag = IndeterminateComponent;


This indicates to me that we're not overriding it soon enough. Maybe we need to do the override earlier?

This also doesn't allow you to mock it to a string to render a host node for example.

sebmarkbage · 2017-03-27T16:22:20Z

src/renderers/testing/ReactTestRendererFiber.js

      };
    case HostComponent: // 5
      return {
        nodeType: 'host',
        type: node.type,
        props: {...node.memoizedProps},
        instance: null, // TODO: use createNodeMock here somehow?
-        rendered: nodeAndSiblingsArray(node.child).map(toTree),
+        rendered: flatten(nodeAndSiblingsArray(node.child).map(toTree)),


Can we make this faster? This is super inefficient atm. We have plenty of examples in the codebase of linked list tree traversal without the intermediate arrays.

But why are only host components flattened and not other types?

sebmarkbage · 2017-03-27T16:34:21Z

@lelandrichardson It is kind of weird to be able to mock based on props, since props can change over the life-time of an instance.

What if it changes between a functional and class component? What if it changes between two different classes with different state types? If so, it should probably throw away the state as if it was a different type.

Would it be enough to mock based on the type alone? If so, a WeakMap using the type as a key would be enough.

lelandrichardson · 2017-03-29T01:41:17Z

Would it be enough to mock based on the type alone? If so, a WeakMap using the type as a key would be enough.

As I think about this more, I think this is correct. It also maybe makes sense to ensure that createComponentMock only gets called once per type, so you don't end up creating a new mock every time, which could maybe cause some strange regressions in your code or something.

lelandrichardson · 2017-03-29T01:42:37Z

@sebmarkbage are we okay with relying on WeakMap being available? Should we fall back to Map in the case that it's not?

sebmarkbage · 2017-03-29T02:11:56Z

As long as we only depend on WeakMap in this renderer and not every renderer, I'm fine with just assuming it is there.

bvaughn · 2017-04-14T02:34:25Z

I've added corresponding tests that show how one could use this property to emulate shallow rendering, as well as what I believe is an even more practical usage, simple mocking of components that have implementations difficult to test.

In case anyone's interested- building on Leland's idea, I started working on a shallow renderer in PR #9426.

gaearon · 2017-10-04T23:32:00Z

Seems like we didn't really go anywhere with this. Thanks for PR though!

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

iamdustan reviewed Feb 13, 2017

View reviewed changes

acdlite reviewed Feb 13, 2017

View reviewed changes

trueadm mentioned this pull request Feb 27, 2017

[Fiber] Investigation: what internals have direct dependencies on Stack modules? #9069

Closed

sebmarkbage removed the GH Review: review-needed label Mar 1, 2017

sebmarkbage mentioned this pull request Mar 1, 2017

[Fiber] Umbrella for remaining features / bugs #7925

Closed

lelandrichardson force-pushed the lmr--create-component-mock-option branch from ee70416 to 154cc6b Compare March 25, 2017 07:47

lelandrichardson force-pushed the lmr--create-component-mock-option branch from 154cc6b to ba99f9e Compare March 25, 2017 07:59

Add createComponentMock option to test renderer

e35416f

lelandrichardson force-pushed the lmr--create-component-mock-option branch from ba99f9e to e35416f Compare March 25, 2017 08:06

bvaughn mentioned this pull request Mar 25, 2017

Error boundaries: Recover from errors thrown in render #2461

Closed

sebmarkbage requested changes Mar 27, 2017

View reviewed changes

sebmarkbage reviewed Mar 27, 2017

View reviewed changes

gaearon mentioned this pull request Apr 13, 2017

React 16 Umbrella ☂️ (and 15.5) #8854

Closed

gaearon closed this Oct 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add createComponentMock option to test renderer #8982

Add createComponentMock option to test renderer #8982

lelandrichardson commented Feb 12, 2017

iamdustan Feb 13, 2017

iamdustan Feb 13, 2017

acdlite commented Feb 13, 2017 •

edited

Loading

acdlite Feb 13, 2017

acdlite Feb 13, 2017

lelandrichardson Feb 13, 2017

acdlite Feb 14, 2017

bvaughn commented Feb 13, 2017

gaearon commented Feb 13, 2017

acdlite commented Feb 13, 2017

lelandrichardson commented Feb 13, 2017

acdlite commented Feb 14, 2017

lelandrichardson commented Feb 14, 2017

acdlite commented Feb 14, 2017

lelandrichardson commented Mar 25, 2017

trueadm commented Mar 27, 2017 •

edited

Loading

sebmarkbage commented Mar 27, 2017

sebmarkbage Mar 27, 2017

sebmarkbage Mar 27, 2017

sebmarkbage Mar 27, 2017

sebmarkbage Mar 27, 2017

sebmarkbage commented Mar 27, 2017

lelandrichardson commented Mar 29, 2017

lelandrichardson commented Mar 29, 2017

sebmarkbage commented Mar 29, 2017

bvaughn commented Apr 14, 2017

gaearon commented Oct 4, 2017

Add createComponentMock option to test renderer #8982

Add createComponentMock option to test renderer #8982

Conversation

lelandrichardson commented Feb 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Feb 13, 2017 • 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

bvaughn commented Feb 13, 2017

gaearon commented Feb 13, 2017

acdlite commented Feb 13, 2017

lelandrichardson commented Feb 13, 2017

acdlite commented Feb 14, 2017

lelandrichardson commented Feb 14, 2017

acdlite commented Feb 14, 2017

lelandrichardson commented Mar 25, 2017

trueadm commented Mar 27, 2017 • edited Loading

sebmarkbage commented Mar 27, 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

sebmarkbage commented Mar 27, 2017

lelandrichardson commented Mar 29, 2017

lelandrichardson commented Mar 29, 2017

sebmarkbage commented Mar 29, 2017

bvaughn commented Apr 14, 2017

gaearon commented Oct 4, 2017

acdlite commented Feb 13, 2017 •

edited

Loading

trueadm commented Mar 27, 2017 •

edited

Loading