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.

Sign up for GitHub

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

Rewrite SyntheticEvent tests using public APIs only #11525

Merged
merged 18 commits into from
Nov 20, 2017
Merged

Rewrite SyntheticEvent tests using public APIs only #11525

Changes from 14 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
7236c56
generate synthetics events using public API
timjacobi Nov 11, 2017
c4cdb2a
run prettier
timjacobi Nov 11, 2017
8d6f118
remove empty line
timjacobi Nov 11, 2017
ff7a9ea
don't use ReactTestUtils
timjacobi Nov 11, 2017
fb0e822
run prettier and fix linter issues
timjacobi Nov 11, 2017
cf2ac41
remove duplicate test
timjacobi Nov 11, 2017
220d369
remove invalid calls to expect
timjacobi Nov 11, 2017
b83719b
set event.defaultPrevented to undefined
timjacobi Nov 11, 2017
1758de2
run all tests and format code
timjacobi Nov 11, 2017
613e878
rename instance variable to node
timjacobi Nov 20, 2017
0697458
remove backtick
timjacobi Nov 20, 2017
850d368
only simulate IE in normalisation test
timjacobi Nov 20, 2017
2005d08
include assignment in definition
timjacobi Nov 20, 2017
7341de0
add missing `persist` test
timjacobi Nov 20, 2017
4fc11e9
use method instead of field to prevent default
timjacobi Nov 20, 2017
f39999b
expect properties to be unchanged on persisted event
timjacobi Nov 20, 2017
0421e59
optimise tests that deal with event persitence
timjacobi Nov 20, 2017
bf69039
declare and assign `event` on the same line if not reassigned later
timjacobi Nov 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
291 changes: 221 additions & 70 deletions packages/react-dom/src/events/__tests__/SyntheticEvent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,114 +9,234 @@

'use strict';

var SyntheticEvent;
var React;
var ReactDOM;
var ReactTestUtils;

