-
-
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
Testing Hooks with mount: onChange not called #1996
Comments
I don't recommend using All you need to test is that a) the |
Yes, I know you don't. But that is not my point of view on the matter. I think Kent C. Dodds explained part of my opinion on the matter. However simulate is still part of the api of enzyme. Reporting an unexpected behavior when using it is still making sense, does it? |
His opinion on testing (which i highly disagree with) is that you should actually simulate things - but that’s not what enzyme’s simulate does, so it’s irrelevant here. The entire simulate api is unexpected behavior, and i intend to remove it completely as part of a future v4. enzyme doesn’t support hooks, and simulate is nothing but sugar over invoking a prop with a fake event object - iow, if you use hooks whatsoever (a currently unreleased feature, but either way, an unsupported one) you should expect enzyme not to work reliably. It’s best to wait to use React features in components until you’re able to unit test them - ie, until enzyme supports them. |
@ljharb Would you like help with removing it? 😃 |
@k3ithl1m thanks, but i'd prefer not to have a major version right now - when that time rolls around, it'll be pretty straightforward to remove it. A PR to update the docs for it, however, would always be appreciated. |
Sure thing. Ill get to it as soon as I can. Just to make sure I'm going towards the right direction. https://airbnb.io/enzyme/docs/api/ReactWrapper/simulate.html This is the page that I need to update right? Should we mention in the docs that it shouldnt be used due to unexpected behaviour? or should we just remove it entirely, and anywhere that has simulation, we remove it. |
That one, and also the ShallowWrapper one. We definitely shouldn't remove the docs for it, because the method still exists, but we should indicate that they're deprecated, and why. |
Well, React 16.8 is out today. I let you decide whether you want to close this issue as I have nothing more to say. |
Indeed it is - so as of today, this is a valid issue :-) we don’t support hooks explicitly, but if you update all of your React package versions, it’s likely most things will just work - can you confirm you still have this issue? (for example, the shallow renderer and test utils in all of the alphas lacked features that the actual 16.8 has) |
I can confirm I still have this issue. |
Enzyme probably should either reexport or automatically use ReactTestRenderer.act but otherwise things should just work. |
Have same issue - re-writing a component with hooks and |
@ljharb how may I help |
@Jessidhia I don't think it should re-export it, but perhaps it should automatically use @k3ithl1m a PR that includes lots of test cases would be great. |
@echoes221 fwiw, there's nothing clean about "simulate", that's testing react and the browser - explicitly invoke a prop function if that's what you want to test. |
@ljharb OK thanks. I think partly I'm seeing some weirdness with the Fabric component library on the top of it when switching to hooked methods over lifecycle - calling the props directly asserts what I need (It also needs to be wrapped in Thanks for the info. Moving forwards into v4, how would you envision testing this going forwards? |
I'm not sure yet; hooks have only been out for a day and a half. It's likely enzyme will call |
@ljharb @ekaradon @chenesan https://codesandbox.io/s/132rz9vq03 added codesandbox where issue is reprodicible even with explicit |
@pgangwani that doesn't give me a super helpful error; adding test cases to #2041 or #2029 would be preferred. |
Sure, Will add in my next set of test cases. Now focussing on other hooks method. |
Just adding in my two cents - it looks like this issue is present with other events such as Reproduction example
|
(This should be re-evaluated after the next release of enzyme comes out; it may be fixed by then) |
v3.10.0 has now been released. Closing; happy to reopen if it's still an issue. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@ekaradon wrong, you don't have to listen to what Dodds preaches. it's ok to test at a lower level regardless of him telling you not to. You can make the public API either through your browser OR through a set of public functions YOU want to test and if you test it in a smart way who cares if you go through an fing button or not. So what if you don't test button wiring. |
Current behavior
When simulating some changes on input with components using hooks, depending of the component, it looks like the
onChange
is not called.Expected behavior
Whatever the component, when simulating changes on input, the
onChange
method must be called.Your environment
API
mount
Version
Adapter
enzyme-adapter-react-16
The single fact to wrap the input with a
div
is hurting the test. Really Weird. Is there anyone having an idea about this?The text was updated successfully, but these errors were encountered: