Use only public api for ReactDOMEventListener-test.js #11327

enapupe · 2017-10-22T20:51:45Z

Related to #11299 – Express more tests via public API
Hey! After a couple hours fighting with this one I think I got the idea..
Could you guys check if I'm in the right direction?

BTW, I lost quite some time until I realized this line: https://github.com/facebook/react/blob/master/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js#L32 was messing up with ReactTestUtils.SimulateNative... Funny, that's probably the reason why we are using just the public api 😅

enapupe · 2017-10-22T22:44:05Z

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

+      document.createElement('div'),
+    );
+
+    ReactDOM.findDOMNode(component).appendChild(otherNode);


I'm not sure about this first test, it seems to have lost its initial purpose.

sebmarkbage · 2017-10-22T23:00:24Z

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

+    );
+
+    ReactDOM.findDOMNode(component).appendChild(otherNode);
+    ReactTestUtils.SimulateNative.mouseOver(otherNode);


For these purposes, I'd like to treat SimulateNative (and all the ReactTestUtils really as an internal API. The reason for that is that they also reach into internals indirectly. They actually bypass some of the implementation details that would make this example not work with normal React.

In normal React this wouldn't work because the container div is not in the document and therefore mouse moves would never be dispatched on top of it. React can't pick it up because it uses event listeners on the document to do it.

Instead, we should insert the container div into the document, dispatch a native event and then remove the div from the document. Like this:
https://github.com/facebook/react/blob/master/packages/react-dom/src/__tests__/ReactDOMInput-test.js#L55-L59

Hey! I did the requested changes, could you take a look at it?
I'm still not sure if the first test is on spot because before it tested two unrelated components and now they have a parent-child relationship.

Also while we are at this file, I'm not sure how to rewrite should not get confused by disappearing elements.. I think I understand the thinking behind it but I'm not sure if it's possible to accomplish the same result without mocking handleTopLevel. I'd have to unmount a component in the middle of the event chain, which seems impossible.

Thanks!

gaearon · 2017-10-26T19:28:55Z

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

+
+    var container = document.createElement('div');
+    var stub = ReactDOM.render(<div onMouseOver={mock} />, container);
+    var node = ReactDOM.findDOMNode(stub);


There's no "stubs" anymore, you can remove these findDOMNode() calls. Return value of ReactDOM.render(<div />) is the node itself.

gaearon · 2017-10-26T19:29:38Z

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

+    var container = document.createElement('div');
+    var stub = ReactDOM.render(<div onMouseOver={mock} />, container);
+    var node = ReactDOM.findDOMNode(stub);
+    var otherComponent = document.createElement('h1');


Let's not call nodes "components". I understand this naming already existed but let's keep it clear in new code. We call DOM nodes "nodes" in newer code (or "containers" when we render into them). So otherNode was a good naming.

gaearon · 2017-10-26T19:33:15Z

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

+    var stub = ReactDOM.render(<div onMouseOver={mock} />, container);
+    var node = ReactDOM.findDOMNode(stub);
+    var otherComponent = document.createElement('h1');
+    node.appendChild(otherComponent);


Why is it appended as a child to node? I don't see it in the original test. My impression (based on the test name) is that the test specifically wants to test it still works if the otherComponent is outside the React tree. Whereas this is putting it inside.

gaearon · 2017-10-26T19:54:07Z

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

+    otherComponent.dispatchEvent(nativeEvent);
+
+    expect(mock).toBeCalled();
+    document.body.removeChild(container);


I think the right way to write this test would be something like:

var mock = jest.fn(); var container = document.createElement('div'); var node = ReactDOM.render(<div onMouseEnter={mock} />, container); var otherNode = document.createElement('h1'); document.body.appendChild(container); document.body.appendChild(otherNode); var nativeEvent = document.createEvent('MouseEvent'); nativeEvent.initMouseEvent('mouseout', true, true, window, 0, 0, 0, 0, 0, false, false, false, false, 0, node); otherNode.dispatchEvent(nativeEvent); expect(mock).toBeCalled();

This way we verify that if an event originates outside of React tree (on otherNode) it still fires an event inside a React tree.

I had to use a different event but that doesn't matter for this test.

gaearon · 2017-10-26T19:56:53Z

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

-      });
+      var nativeEvent = document.createEvent('Event');
+      nativeEvent.initEvent('mouseout', true, true);
+      ReactDOM.findDOMNode(childControl).dispatchEvent(nativeEvent);


No findDOMNode please.

gaearon · 2017-10-26T19:57:04Z

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

      var parentContainer = document.createElement('div');
-      var parentControl = <div>Parent</div>;
+      var parentControl = <div onMouseOut={onMouseOut}>div</div>;
      childControl = ReactDOM.render(childControl, childContainer);
      parentControl = ReactDOM.render(parentControl, parentContainer);
      ReactDOM.findDOMNode(parentControl).appendChild(childContainer);


Here, too, we can remove all findDOMNodes.

gaearon · 2017-10-26T19:58:27Z

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

@@ -90,22 +95,25 @@ describe('ReactDOMEventListener', () => {
      ReactDOM.findDOMNode(parentControl).appendChild(childContainer);


Same re: findDOMNode.

gaearon · 2017-10-26T19:58:47Z

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

        return <div><div id="outer">{inner}</div></div>;
      }
    }

    var instance = ReactTestUtils.renderIntoDocument(<Wrapper />);
+    ReactTestUtils.SimulateNative.mouseOut(instance.getInner());


Let's rewrite this one not to use SimulateNative either. Can we?

enapupe · 2017-10-26T22:10:17Z

Thanks for the feedback!
The changes were addressed but there are still 2 tests left to go. I'm not sure how to proceed here because both tests change handleTopLevel behaviour.
One seems to test if ReactDOMEventListener can handle an element being unmounted in the middle of an event chain.
The other I'm not even sure if I understand, it mentions two different roots enqueuing events, but the current test only dispatches one event so I'm not really sure what's that about. THanks!

gaearon · 2017-10-26T22:29:56Z

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

+    document.body.appendChild(container);
+    document.body.appendChild(otherNode);
+
+    var nativeEvent = document.createEvent('MouseEvent');


I saw another PR using new MouseEvent(type, args), could you check if that one's going to be shorter?

Yeah it seems

new MouseEvent('mouseout', { bubbles: true, cancelable: true, relatedTarget: node, })

works just fine.

gaearon · 2017-10-27T00:03:02Z

One seems to test if ReactDOMEventListener can handle an element being unmounted in the middle of an event chain.

I think it just wants to test that if you unmountComponentAtNode() during a React event (e.g. in child component's handler), the parent still receives the event too. Does that seem to make sense? Can you try to write a test for it? It might also help to view the blame for these lines, find the code that “fixed” this case (when this code was introduced), and then “undo” the fix to see if your test catches the regression. This would tell you if you’re testing for the same thing it was originally written for or not.

The other I'm not even sure if I understand, it mentions two different roots enqueuing events, but the current test only dispatches one event so I'm not really sure what's that about. THanks!

React batches updates occurring inside the same browser event. Means it applies all changes when it exists the top-level browser event handler (instead of when it exits component handler). This test verifies this behavior is true even if two components live in separate React trees (but in the same document).

In other words it wants to verify that if you do

var mock = jest.fn();

and then, inside child (in a separate root) event handler:

ReactDOM.render(<div>1</div>, childContainer);
mock(childNode.textContent);

and inside parent (in a separate root) event handler:

ReactDOM.render(<div>2</div>, childContainer);
mock(childNode.textContent);

Both calls to mock should've been called with 'Child' because both updates are batched together and neither one is applied yet inside React event handlers. You should, however, check that when after dispatching is done, childNode.textContent is now set to '2'. (Now that updates are flushed.)

gaearon · 2017-10-31T14:19:40Z

Looks like a test is still failing. Would you like to continue working on this?

enapupe · 2017-10-31T14:24:53Z

Yeah there's just one missing.
I'll try to do it until tomorrow, otherwise I'll be traveling for a few days and will be back only after Nov 5th, feel free to take over if you must.

gaearon · 2017-10-31T14:35:26Z

Thanks, sounds good!

enapupe · 2017-11-01T16:04:32Z

So, I think I was able to reproduce the same test case but I couldn't find the code that introduced that fix you mentioned, so confirmation is still necessary. The blame for that lines seem to be sort of detached from the git tree..

There was no need to extract React elements into separate variables.

I could not get it to fail on master so it was probably testing something specific to Stack implementation details. It was also already broken because it didn't look at the right argument and never actually called `unmountComponentAtNode`. Instead I replaced it with original repro case from #1105 which is when it was introduced.

gaearon · 2017-11-03T20:30:23Z

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

-          <div>{topLevelTarget === childNode ? '1' : '2'}</div>,
-          childContainer,
-        );
-        // Since we're batching, neither update should yet have gone through.


Comments like this are valuable because they're not obvious.

addressed

gaearon · 2017-11-03T20:43:54Z

This looks great! Thanks. I made a few small tweaks.

* Use only public api for ReactDOMEventListener-test.js * Use less confusing naming There was no need to extract React elements into separate variables. * Replace the "disappearance" test I could not get it to fail on master so it was probably testing something specific to Stack implementation details. It was also already broken because it didn't look at the right argument and never actually called `unmountComponentAtNode`. Instead I replaced it with original repro case from facebook#1105 which is when it was introduced. * Tweak naming and add comments * Missed this one

facebook-github-bot added the CLA Signed label Oct 22, 2017

enapupe commented Oct 22, 2017

View reviewed changes

sebmarkbage previously requested changes Oct 22, 2017

View reviewed changes

gaearon requested changes Oct 26, 2017

View reviewed changes

gaearon mentioned this pull request Oct 26, 2017

Express more tests via public API #11299

Closed

26 tasks

gaearon reviewed Oct 26, 2017

View reviewed changes

Use only public api for ReactDOMEventListener-test.js

9ad2c00

gaearon added 3 commits November 3, 2017 19:22

Merge branch 'master' into reactdom-event-listener

9060cca

Use less confusing naming

c84207b

There was no need to extract React elements into separate variables.

gaearon reviewed Nov 3, 2017

View reviewed changes

Tweak naming and add comments

d188ba0

gaearon changed the title ~~(WIP) Use only public api for ReactDOMEventListener-test.js~~ Use only public api for ReactDOMEventListener-test.js Nov 3, 2017

Missed this one

e11af09

gaearon approved these changes Nov 3, 2017

View reviewed changes

gaearon merged commit bfa2690 into facebook:master Nov 3, 2017

gaearon mentioned this pull request Dec 22, 2017

Reduce the handleTopLevel() event code indirection #11915

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use only public api for ReactDOMEventListener-test.js #11327

Use only public api for ReactDOMEventListener-test.js #11327

enapupe commented Oct 22, 2017 •

edited

Loading

enapupe Oct 22, 2017 •

edited

Loading

sebmarkbage Oct 22, 2017

enapupe Oct 23, 2017 •

edited

Loading

gaearon Oct 26, 2017

gaearon Oct 26, 2017

gaearon Oct 26, 2017

gaearon Oct 26, 2017

gaearon Oct 26, 2017

gaearon Oct 26, 2017

gaearon Oct 26, 2017

gaearon Oct 26, 2017

enapupe commented Oct 26, 2017

gaearon Oct 26, 2017

enapupe Oct 26, 2017 •

edited

Loading

gaearon commented Oct 27, 2017 •

edited

Loading

gaearon commented Oct 31, 2017

enapupe commented Oct 31, 2017 •

edited

Loading

gaearon commented Oct 31, 2017

enapupe commented Nov 1, 2017

gaearon Nov 3, 2017

gaearon commented Nov 3, 2017

		@@ -90,22 +95,25 @@ describe('ReactDOMEventListener', () => {
		ReactDOM.findDOMNode(parentControl).appendChild(childContainer);

Use only public api for ReactDOMEventListener-test.js #11327

Use only public api for ReactDOMEventListener-test.js #11327

Conversation

enapupe commented Oct 22, 2017 • edited Loading

enapupe Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enapupe Oct 23, 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

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

Choose a reason for hiding this comment

enapupe commented Oct 26, 2017

Choose a reason for hiding this comment

enapupe Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

gaearon commented Oct 27, 2017 • edited Loading

gaearon commented Oct 31, 2017

enapupe commented Oct 31, 2017 • edited Loading

gaearon commented Oct 31, 2017

enapupe commented Nov 1, 2017

Choose a reason for hiding this comment

gaearon commented Nov 3, 2017

enapupe commented Oct 22, 2017 •

edited

Loading

enapupe Oct 22, 2017 •

edited

Loading

enapupe Oct 23, 2017 •

edited

Loading

enapupe Oct 26, 2017 •

edited

Loading

gaearon commented Oct 27, 2017 •

edited

Loading

enapupe commented Oct 31, 2017 •

edited

Loading