describe('SyntheticEvent', () => {
var createEvent;
var container;

beforeEach(() => {
// TODO: can we express this test with only public API?
SyntheticEvent = require('events/SyntheticEvent').default;
React = require('react');
ReactDOM = require('react-dom');
ReactTestUtils = require('react-dom/test-utils');

createEvent = function(nativeEvent) {
var target = require('../getEventTarget').default(nativeEvent);
return SyntheticEvent.getPooled({}, '', nativeEvent, target);
};
container = document.createElement('div');
document.body.appendChild(container);
});

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

it('should normalize `target` from the nativeEvent', () => {
var target = document.createElement('div');
var syntheticEvent = createEvent({srcElement: target});
var node;
var expectedCount = 0;

var eventHandler = syntheticEvent => {
expect(syntheticEvent.target).toBe(node);

expectedCount++;
};
node = ReactDOM.render(<div onClick={eventHandler} />, container);

var event = document.createEvent('Event');
event.initEvent('click', true, true);
// Emulate IE8
Object.defineProperty(event, 'target', {
get() {},
});
Object.defineProperty(event, 'srcElement', {
get() {
return node;
},
});
node.dispatchEvent(event);

expect(syntheticEvent.target).toBe(target);
expect(syntheticEvent.type).toBe(undefined);
expect(expectedCount).toBe(1);
});

it('should be able to `preventDefault`', () => {
var nativeEvent = {};
var syntheticEvent = createEvent(nativeEvent);
var node;
var expectedCount = 0;

expect(syntheticEvent.isDefaultPrevented()).toBe(false);
syntheticEvent.preventDefault();
expect(syntheticEvent.isDefaultPrevented()).toBe(true);
var eventHandler = syntheticEvent => {
expect(syntheticEvent.isDefaultPrevented()).toBe(false);
syntheticEvent.preventDefault();
expect(syntheticEvent.isDefaultPrevented()).toBe(true);
expect(syntheticEvent.defaultPrevented).toBe(true);

expectedCount++;
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

node = ReactDOM.render(<div onClick={eventHandler} />, container);

expect(syntheticEvent.defaultPrevented).toBe(true);
var event;
event = document.createEvent('Event');
event.initEvent('click', true, true);
node.dispatchEvent(event);

expect(nativeEvent.returnValue).toBe(false);
expect(expectedCount).toBe(1);
});

it('should be prevented if nativeEvent is prevented', () => {
expect(createEvent({defaultPrevented: true}).isDefaultPrevented()).toBe(
true,
);
expect(createEvent({returnValue: false}).isDefaultPrevented()).toBe(true);
var node;
var expectedCount = 0;

var eventHandler = syntheticEvent => {
expect(syntheticEvent.isDefaultPrevented()).toBe(true);

expectedCount++;
};
node = ReactDOM.render(<div onClick={eventHandler} />, container);

var event;
event = document.createEvent('Event');
event.initEvent('click', true, true);
Object.defineProperty(event, 'defaultPrevented', {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be event.preventDefault() call instead? Since jsdom should implement it by providing .defaultPrevented.

get() {
return true;
},
});
node.dispatchEvent(event);

event = document.createEvent('Event');
event.initEvent('click', true, true);
// Emulate IE8
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this test have to do with IE8?

The test title says "should be prevented if nativeEvent is prevented". To test this, remove these properties and just call e.nativeEvent.preventDefault(). Then check e.isDefaultPrevented().

Copy link
Member Author

@timjacobi timjacobi Nov 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous version of this test had two expectations. On where defaultPrevented was true and one where returnValue was false. I checked why and it turned out IE uses returnValue instead of defaultPrevented. Happy to use e.nativeEvent.preventDefault() but it will have less parity with the previous version of the test.

Object.defineProperty(event, 'defaultPrevented', {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me now, thanks for explaining.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that we have dropped support for IE8 and IE9 doesn't use srcElement anymore so we could simplify this like you have suggested initially. Your call.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to keep the test for srcElement. You're right we don't support IE8 anymore but the code is there so the test should be there too. We can later remove them together but that is unrelated to the task at hand (converting the test to use public API).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words I'd like to leave this part tested.

get() {},
});
Object.defineProperty(event, 'returnValue', {
get() {
return false;
},
});
node.dispatchEvent(event);

expect(expectedCount).toBe(2);
});

it('should be able to `stopPropagation`', () => {
var nativeEvent = {};
var syntheticEvent = createEvent(nativeEvent);
var node;
var expectedCount = 0;

expect(syntheticEvent.isPropagationStopped()).toBe(false);
syntheticEvent.stopPropagation();
expect(syntheticEvent.isPropagationStopped()).toBe(true);
var eventHandler = syntheticEvent => {
expect(syntheticEvent.isPropagationStopped()).toBe(false);
syntheticEvent.stopPropagation();
expect(syntheticEvent.isPropagationStopped()).toBe(true);

expect(nativeEvent.cancelBubble).toBe(true);
expectedCount++;
};
node = ReactDOM.render(<div onClick={eventHandler} />, container);

var event;
event = document.createEvent('Event');
event.initEvent('click', true, true);
node.dispatchEvent(event);

expect(expectedCount).toBe(1);
});

it('should be able to `persist`', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this test missing?

var syntheticEvent = createEvent({});
var node;
var expectedCount = 0;

var eventHandler = syntheticEvent => {
expect(syntheticEvent.isPersistent()).toBe(false);
syntheticEvent.persist();
expect(syntheticEvent.isPersistent()).toBe(true);

expect(syntheticEvent.isPersistent()).toBe(false);
syntheticEvent.persist();
expect(syntheticEvent.isPersistent()).toBe(true);
expectedCount++;
};
node = ReactDOM.render(<div onClick={eventHandler} />, container);

var event;
event = document.createEvent('Event');
event.initEvent('click', true, true);
node.dispatchEvent(event);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to test that we can still access syntheticEvent fields here. Since that's the thing that persist allows.

Let's save syntheticEvent in a variable and asset its target and type here.

expect(expectedCount).toBe(1);
});

it('should be nullified if the synthetic event has called destructor and log warnings', () => {
spyOn(console, 'error');
var target = document.createElement('div');
var syntheticEvent = createEvent({srcElement: target});
syntheticEvent.destructor();
expect(syntheticEvent.type).toBe(null);
expect(syntheticEvent.nativeEvent).toBe(null);
expect(syntheticEvent.target).toBe(null);
// once for each property accessed
expectDev(console.error.calls.count()).toBe(3);
// assert the first warning for accessing `type`
expectDev(console.error.calls.argsFor(0)[0]).toBe(
'Warning: This synthetic event is reused for performance reasons. If ' +
"you're seeing this, you're accessing the property `type` on a " +
'released/nullified synthetic event. This is set to null. If you must ' +
'keep the original synthetic event around, use event.persist(). ' +
'See https://fb.me/react-event-pooling for more information.',
);
var node;
var expectedCount = 0;

var eventHandler = syntheticEvent => {
syntheticEvent.destructor();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a private API. Instead of triggering it explicitly we should save the synthetic event to a variable. Then we should assert at the end of the test (after dispatching events) that accessing them does not work.

expect(syntheticEvent.type).toBe(null);
expect(syntheticEvent.nativeEvent).toBe(null);
expect(syntheticEvent.target).toBe(null);
// once for each property accessed
expectDev(console.error.calls.count()).toBe(3);
// assert the first warning for accessing `type`
expectDev(console.error.calls.argsFor(0)[0]).toBe(
'Warning: This synthetic event is reused for performance reasons. If ' +
"you're seeing this, you're accessing the property `type` on a " +
'released/nullified synthetic event. This is set to null. If you must ' +
'keep the original synthetic event around, use event.persist(). ' +
'See https://fb.me/react-event-pooling for more information.',
);

expectedCount++;
};
node = ReactDOM.render(<div onClick={eventHandler} />, container);

var event;
event = document.createEvent('Event');
event.initEvent('click', true, true);
node.dispatchEvent(event);

expect(expectedCount).toBe(1);
});

it('should warn when setting properties of a destructored synthetic event', () => {
spyOn(console, 'error');
var target = document.createElement('div');
var syntheticEvent = createEvent({srcElement: target});
syntheticEvent.destructor();
expect((syntheticEvent.type = 'MouseEvent')).toBe('MouseEvent');
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toBe(
'Warning: This synthetic event is reused for performance reasons. If ' +
"you're seeing this, you're setting the property `type` on a " +
'released/nullified synthetic event. This is effectively a no-op. If you must ' +
'keep the original synthetic event around, use event.persist(). ' +
'See https://fb.me/react-event-pooling for more information.',
);
var node;
var expectedCount = 0;

var eventHandler = syntheticEvent => {
syntheticEvent.destructor();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

expect((syntheticEvent.type = 'MouseEvent')).toBe('MouseEvent');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this asserting? I don't understand this line.

expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toBe(
'Warning: This synthetic event is reused for performance reasons. If ' +
"you're seeing this, you're setting the property `type` on a " +
'released/nullified synthetic event. This is effectively a no-op. If you must ' +
'keep the original synthetic event around, use event.persist(). ' +
'See https://fb.me/react-event-pooling for more information.',
);

expectedCount++;
};
node = ReactDOM.render(<div onClick={eventHandler} />, container);

var event;
event = document.createEvent('Event');
event.initEvent('click', true, true);
node.dispatchEvent(event);

expect(expectedCount).toBe(1);
});

it('should warn if the synthetic event has been released when calling `preventDefault`', () => {
spyOn(console, 'error');
var syntheticEvent = createEvent({});
SyntheticEvent.release(syntheticEvent);
var node;
var expectedCount = 0;
var syntheticEvent;

var eventHandler = e => {
syntheticEvent = e;
expectedCount++;
};
node = ReactDOM.render(<div onClick={eventHandler} />, container);

var event;
event = document.createEvent('Event');
event.initEvent('click', true, true);
node.dispatchEvent(event);

syntheticEvent.preventDefault();
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toBe(
Expand All @@ -126,12 +246,27 @@ describe('SyntheticEvent', () => {
'keep the original synthetic event around, use event.persist(). ' +
'See https://fb.me/react-event-pooling for more information.',
);
expect(expectedCount).toBe(1);
});

it('should warn if the synthetic event has been released when calling `stopPropagation`', () => {
spyOn(console, 'error');
var syntheticEvent = createEvent({});
SyntheticEvent.release(syntheticEvent);
var node;
var expectedCount = 0;
var syntheticEvent;

var eventHandler = e => {
syntheticEvent = e;
expectedCount++;
};
node = ReactDOM.render(<div onClick={eventHandler} />, container);

var event;
event = document.createEvent('Event');
event.initEvent('click', true, true);

node.dispatchEvent(event);

syntheticEvent.stopPropagation();
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toBe(
Expand All @@ -141,6 +276,7 @@ describe('SyntheticEvent', () => {
'keep the original synthetic event around, use event.persist(). ' +
'See https://fb.me/react-event-pooling for more information.',
);
expect(expectedCount).toBe(1);
});

// TODO: reenable this test. We are currently silencing these warnings when
Expand All @@ -153,8 +289,8 @@ describe('SyntheticEvent', () => {
function assignEvent(e) {
event = e;
}
var instance = ReactDOM.render(<div onClick={assignEvent} />, element);
ReactTestUtils.Simulate.click(ReactDOM.findDOMNode(instance));
var node = ReactDOM.render(<div onClick={assignEvent} />, element);
ReactTestUtils.Simulate.click(ReactDOM.findDOMNode(node));
expectDev(console.error.calls.count()).toBe(0);

// access a property to cause the warning
Expand All @@ -172,9 +308,23 @@ describe('SyntheticEvent', () => {

it('should warn if Proxy is supported and the synthetic event is added a property', () => {
spyOn(console, 'error');
var syntheticEvent = createEvent({});
syntheticEvent.foo = 'bar';
SyntheticEvent.release(syntheticEvent);
var node;
var expectedCount = 0;
var syntheticEvent;

var eventHandler = e => {
e.foo = 'bar';
syntheticEvent = e;
expectedCount++;
};
node = ReactDOM.render(<div onClick={eventHandler} />, container);

var event;
event = document.createEvent('Event');
event.initEvent('click', true, true);

node.dispatchEvent(event);

expect(syntheticEvent.foo).toBe('bar');
if (typeof Proxy === 'function') {
expectDev(console.error.calls.count()).toBe(1);
Expand All @@ -187,5 +337,6 @@ describe('SyntheticEvent', () => {
} else {
expectDev(console.error.calls.count()).toBe(0);
}
expect(expectedCount).toBe(1);
});
});