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 5 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
306 changes: 236 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,249 @@

'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 instance;
var expectedCount = 0;

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

expectedCount++;
};
instance = ReactDOM.render(<div onClick={eventHandler} />, container);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's rename this to node. It's not a React instance.


expect(syntheticEvent.target).toBe(target);
expect(syntheticEvent.type).toBe(undefined);
var event;
event = document.createEvent('Event');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style nit: let's include assignment into definition?

event.initEvent('click', true, true);
// Emulate IE8
Object.defineProperty(event, 'target', {
get() {},
});
Object.defineProperty(event, 'srcElement', {
get() {
return instance;
},
});
instance.dispatchEvent(event);

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

it('should be able to `preventDefault`', () => {
var nativeEvent = {};
var syntheticEvent = createEvent(nativeEvent);
var instance;
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);
// TODO: Figure out why this is undefined when switching to public API
// expect(nativeEvent.returnValue).toBe(false);

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.

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

expect(nativeEvent.returnValue).toBe(false);
var event;
event = document.createEvent('Event');
event.initEvent('click', true, true);
instance.dispatchEvent(event);

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 instance;
var expectedCount = 0;

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

expectedCount++;
};
instance = 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;
},
});
instance.dispatchEvent(event);

// TODO: figure out why this fails
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'm currently investigating this.

// event = document.createEvent('Event');
// event.initEvent('click', true, true);
// Object.defineProperty(event, 'returnValue', {
// get(){
// return false;
// }
// });
// instance.dispatchEvent(event);

// TODO: change to 2 when problem above is resolved
expect(expectedCount).toBe(1);
});

it('should be able to `stopPropagation`', () => {
var nativeEvent = {};
var syntheticEvent = createEvent(nativeEvent);
var instance;
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);
// TODO: Figure out why this is undefined when switching to public API
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'm currently investigating this.

// expect(nativeEvent.cancelBubble).toBe(true);

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

var event;
event = document.createEvent('Event');
event.initEvent('click', true, true);
instance.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({});
it('should be able to `stopPropagation`', () => {
var instance;
var expectedCount = 0;

var eventHandler = syntheticEvent => {
expect(syntheticEvent.isPersistent()).toBe(false);
syntheticEvent.persist();
expect(syntheticEvent.isPersistent()).toBe(true);
// TODO: Figure out why this is undefined when switching to public API
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'm currently investigating this.

// expect(nativeEvent.cancelBubble).toBe(true);

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

var event;
event = document.createEvent('Event');
event.initEvent('click', true, true);
instance.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(syntheticEvent.isPersistent()).toBe(false);
syntheticEvent.persist();
expect(syntheticEvent.isPersistent()).toBe(true);
expect(expectedCount).toBe(1);
});

it('should be nullified if the synthetic event has called destructor and log warnings', () => {
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 instance;
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++;
};
instance = ReactDOM.render(<div onClick={eventHandler} />, container);

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

Choose a reason for hiding this comment

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

Again, this is specific to IE8 and there is just a single test needed for this. There's no need to add srcElement testing to every test.

get() {
return instance;
},
});
instance.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 instance;
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++;
};
instance = ReactDOM.render(<div onClick={eventHandler} />, container);

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

Choose a reason for hiding this comment

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

Same.

get() {
return instance;
},
});
instance.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 instance;
var expectedCount = 0;
var syntheticEvent;

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

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

syntheticEvent.preventDefault();
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toBe(
Expand All @@ -126,12 +261,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 instance;
var expectedCount = 0;
var syntheticEvent;

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

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

instance.dispatchEvent(event);

syntheticEvent.stopPropagation();
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toBe(
Expand All @@ -141,6 +291,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 Down Expand Up @@ -172,9 +323,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 instance;
var expectedCount = 0;
var syntheticEvent;

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

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

instance.dispatchEvent(event);

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