Skip to content
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

Bug: ReactTestUtil.Simulate toLowerString mutating event type. #7418

Closed
uxtx opened this issue Aug 3, 2016 · 3 comments
Closed

Bug: ReactTestUtil.Simulate toLowerString mutating event type. #7418

uxtx opened this issue Aug 3, 2016 · 3 comments

Comments

@uxtx
Copy link

uxtx commented Aug 3, 2016

Current behaviour

ReactTestUtil is returning nodes with event type cast to lower case strings. This is causing test failures in several external repos (react-select being a public-facing example). As far as i can tell, the issue appears to be that this type mis-match is causing DOM nodes to not to return the same node that was initially called.

Expected behaviour

ReactTestUtil should return the proper event type, cast to string.

Steps to verify

  • Clone https://github.com/JedWatson/react-select, npm install && npm test verify tests fail and react and react-addon-test-util are using v15.3.0.
  • Replace line 429 node_modules/react/lib/ReactTestUtils.js with fakeNativeEvent.type = eventType.toString();
  • run npm test and verify tests pass.

Tested with

React 15.3.0 / Chrome 51 / OS X

@zpao
Copy link
Member

zpao commented Aug 3, 2016

Can you point at a specific failing test? "keyDown" isn't what the browser event type is and that's what we're creating at that place in the code.

@uxtx
Copy link
Author

uxtx commented Aug 3, 2016

@zpao sure thing - a specific failing test can be found here where the node queried for is returning empty https://github.com/JedWatson/react-select/blob/master/test/Select-test.js#L230

@zpao
Copy link
Member

zpao commented Aug 3, 2016

Alright, tracked this down. This is a problem with react-select and the way it's testing, not with React. It was purely because we were not setting event.type before that your tests were passing at all. As our testing utils improve (as with #6154), it is exposing that the tests were not written correctly before.

Let's start here: https://github.com/JedWatson/react-select/blob/e19bce383a8fd1694278de47b6d00a608ea99f2d/src/Select.js#L268. That code is checking the event type and then further checking the button that was pressed. There's nothing wrong with that. Previously we didn't set the type so the button condition wasn't checked at all. Now we set the type and event.button is undefined, so you return early.

Now to the first test failure: https://github.com/JedWatson/react-select/blob/e19bce383a8fd1694278de47b6d00a608ea99f2d/test/Select-test.js#L228. That is simulating a mousedown event, however it's not specifying which button is being pressed, resulting in event.button === undefined, leading to the condition above. There are a ton of these - you'll probably see a pattern related to the failing tests. There may be some other cases which are slightly different but should be fixed as well.

The code we have in React now is more correct than it used to be and so the recommendation is going to be that you fix the tests in react-select. Feel free to point them here if you need something to back up your claim (there or in any other projects suffering from this).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants