-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
spy on wrapper.instance() method not called when using shallow() #944
Comments
If you use class properties instead of proper manual this-binding, you're just stuck. |
Thanks for the reply 😄 Is there a reason why class properties work with |
I'm not actually sure why it works with |
Okay cool thanks, that does make a lot of sense 👍 Out of curiosity, I took a peek at the source code and below is what I found. The const spy = jest.spyOn(wrapper.instance(), 'handleButtonClick'); // replace function via reference
wrapper.update(); // forceUpdate()
wrapper.find('button').simulate('click'); // actually calls the spy function The I thought if you then call Possible bug? Documentation? |
Would you like a PR adding these examples to the docs? |
Yes, sorry, that sounds great. |
I've been trying to follow the follow the contributing guide and I am unable to build the docs. Must have spent a couple of hours on this, but did the following:
It failed on I then installed node@6 and it got a little further but still failed with Does anyone have any tips, can anyone else get the docs to build? If so how? |
I'm having a similar issue - still trying to figure out a solution given what you've described @samit4me OTOH, I've had a bunch of experience working w/GitBook, so maybe I can help debug your install issue? Based on the issue you linked, I assume you're on a Mac. What OS version? And details of your node setup? |
@jebeck Any help with GitBook would be awesome 👍. I've tried on macOS 10.12.5 and Win 10 with node versions 6, 7 and 8. Out of curiosity, are you able to get the docs to build based on the instructions in the contributing guide? |
The docs only build for me in node 6; they fail in node 7 and 8. @jebeck any help you could provide on that, too, would be great :-) |
What OS are you running @ljharb ? |
Sorry I've taken so long to look into this. Unfortunately, I'm having the same problems. I just tried a version bump of GitBook up to 3.x (because I'm using that in other projects with newer node versions with zero problems), but that resulted in config errors. I'll open a PR with the config changes, and you all can try it out and see if that's the direction you want to go in. It doesn't work quite yet, looks like there will have to be an awful lot of link rewriting from docs/etc/file.md to just etc/file.md because it's necessary to specify CC @samit4me |
I just uninstalled all versions of node, npm, yarn etc and installed nvm and then tried on macOS Sierra 10.12.5 with node 6.11.1 and npm 3.10.10. After following the guide it failed on I then bumped the version of GitBook to 3.x as per @jebeck PR and it built correctly with the same versions listed above. I don't have any experience with GitBook but it sounds like a good idea to upgrade. Will put this on hold until these GitBook issues are resolved. |
@samit4me in your examples if you replace see: #622
I've been following your advice on constructor binding and it's performance issues but it seems to conflict with what they're saying here facebook/react#9851 (comment) tldr; performance wise, doing a constructor bind is identical to having a class property, with the added benefit of also being on the prototype (and the "performance" hit that comes with it) |
That comment is incorrect about them being essentially the same. In practice, the performance might be the same with React, but that doesn't mean they're equivalent choices. |
any updates on this? still the case on latest enzyme and jest |
@bochen2014 no update on the difference here between mount and shallow, no |
with latest // shallow case
it.only('spyOn', () => {
const wrapper = shallow(<ToggleCheckbox />);
const spy = jest.spyOn(wrapper.instance(), 'onChange');
wrapper.instance().forceUpdate();
const myBtn = wrapper.find('MyButton')
myBtn.simulate('click');
// fail; console.log shows that onChange method is called but assert still fails. why??
expect(spy).toBeCalled();
})
// mount case
it.only('spyOn', () => {
const wrapper = mount(<ToggleCheckbox />);
const spy = jest.spyOn(wrapper.instance(), 'onChange');
wrapper.update();
const myBtn = wrapper.find('MyButton')
myBtn.simulate('click');
// fail; onChange method is NOT called and assertion failed
expect(spy).toBeCalled();
}) (I can confirm that using traditional prototype functions and explicit binding works well with enzyme ) |
@bochen2014 right; it's simply impossible to spy on the function successfully when it's a class property, because the reference is captured before you spy on it. Don't put functions on class properties, full stop. |
thanks for letting me know ;-) I can understand that if you do wrapper.instance().onChange = jest.fn() aren't we replacing the instance method? I think @samit4me has done some insight research and find it more like a defect. have you got a chance to look at it? |
@bochen2014 yes you are; but you're replacing it after a permanent reference to the original is held in the rendered elements. It's very common, but it's still not a bug - this is how function-valued class properties in react components are supposed to behave, and the only solution is to not use them. |
that makes sense to me. |
@ljharb @samit4me @jebeck @bochen2014
My App render() looks like this (I am using ArrowKeysReact for acessing keyboard arrows and I pass it to arrowDown function):
And other tests (rendering ones) are working (if you wonder):
Please for help, I decided to start TDD and now I am stopped from prodcutive work from a few days... |
After many experiments, I finally can spy functions that I pass as a props:
But when it is typical component method that maybe influe to component state and not uses props I still can't pass it by doing this:
How can I solve that problem? Pleaase for help and greetings. |
@Artimal the only solution to that problem is to NEVER use arrow functions in class fields, to make your |
@ljharb Thank you for your response.
|
OK, clearly my guesses about your |
An alternate approach that also works: Instead of assigning your function directly as the event handler, call it within an anonymous function. The anonymous function will dereference Let's assume we have an arbitrary
...this will pass:
...and this will fail:
The tradeoff here is that we're using an arrow function as our handler- it'll be recreated with each render, which is a very minor penalty but still one to consider. |
@hmorgancode this has huge performance implications, especially when passed into a PureComponent. The best practice remains, use an instance method that's constructor-bound. |
@ljharb oh, woah, I totally blanked on the function equality aspect of this. My bad! To clarify, you're referring mainly to the fact that each time an arrow function is 'recreated' in render, it's:
|
That's correct. You can avoid that by using either a constructor-bound instance method, or an arrow function in a class property. However, an arrow function in a class property has the downside of being less optimizeable and less testable. |
@ljharb why would arrow functions on a class be less optimizable? Would it be bc the arrow function on the class would not have a reference to the name of the function and would keep re creating new functions? Though, when using class bound functions it will be able to reference the function name, thus not recreating new functions? Thanks |
@iwllyu damn hell thank you Your advice with |
@sstern6 as we've discussed separately, the "meat" of the function would live on the prototype, which would get easily optimized - and then the only piece that repeats N times is the trivial bind-proxy. |
This seems answered; the solution remains "never use arrow functions in class properties". |
@iwllyu Thank you. |
Hi @ljharb , I was wondering if you could please expand on your previous comment:
I am not sure if I completely understood it. I understood that class properties are defined on the component instance rather than its prototype. However I don't quite get why it could not be overridden by a mock if we do
Could you please point me in the right direction? Thanks a lot for your help! |
Because the unspied version will have already been passed as a prop into the render tree. The spy afterwards has no effect. In other words, never put an arrow function in any class field ever - instead, make a normal instance method, and do |
Thanks a lot for the explanation @ljharb ! I am assuming the reason that this only happens to arrow function class fields but not to to normal class field is due to the following reason: Each time you pass the reference to the arrow function class field into the render tree , a new arrow function instance is created for the render tree to use (which has no reference to the instance method). When we spy on the instance method, we are only spying the original version of the method, not the one that has been passed in. So basically the two copies don't point to the same reference as normal functions would. On the other hand, using a normal class method (or binding it in the constructor), we are not creating a new instance of the method, but rather passing in the reference. Therefore by spying on the instance method, we are also changing what has been passed into the render tree as they are connected by reference. Am I correct? |
@alanyinjs it would happen on normal class fields too if you assign them to any non-primitive, but people don't tend to try to replace non-functions with spies. Your analysis seems correct (noting that you'll need to spy on the instance method before any instance is created). |
@ljharb Yeah you're right... Just one more question: I still haven't got why spying I get why However what I don't get is why using normal function and binding would not fail but arrow function class fields would. As binding in the constructor Thanks for answering my questions. |
The only way to make it work is to spy on the method before the render method is called. When rendering a component with enzyme, the render method is called immediately - so by the time |
Simulating instance test been done with https://github.com/airbnb/enzyme/issues/944\#issuecomment-362103255.
Here is a simple example, i hope it helps. class App extends React.Component {
componentDidMount(){
this.handleSomething();
}
handleSomething = () => {
//
}
...
} const wrapper = shallow(<Component />, { disableLifecycleMethods: true })
const instance = wrapper.instance();
const handleSomethingSpy = jest.spyOn(instance, 'handleSomething')
instance.componentDidMount();
expect(handleSomethingSpy).toHaveBeenCalled() |
@enestufekci that should work in this case because |
Everything I'm reading here makes me think that my code here should NOT work, but it does. I wonder if there's some new kinda magic happening under the hood, because I recall having the problem that others have been having in the past, but not now.
|
My test : describe('Personal Profile', () => {
it('renders', () => {
const wrapper = mount(
<PersonalProfile store={store}/>
);
const spy = jest.spyOn(wrapper.instance(), 'handleChangetype')
wrapper.update();
wrapper.find(Typeahead).at(2).simulate('change');
console.log(wrapper.find(Typeahead).at(2).simulate('change').debug())
expect(spy).toHaveBeenCalled();
});
}); In my js file, I have not used arrow functions and bound the method in the constructor though I m having this error. can anybody help ? |
@bhargav11-crest you can't spy on the instance after making the wrapper - you need to spy on |
Testing a component method in isolation can be done via
wrapper.instance().method()
, see #208.Testing a user event actually calls that method can be achieved using
jest.spyOn()
and.simulate()
.To the best of my knowledge, you can spy on either the prototype or instance:
jest.spyOn(Component.prototype, 'method')
jest.spyOn(wrapper.instance(), 'method')
It seems that the prototype spy will work if you render with either
shallow()
ormount()
, but when using the instance,mount()
works andshallow()
does not.This would be fine, just use prototype right! Unfortunately, our team is lazy and don't enjoy binding every method to
this
in theconstructor()
, so we use class properties to avoid it. The downside of class properties is that they do not appear on the prototype, so we are forced to spy on the instance. This works perfectly if you render withmount()
but in unit tests, we always useshallow()
so we can test the "unit" in isolation.Can you see the dilemma? Should
shallow()
treat spies differently thanmount()
?To demonstrate the issue below is a component and the associated test:
If you want to try it out, take a look at this repo. Any help will be greatly appreciated.
The text was updated successfully, but these errors were encountered: