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

Implement createNodeMock for ReactTestRenderer #7649

Merged
merged 15 commits into from Sep 13, 2016

Conversation

Projects
None yet
8 participants
@aweary
Member

aweary commented Sep 2, 2016

This lets users pass in an optional testOptions object as the second argument to ReactTestRenderer.create that allows for custom mocking behavior. As of now that only includes createNodeMock, which accepts the current element and should return a mock ref object.

The testOptions is stored on the component instance so it doesn't leak between instances.

cc @gaearon @spicyj

Show outdated Hide outdated src/renderers/testing/ReactTestMount.js
@@ -20,6 +20,10 @@ var getHostComponentFromComposite = require('getHostComponentFromComposite');
var instantiateReactComponent = require('instantiateReactComponent');
var invariant = require('invariant');
type TestRendererMockConfig = {
getMockRef: (element: ReactElement) => Object,

This comment has been minimized.

@aweary

aweary Sep 2, 2016

Member

Not sure how this would be typed, is there a better way to type an object that looks like a DOM node?

@aweary

aweary Sep 2, 2016

Member

Not sure how this would be typed, is there a better way to type an object that looks like a DOM node?

This comment has been minimized.

@gaearon

gaearon Sep 2, 2016

Member

Looks fine to me.

@gaearon

gaearon Sep 2, 2016

Member

Looks fine to me.

Show outdated Hide outdated src/renderers/testing/ReactTestRenderer.js
if (this._mockConfig) {
var { getMockRef } = this._mockConfig;
if (getMockRef) {
return getMockRef(this._currentElement);

This comment has been minimized.

@gaearon

gaearon Sep 2, 2016

Member

Can we make a default mockConfig implementation and always assume mockConfig exists?

@gaearon

gaearon Sep 2, 2016

Member

Can we make a default mockConfig implementation and always assume mockConfig exists?

This comment has been minimized.

@aweary

aweary Sep 2, 2016

Member

Should we have a default getMockRef implementation as well and just return this._mockConfig.getMockRef(this._currentElement)? Or just keep that check

@aweary

aweary Sep 2, 2016

Member

Should we have a default getMockRef implementation as well and just return this._mockConfig.getMockRef(this._currentElement)? Or just keep that check

This comment has been minimized.

@gaearon

gaearon Sep 3, 2016

Member

Yea, default impl is what I meant.

@gaearon

gaearon Sep 3, 2016

Member

Yea, default impl is what I meant.

Show outdated Hide outdated src/renderers/testing/__tests__/ReactTestRenderer-test.js
expect(log).toEqual([{}]);
});
it('allows an optional getMockRef function', function() {

This comment has been minimized.

@gaearon

gaearon Sep 2, 2016

Member

Let’s also test that we receive the correct element.

@gaearon

gaearon Sep 2, 2016

Member

Let’s also test that we receive the correct element.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 2, 2016

Coverage Status

Coverage increased (+0.01%) to 87.243% when pulling d7bd655 on Aweary:mock-dom-instance into a70acb3 on facebook:master.

Coverage Status

Coverage increased (+0.01%) to 87.243% when pulling d7bd655 on Aweary:mock-dom-instance into a70acb3 on facebook:master.

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 3, 2016

Member

FYI this has some issues with refs rendered by nested components, still working that out

Member

aweary commented Sep 3, 2016

FYI this has some issues with refs rendered by nested components, still working that out

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 5, 2016

Member

Hmm, I'm not sure how I can efficiently provide access to the mockConfig to children at an arbitrary depth. I can walk the render tree and attach it to each instance, but that's not very efficient.

Member

aweary commented Sep 5, 2016

Hmm, I'm not sure how I can efficiently provide access to the mockConfig to children at an arbitrary depth. I can walk the render tree and attach it to each instance, but that's not very efficient.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 5, 2016

Member

I haven't looked at the diff in detail but I think you can make it have a custom transaction (like server rendering does). Then you can read the config from the transaction since it's threaded all the way down.

An alternative is to put context on TopLevelWrapper but this is probably more hacky.

Member

gaearon commented Sep 5, 2016

I haven't looked at the diff in detail but I think you can make it have a custom transaction (like server rendering does). Then you can read the config from the transaction since it's threaded all the way down.

An alternative is to put context on TopLevelWrapper but this is probably more hacky.

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 5, 2016

Member

Yeah I tried TopLevelWrapper first, but it's not actually set on all instances. It only seems to be set on instances at the top of the render tree. I'll give the transaction approach a try.

Member

aweary commented Sep 5, 2016

Yeah I tried TopLevelWrapper first, but it's not actually set on all instances. It only seems to be set on instances at the top of the render tree. I'll give the transaction approach a try.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 5, 2016

Member

Context should propagate downwards too so it must be something else. Regardless transaction seems to be a better approach.

Member

gaearon commented Sep 5, 2016

Context should propagate downwards too so it must be something else. Regardless transaction seems to be a better approach.

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 5, 2016

Member

For _topLevelWrapper you can see in ReactTestMount that it's only set on the top-level _renderedComponent. So that might just be another separate issue we should address.

I'll look into the transactional approach today 👍

Member

aweary commented Sep 5, 2016

For _topLevelWrapper you can see in ReactTestMount that it's only set on the top-level _renderedComponent. So that might just be another separate issue we should address.

I'll look into the transactional approach today 👍

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 5, 2016

Member

Does _topLevelWrapper field matter? I don't think that field is important for context propagation. Context is passed down through ReactCompositeComponent and afaik this should work any number of composites down.

Member

gaearon commented Sep 5, 2016

Does _topLevelWrapper field matter? I don't think that field is important for context propagation. Context is passed down through ReactCompositeComponent and afaik this should work any number of composites down.

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 5, 2016

Member

Oh, no, sorry I was just responding to

An alternative is to put context on TopLevelWrapper but this is probably more hacky.

Member

aweary commented Sep 5, 2016

Oh, no, sorry I was just responding to

An alternative is to put context on TopLevelWrapper but this is probably more hacky.

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 5, 2016

Member

One issue I see with trying to use the transaction to propagate the mock config, is that ReactTestComponent.prototype.getPublicInstance doesn't seem to have access to any transaction object.

Member

aweary commented Sep 5, 2016

One issue I see with trying to use the transaction to propagate the mock config, is that ReactTestComponent.prototype.getPublicInstance doesn't seem to have access to any transaction object.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 5, 2016

Member

Let's pass it in there?

Member

gaearon commented Sep 5, 2016

Let's pass it in there?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 5, 2016

Member

Actually it should be easier to save mock config from transaction in mountComponent as an instance field. Then access it anytime. Would that work?

Member

gaearon commented Sep 5, 2016

Actually it should be easier to save mock config from transaction in mountComponent as an instance field. Then access it anytime. Would that work?

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 5, 2016

Member

Let's pass it in there?

ReactRef doesn't currently have access to transaction, but I suppose it could be threaded in from ReactReconcilier.

Actually it should be easier to save mock config from transaction in mountComponent as an instance field. Then access it anytime. Would that work?

Does it make sense to use the transaction as a host for instance-specific config? To make this work we'd have to call injectReconcileTransaction every time ReactTestRenderer.create is called, since mockConfig implementation can diff between calls.

Also, Is there a guarantee that transaction passed to mountComponent will always be the transaction type that we inject in create? If we have sequential create calls it seems like we may have a potential race condition where the next transaction type is injected before the first's mountComponent method is called.

Global injection makes this tricky 😄

Member

aweary commented Sep 5, 2016

Let's pass it in there?

ReactRef doesn't currently have access to transaction, but I suppose it could be threaded in from ReactReconcilier.

Actually it should be easier to save mock config from transaction in mountComponent as an instance field. Then access it anytime. Would that work?

Does it make sense to use the transaction as a host for instance-specific config? To make this work we'd have to call injectReconcileTransaction every time ReactTestRenderer.create is called, since mockConfig implementation can diff between calls.

Also, Is there a guarantee that transaction passed to mountComponent will always be the transaction type that we inject in create? If we have sequential create calls it seems like we may have a potential race condition where the next transaction type is injected before the first's mountComponent method is called.

Global injection makes this tricky 😄

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 5, 2016

Member

Maybe I misunderstand how it works but I thought transaction is created on every root mount. For example server rendering creates a transaction (passing makeStaticMarkup, a config just like getMockRef) on every renderToStringImpl call. I don't see any injection being done there, transaction is just passed as an argument to root reconciler call. Can we do the same here?

Member

gaearon commented Sep 5, 2016

Maybe I misunderstand how it works but I thought transaction is created on every root mount. For example server rendering creates a transaction (passing makeStaticMarkup, a config just like getMockRef) on every renderToStringImpl call. I don't see any injection being done there, transaction is just passed as an argument to root reconciler call. Can we do the same here?

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 5, 2016

Member

I'm probably the one misunderstanding, but from what I can see it injects the transaction class in ReactTestRenderer:

ReactUpdates.injection.injectReconcileTransaction(
  ReactTestReconcileTransaction
);

Then the actual transaction object is constructed from the object pool every time it's needed, which is on mount and on update

  var transaction = ReactUpdates.ReactReconcileTransaction.getPooled();
  var image = transaction.perform(
    mountComponentIntoNode,
    null,
    componentInstance,
    transaction
  );
  ReactUpdates.ReactReconcileTransaction.release(transaction);

but the way ReactServerRenderingTransaction works with makeStaticMarkup is that makeStaticMarkup is actually passed to the getPooled call every time the transaction is created. So even though the transaction is recreated from an injected global class, it gets that call-specific config every time. So I think we can just do the same here! I'll give that a try.

Member

aweary commented Sep 5, 2016

I'm probably the one misunderstanding, but from what I can see it injects the transaction class in ReactTestRenderer:

ReactUpdates.injection.injectReconcileTransaction(
  ReactTestReconcileTransaction
);

Then the actual transaction object is constructed from the object pool every time it's needed, which is on mount and on update

  var transaction = ReactUpdates.ReactReconcileTransaction.getPooled();
  var image = transaction.perform(
    mountComponentIntoNode,
    null,
    componentInstance,
    transaction
  );
  ReactUpdates.ReactReconcileTransaction.release(transaction);

but the way ReactServerRenderingTransaction works with makeStaticMarkup is that makeStaticMarkup is actually passed to the getPooled call every time the transaction is created. So even though the transaction is recreated from an injected global class, it gets that call-specific config every time. So I think we can just do the same here! I'll give that a try.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 5, 2016

Member

Dynamic injection is a mess. Sorry.

Member

gaearon commented Sep 5, 2016

Dynamic injection is a mess. Sorry.

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 5, 2016

Member

Yes it is, and no sorry needed 🤘 It's at the very least a fun challenge

Member

aweary commented Sep 5, 2016

Yes it is, and no sorry needed 🤘 It's at the very least a fun challenge

@aweary aweary closed this Sep 5, 2016

@aweary aweary reopened this Sep 5, 2016

@ghost ghost added the CLA Signed label Sep 5, 2016

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 5, 2016

Coverage Status

Coverage decreased (-0.03%) to 87.202% when pulling d7bd655 on Aweary:mock-dom-instance into a70acb3 on facebook:master.

Coverage Status

Coverage decreased (-0.03%) to 87.202% when pulling d7bd655 on Aweary:mock-dom-instance into a70acb3 on facebook:master.

@cpojer

This comment has been minimized.

Show comment
Hide comment
@cpojer

cpojer Sep 6, 2016

Contributor

I was wondering if we could have a top level getMockRef per test renderer instance that would get arguments based on the ref, like the current element and the name of the expected ref? For example it could be called with the instance of an <Input /> component that renders down to a <input ref="text" /> component. getMockRef would then be called with `getMockRef(InputInstance, 'text)' and would be called once per ref in the tree. I'm not sure if this makes sense, just figured I'll throw the idea out there.

Contributor

cpojer commented Sep 6, 2016

I was wondering if we could have a top level getMockRef per test renderer instance that would get arguments based on the ref, like the current element and the name of the expected ref? For example it could be called with the instance of an <Input /> component that renders down to a <input ref="text" /> component. getMockRef would then be called with `getMockRef(InputInstance, 'text)' and would be called once per ref in the tree. I'm not sure if this makes sense, just figured I'll throw the idea out there.

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 6, 2016

Member

I was wondering if we could have a top level getMockRef per test renderer instance that would get arguments based on the ref, like the current element and the name of the expected ref? For example it could be called with the instance of an component that renders down to a component. getMockRef would then be called with `getMockRef(InputInstance, 'text)' and would be called once per ref in the tree. I'm not sure if this makes sense, just figured I'll throw the idea out there.

I think that's what we're working on here, if I'm following. getMockRef is called when getPublicInstance is supposed to be called (which is what's passed to the ref function or attached as the ref for legacy refs). We'll pass in the current element to getMockRef so users can decide what kind of mock instance to return. Does that sound in line with what you're thinking?

Member

aweary commented Sep 6, 2016

I was wondering if we could have a top level getMockRef per test renderer instance that would get arguments based on the ref, like the current element and the name of the expected ref? For example it could be called with the instance of an component that renders down to a component. getMockRef would then be called with `getMockRef(InputInstance, 'text)' and would be called once per ref in the tree. I'm not sure if this makes sense, just figured I'll throw the idea out there.

I think that's what we're working on here, if I'm following. getMockRef is called when getPublicInstance is supposed to be called (which is what's passed to the ref function or attached as the ref for legacy refs). We'll pass in the current element to getMockRef so users can decide what kind of mock instance to return. Does that sound in line with what you're thinking?

@cpojer

This comment has been minimized.

Show comment
Hide comment
@cpojer

cpojer Sep 6, 2016

Contributor

Yeah exactly! But how can I decide which mock ref to use if there are two different refs used in the same component?

Contributor

cpojer commented Sep 6, 2016

Yeah exactly! But how can I decide which mock ref to use if there are two different refs used in the same component?

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 6, 2016

Member

Passing in the current element should let you return different mocks for different element types. Is there a use case for returning different mocks for the same element type (e.g., two refs on two divs)? If not then the current element should be sufficient

Member

aweary commented Sep 6, 2016

Passing in the current element should let you return different mocks for different element types. Is there a use case for returning different mocks for the same element type (e.g., two refs on two divs)? If not then the current element should be sufficient

@cpojer

This comment has been minimized.

Show comment
Hide comment
@cpojer

cpojer Sep 6, 2016

Contributor

Oh I see, this is an element, not a component.

Contributor

cpojer commented Sep 6, 2016

Oh I see, this is an element, not a component.

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 6, 2016

Member

Even if it was a component, the 'type' attribute should tell you which component it is 👍

Member

aweary commented Sep 6, 2016

Even if it was a component, the 'type' attribute should tell you which component it is 👍

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 8, 2016

Member

So, looking into attaching the mockConfig to the transaction: the one issue I'm running into is that accessing the transaction in ReactRef.attachRefs isn't as straightforward as I'd hoped.

ReactRef.attachRefs is called indirectly in ReactReconciler in a helper function, attachRefs.

function attachRefs() {
  ReactRef.attachRefs(this, this._currentElement);
}

That function is enqueued in the reactMountReady callback, which is a pooled instance of CallbackQueue.

transaction.getReactMountReady().enqueue(attachRefs, internalInstance);

The issue is that CallbackQueue only lets you provide the callback and the context for the callback; there's no way to pass any additional arguments to the enqueued callback. We could just tack on the mockConfig to internalInstance but then we'd need to add _mockConfig as a property on all instance types beforehand since at this point the instance disallows extensions, and I don't think ReactReconciler should be concerned with a transaction-specific config field.

Another option is to allow an argument (or multiple?) to be provided in CallbackQueue

  enqueue: function(callback, context, arg) {
    this._callbacks = this._callbacks || [];
    this._contexts = this._contexts || [];
    this._args = this._args || [];
    this._callbacks.push(callback);
    this._contexts.push(context);
    this._args.push(arg);
  },

and then just pass in the argument when when we call call on the queued callback.

      for (var i = 0; i < callbacks.length; i++) {
        callbacks[i].call(contexts[i], args[i]);
      }

cc @spicyj @gaearon what do you guys think?

Member

aweary commented Sep 8, 2016

So, looking into attaching the mockConfig to the transaction: the one issue I'm running into is that accessing the transaction in ReactRef.attachRefs isn't as straightforward as I'd hoped.

ReactRef.attachRefs is called indirectly in ReactReconciler in a helper function, attachRefs.

function attachRefs() {
  ReactRef.attachRefs(this, this._currentElement);
}

That function is enqueued in the reactMountReady callback, which is a pooled instance of CallbackQueue.

transaction.getReactMountReady().enqueue(attachRefs, internalInstance);

The issue is that CallbackQueue only lets you provide the callback and the context for the callback; there's no way to pass any additional arguments to the enqueued callback. We could just tack on the mockConfig to internalInstance but then we'd need to add _mockConfig as a property on all instance types beforehand since at this point the instance disallows extensions, and I don't think ReactReconciler should be concerned with a transaction-specific config field.

Another option is to allow an argument (or multiple?) to be provided in CallbackQueue

  enqueue: function(callback, context, arg) {
    this._callbacks = this._callbacks || [];
    this._contexts = this._contexts || [];
    this._args = this._args || [];
    this._callbacks.push(callback);
    this._contexts.push(context);
    this._args.push(arg);
  },

and then just pass in the argument when when we call call on the queued callback.

      for (var i = 0; i < callbacks.length; i++) {
        callbacks[i].call(contexts[i], args[i]);
      }

cc @spicyj @gaearon what do you guys think?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 12, 2016

Member

I didn’t want to say Node because we also use test renderer on native, and presumably at some point we’ll make native host instances have string element type? Then this could be used for more than DOM nodes. I don’t really know enough about RN to say.

createRefMock() sounds good to me. @aweary let’s change to that and then get it in?

Member

gaearon commented Sep 12, 2016

I didn’t want to say Node because we also use test renderer on native, and presumably at some point we’ll make native host instances have string element type? Then this could be used for more than DOM nodes. I don’t really know enough about RN to say.

createRefMock() sounds good to me. @aweary let’s change to that and then get it in?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Sep 12, 2016

Member

I was thinking of those as nodes too. :)

Member

sophiebits commented Sep 12, 2016

I was thinking of those as nodes too. :)

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 12, 2016

Member

Hmm, then I like createNodeMock the most.
Ref is more jargon-y and can be confused with composite refs.

Member

gaearon commented Sep 12, 2016

Hmm, then I like createNodeMock the most.
Ref is more jargon-y and can be confused with composite refs.

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 12, 2016

Member

@gaearon @spicyj updated it to createNodeMock 👍

Member

aweary commented Sep 12, 2016

@gaearon @spicyj updated it to createNodeMock 👍

@ghost ghost added the CLA Signed label Sep 12, 2016

@gaearon gaearon changed the title from Implement getMockRef for ReactTestRenderer to Implement createNode for ReactTestRenderer Sep 13, 2016

@gaearon gaearon changed the title from Implement createNode for ReactTestRenderer to Implement createNodeMock for ReactTestRenderer Sep 13, 2016

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 13, 2016

Member

Great job.
Leaving the honor of pressing "Squash and Merge" to you 😄

Member

gaearon commented Sep 13, 2016

Great job.
Leaving the honor of pressing "Squash and Merge" to you 😄

@ghost ghost added the CLA Signed label Sep 13, 2016

@gaearon gaearon assigned aweary and unassigned gaearon Sep 13, 2016

@ghost ghost added the CLA Signed label Sep 13, 2016

@aweary aweary merged commit f3569a2 into facebook:master Sep 13, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 87.318%
Details

@zpao zpao modified the milestones: 15-next, 15.4.0 Oct 4, 2016

zpao added a commit that referenced this pull request Oct 4, 2016

Implement createNodeMock for ReactTestRenderer (#7649)
* Implement optional mockConfig and getMockRef

* default mockConfig, walk render tree

* Pass mockConfig to transaction

* Attach mockConfig to transaction

* type mockConfig in ReactRef

* Expect object in native component ref test

* Fix argument name for attachRefs

* Add mockConfig support to legacy refs

* Pass transaction to getPublicInstance

* Implement getMockConfig on ReactTestReconcileTransaction

* Merge defaultMockConfig and mockConfig options

* Rename mockConfig to testOptions

* Break getPublicInstnce into three lines

* createMockRef -> createNodeMock

(cherry picked from commit f3569a2)

@roonyh roonyh referenced this pull request Oct 10, 2016

Closed

How do I use jest.mock ? #38

@aweary aweary referenced this pull request Feb 5, 2017

Merged

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

4 of 8 tasks complete

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v16 #29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment