Skip to content

Commit

Permalink
Use native event dispatching instead of Simulate or SimulateNative
Browse files Browse the repository at this point in the history 
In facebook#12629 @gaearon suggested that it would be better to drop usage of
`ReactTestUtils.Simulate` and `ReactTestUtils.SimulateNative`. In this
PR I’m attempting at removing it from a lot of places with only a few
leftovers.

Those leftovers can be categorized into three groups:

1. Anything that tests that `SimulateNative` throws. This is a property
   that native event dispatching doesn’t have so I can’t convert that
   easily. Affected test suites: `EventPluginHub-test`,
   `ReactBrowserEventEmitter-test`.
2. Anything that tests `ReactTestUtils` directly. Affected test suites:
   `ReactBrowserEventEmitter-test` (this file has one test that reads
    "should have mouse enter simulated by test utils"),
    `ReactTestUtils-test`.
3. Anything that dispatches a `change` event. The reason here goes a bit
   deeper and is rooted in the way we shim onChange. Usually when using
   native event dispatching, you would set the node’s `.value` and then
   dispatch the event. However inside [`inputValueTracking.js`][] we
   install a setter on the node’s `.value` that will ignore the next
   `change` event (I found [this][near-perfect-oninput-shim] article
   from Sophie that explains that this is to avoid onChange when
   updating the value via JavaScript).

All remaining usages of `Simulate` or `SimulateNative` can be avoided
by mounting the containers inside the `document` and dispatching native
events.

Here some remarks:

1. I’m using `Element#click()` instead of `dispatchEvent`. In the jsdom
   changelog I read that `click()` now properly sets the correct values
   (you can also verify it does the same thing by looking at the
   [source][jsdom-source]).
2. I had to update jsdom in order to get `TouchEvent` constructors
   working (and while doing so also updated jest). There was one
   unexpected surprise: `ReactScheduler-test` was relying on not having
   `window.performance` available. I’ve recreated the previous
   environment by deleting this property from the global object.
3. I was a bit confused that `ReactTestUtils.renderIntoDocument()` does
   not render into the document 🤷‍

[`inputValueTracking.js`]: https://github.com/facebook/react/blob/392530104c00c25074ce38e1f7e1dd363018c7ce/packages/react-dom/src/client/inputValueTracking.js#L79
[near-perfect-oninput-shim]: https://sophiebits.com/2013/06/18/a-near-perfect-oninput-shim-for-ie-8-and-9.html
[jsdom-source]: https://github.com/jsdom/jsdom/blob/45b77f5d21cef74cad278d089937d8462c29acce/lib/jsdom/living/nodes/HTMLElement-impl.js#L43-L76
  • Loading branch information
@philipp-spiess
philipp-spiess committed Jun 12, 2018
1 parent 1594409 commit dfe5d83
Show file tree
Hide file tree
Showing 15 changed files with 560 additions and 404 deletions.
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
"gzip-js": "~0.3.2",
"gzip-size": "^3.0.0",
"jasmine-check": "^1.0.0-rc.0",
"jest": "^23.0.1",
"jest": "^23.1.0",
"jest-diff": "^23.0.1",
"merge-stream": "^1.0.0",
"minimatch": "^3.0.4",
Expand Down
36 changes: 22 additions & 14 deletions packages/react-dom/src/__tests__/ReactBrowserEventEmitter-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ let getListener;
let putListener;
let deleteAllListeners;

let container;

function registerSimpleTestHandler() {
putListener(CHILD, ON_CLICK_KEY, LISTENER);
const listener = getListener(CHILD, ON_CLICK_KEY);
Expand All @@ -63,7 +65,8 @@ describe('ReactBrowserEventEmitter', () => {
ReactBrowserEventEmitter = require('../events/ReactBrowserEventEmitter');
ReactTestUtils = require('react-dom/test-utils');

const container = document.createElement('div');
container = document.createElement('div');
document.body.appendChild(container);

let GRANDPARENT_PROPS = {};
let PARENT_PROPS = {};
Expand Down Expand Up @@ -129,6 +132,11 @@ describe('ReactBrowserEventEmitter', () => {
idCallOrder = [];
});

afterEach(() => {
document.body.removeChild(container);
container = null;
});

it('should store a listener correctly', () => {
registerSimpleTestHandler();
const listener = getListener(CHILD, ON_CLICK_KEY);
Expand All @@ -150,25 +158,25 @@ describe('ReactBrowserEventEmitter', () => {

it('should invoke a simple handler registered on a node', () => {
registerSimpleTestHandler();
ReactTestUtils.Simulate.click(CHILD);
CHILD.click();
expect(LISTENER).toHaveBeenCalledTimes(1);
});

it('should not invoke handlers if ReactBrowserEventEmitter is disabled', () => {
registerSimpleTestHandler();
ReactBrowserEventEmitter.setEnabled(false);
ReactTestUtils.SimulateNative.click(CHILD);
CHILD.click();
expect(LISTENER).toHaveBeenCalledTimes(0);
ReactBrowserEventEmitter.setEnabled(true);
ReactTestUtils.SimulateNative.click(CHILD);
CHILD.click();
expect(LISTENER).toHaveBeenCalledTimes(1);
});

it('should bubble simply', () => {
putListener(CHILD, ON_CLICK_KEY, recordID.bind(null, CHILD));
putListener(PARENT, ON_CLICK_KEY, recordID.bind(null, PARENT));
putListener(GRANDPARENT, ON_CLICK_KEY, recordID.bind(null, GRANDPARENT));
ReactTestUtils.Simulate.click(CHILD);
CHILD.click();
expect(idCallOrder.length).toBe(3);
expect(idCallOrder[0]).toBe(CHILD);
expect(idCallOrder[1]).toBe(PARENT);
Expand All @@ -179,7 +187,7 @@ describe('ReactBrowserEventEmitter', () => {
putListener(GRANDPARENT, ON_CLICK_KEY, recordID.bind(null, 'GRANDPARENT'));
putListener(PARENT, ON_CLICK_KEY, recordID.bind(null, 'PARENT'));
putListener(CHILD, ON_CLICK_KEY, recordID.bind(null, 'CHILD'));
ReactTestUtils.Simulate.click(CHILD);
CHILD.click();
expect(idCallOrder).toEqual(['CHILD', 'PARENT', 'GRANDPARENT']);

idCallOrder = [];
Expand All @@ -191,7 +199,7 @@ describe('ReactBrowserEventEmitter', () => {
recordID.bind(null, 'UPDATED_GRANDPARENT'),
);

ReactTestUtils.Simulate.click(CHILD);
CHILD.click();
expect(idCallOrder).toEqual(['CHILD', 'PARENT', 'UPDATED_GRANDPARENT']);
});

Expand Down Expand Up @@ -224,7 +232,7 @@ describe('ReactBrowserEventEmitter', () => {
recordID(GRANDPARENT);
expect(event.currentTarget).toBe(GRANDPARENT);
});
ReactTestUtils.Simulate.click(CHILD);
CHILD.click();
expect(idCallOrder.length).toBe(3);
expect(idCallOrder[0]).toBe(CHILD);
expect(idCallOrder[1]).toBe(PARENT);
Expand All @@ -239,7 +247,7 @@ describe('ReactBrowserEventEmitter', () => {
recordIDAndStopPropagation.bind(null, PARENT),
);
putListener(GRANDPARENT, ON_CLICK_KEY, recordID.bind(null, GRANDPARENT));
ReactTestUtils.Simulate.click(CHILD);
CHILD.click();
expect(idCallOrder.length).toBe(2);
expect(idCallOrder[0]).toBe(CHILD);
expect(idCallOrder[1]).toBe(PARENT);
Expand All @@ -254,7 +262,7 @@ describe('ReactBrowserEventEmitter', () => {
e.isPropagationStopped = () => true;
});
putListener(GRANDPARENT, ON_CLICK_KEY, recordID.bind(null, GRANDPARENT));
ReactTestUtils.Simulate.click(CHILD);
CHILD.click();
expect(idCallOrder.length).toBe(2);
expect(idCallOrder[0]).toBe(CHILD);
expect(idCallOrder[1]).toBe(PARENT);
Expand All @@ -268,7 +276,7 @@ describe('ReactBrowserEventEmitter', () => {
);
putListener(PARENT, ON_CLICK_KEY, recordID.bind(null, PARENT));
putListener(GRANDPARENT, ON_CLICK_KEY, recordID.bind(null, GRANDPARENT));
ReactTestUtils.Simulate.click(CHILD);
CHILD.click();
expect(idCallOrder.length).toBe(1);
expect(idCallOrder[0]).toBe(CHILD);
});
Expand All @@ -277,7 +285,7 @@ describe('ReactBrowserEventEmitter', () => {
putListener(CHILD, ON_CLICK_KEY, recordIDAndReturnFalse.bind(null, CHILD));
putListener(PARENT, ON_CLICK_KEY, recordID.bind(null, PARENT));
putListener(GRANDPARENT, ON_CLICK_KEY, recordID.bind(null, GRANDPARENT));
ReactTestUtils.Simulate.click(CHILD);
CHILD.click();
expect(idCallOrder.length).toBe(3);
expect(idCallOrder[0]).toBe(CHILD);
expect(idCallOrder[1]).toBe(PARENT);
Expand All @@ -300,7 +308,7 @@ describe('ReactBrowserEventEmitter', () => {
};
putListener(CHILD, ON_CLICK_KEY, handleChildClick);
putListener(PARENT, ON_CLICK_KEY, handleParentClick);
ReactTestUtils.Simulate.click(CHILD);
CHILD.click();
expect(handleParentClick).toHaveBeenCalledTimes(1);
});

Expand All @@ -310,7 +318,7 @@ describe('ReactBrowserEventEmitter', () => {
putListener(PARENT, ON_CLICK_KEY, handleParentClick);
};
putListener(CHILD, ON_CLICK_KEY, handleChildClick);
ReactTestUtils.Simulate.click(CHILD);
CHILD.click();
expect(handleParentClick).toHaveBeenCalledTimes(0);
});

Expand Down
8 changes: 6 additions & 2 deletions packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1065,16 +1065,20 @@ describe('ReactComponentLifeCycle', () => {
}
}

ReactTestUtils.renderIntoDocument(<Parent />);
const container = document.createElement('div');
document.body.appendChild(container);
ReactDOM.render(<Parent />, container);
expect(divRef.current.textContent).toBe('remote:0, local:0');

// Trigger setState() calls
childInstance.updateState();
expect(divRef.current.textContent).toBe('remote:1, local:1');

// Trigger batched setState() calls
ReactTestUtils.Simulate.click(divRef.current);
divRef.current.click();
expect(divRef.current.textContent).toBe('remote:2, local:2');

document.body.removeChild(container);
});

it('should pass the return value from getSnapshotBeforeUpdate to componentDidUpdate', () => {
Expand Down
8 changes: 5 additions & 3 deletions packages/react-dom/src/__tests__/ReactCompositeComponent-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,13 +140,15 @@ describe('ReactCompositeComponent', () => {
});

it('should react to state changes from callbacks', () => {
const instance = ReactTestUtils.renderIntoDocument(<MorphingComponent />);
const container = document.createElement('div');
document.body.appendChild(container);
const instance = ReactDOM.render(<MorphingComponent />, container);
let el = ReactDOM.findDOMNode(instance);
expect(el.tagName).toBe('A');

ReactTestUtils.Simulate.click(el);
el.click();
el = ReactDOM.findDOMNode(instance);
expect(el.tagName).toBe('B');
document.body.removeChild(container);
});

it('should rewire refs when rendering to different child types', () => {
Expand Down
4 changes: 1 addition & 3 deletions packages/react-dom/src/__tests__/ReactCompositeComponentNestedState-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,11 @@

let React;
let ReactDOM;
let ReactTestUtils;

describe('ReactCompositeComponentNestedState-state', () => {
beforeEach(() => {
React = require('react');
ReactDOM = require('react-dom');
ReactTestUtils = require('react-dom/test-utils');
});

it('should provide up to date values for props', () => {
Expand Down Expand Up @@ -102,7 +100,7 @@ describe('ReactCompositeComponentNestedState-state', () => {
void ReactDOM.render(<ParentComponent logger={logger} />, container);

// click "light green"
ReactTestUtils.Simulate.click(container.childNodes[0].childNodes[3]);
container.childNodes[0].childNodes[3].click();

expect(logger.mock.calls).toEqual([
['parent-render', 'blue'],
Expand Down
13 changes: 5 additions & 8 deletions packages/react-dom/src/__tests__/ReactDOM-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,15 +308,9 @@ describe('ReactDOM', () => {
it("shouldn't fire duplicate event handler while handling other nested dispatch", () => {
const actual = [];

function click(node) {
ReactTestUtils.Simulate.click(node, {
path: [node, container],
});
}

class Wrapper extends React.Component {
componentDidMount() {
click(this.ref1);
this.ref1.click();
}

render() {
Expand All @@ -325,7 +319,7 @@ describe('ReactDOM', () => {
<div
onClick={() => {
actual.push('1st node clicked');
click(this.ref2);
this.ref2.click();
}}
ref={ref => (this.ref1 = ref)}
/>
Expand All @@ -341,13 +335,16 @@ describe('ReactDOM', () => {
}

const container = document.createElement('div');
document.body.appendChild(container);
ReactDOM.render(<Wrapper />, container);

const expected = [
'1st node clicked',
"2nd node clicked imperatively from 1st's handler",
];
expect(actual).toEqual(expected);

document.body.removeChild(container);
});

it('should not crash with devtools installed', () => {
Expand Down
48 changes: 31 additions & 17 deletions packages/react-dom/src/__tests__/ReactDOMFiber-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

const React = require('react');
const ReactDOM = require('react-dom');
const ReactTestUtils = require('react-dom/test-utils');
const PropTypes = require('prop-types');

describe('ReactDOMFiber', () => {
Expand Down Expand Up @@ -843,6 +842,7 @@ describe('ReactDOMFiber', () => {

it('should bubble events from the portal to the parent', () => {
const portalContainer = document.createElement('div');
document.body.appendChild(portalContainer);

const ops = [];
let portal = null;
Expand All @@ -863,13 +863,17 @@ describe('ReactDOMFiber', () => {

expect(portal.tagName).toBe('DIV');

ReactTestUtils.Simulate.click(portal);
portal.click();

expect(ops).toEqual(['portal clicked', 'parent clicked']);

document.body.removeChild(portalContainer);
});

it('should not onMouseLeave when staying in the portal', () => {
const portalContainer = document.createElement('div');
document.body.appendChild(container);
document.body.appendChild(portalContainer);

let ops = [];
let firstTarget = null;
Expand All @@ -878,14 +882,22 @@ describe('ReactDOMFiber', () => {

function simulateMouseMove(from, to) {
if (from) {
ReactTestUtils.SimulateNative.mouseOut(from, {
relatedTarget: to,
});
from.dispatchEvent(
new MouseEvent('mouseout', {
bubbles: true,
cancelable: true,
relatedTarget: to,
}),
);
}
if (to) {
ReactTestUtils.SimulateNative.mouseOver(to, {
relatedTarget: from,
});
to.dispatchEvent(
new MouseEvent('mouseover', {
bubbles: true,
cancelable: true,
relatedTarget: from,
}),
);
}
}

Expand Down Expand Up @@ -928,6 +940,9 @@ describe('ReactDOMFiber', () => {
'leave portal',
'leave parent', // Only when we leave the portal does onMouseLeave fire.
]);

document.body.removeChild(container);
document.body.removeChild(portalContainer);
});

it('should throw on bad createPortal argument', () => {
Expand Down Expand Up @@ -967,6 +982,7 @@ describe('ReactDOMFiber', () => {
});

it('should not update event handlers until commit', () => {
document.body.appendChild(container);
let ops = [];
const handlerA = () => ops.push('A');
const handlerB = () => ops.push('B');
Expand All @@ -988,7 +1004,7 @@ describe('ReactDOMFiber', () => {
class Click extends React.Component {
constructor() {
super();
click(node);
node.click();
}
render() {
return null;
Expand All @@ -1000,27 +1016,23 @@ describe('ReactDOMFiber', () => {
const node = container.firstChild;
expect(node.tagName).toEqual('DIV');

function click(target) {
ReactTestUtils.Simulate.click(target);
}

click(node);
node.click();

expect(ops).toEqual(['A']);
ops = [];

// Render with the other event handler.
inst.flip();

click(node);
node.click();

expect(ops).toEqual(['B']);
ops = [];

// Rerender without changing any props.
inst.tick();

click(node);
node.click();

expect(ops).toEqual(['B']);
ops = [];
Expand All @@ -1040,8 +1052,10 @@ describe('ReactDOMFiber', () => {
ops = [];

// Any click that happens after commit, should invoke A.
click(node);
node.click();
expect(ops).toEqual(['A']);

document.body.removeChild(container);
});

it('should not crash encountering low-priority tree', () => {
Expand Down

0 comments on commit dfe5d83

Please sign in to comment.