Rewrite SyntheticEvent tests using public APIs only #11525

timjacobi · 2017-11-11T01:07:25Z

Related to #11299

rewritten createEvent to use public APIs
removed all references SyntheticEvent.release

In order to test under realistic circumstances I had to move the expectations into a callback in mosts tests to overcome the effects of event pooling.

* rewritten createEvent to use public APIs * removed all references SyntheticEvent.release In order to test under realistic circumstances I had to move the expectations into a callback in mosts tests to overcome the effects of event pooling.

gaearon · 2017-11-11T12:20:27Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

-    createEvent = function(nativeEvent) {
-      var target = require('../getEventTarget').default(nativeEvent);
-      return SyntheticEvent.getPooled({}, '', nativeEvent, target);
+    createEvent = (nativeEvent, callback) => {


Can we get rid of this helper and just inline calls in each test? I don't mind some repetition.

gaearon · 2017-11-11T12:21:29Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

-      return SyntheticEvent.getPooled({}, '', nativeEvent, target);
+    createEvent = (nativeEvent, callback) => {
+      var instance;
+      var container = document.createElement('div');


I would like to use the same beforeEach/afterEach structure as in #11365. Set up a container and add it to the document, then remove it.

gaearon · 2017-11-11T12:22:43Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

+        <div ref={ref => (instance = ref)} onClick={callback} />,
+        container,
+      );
+      ReactTestUtils.SimulateNative.click(instance, nativeEvent);


Unfortunately Simulate* APIs are not very useful for testing because they are sort of like private APIs too. If you look at #11365 you'll see we're trying to avoid them and dispatch native browser events on DOM nodes instead. This PR should too.

gaearon · 2017-11-11T12:23:10Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

    };
  });

-  it('should normalize `target` from the nativeEvent', () => {
+  // TODO: This is not testable with the public APIs, write a test for getEventTarget


Why is it not testable? Pretty sure #11365 tests this case.

timjacobi · 2017-11-11T15:43:06Z

Thanks for the comments. It's all understood and I'm working on an update.

timjacobi · 2017-11-11T17:04:56Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

+    });
+    instance.dispatchEvent(event);
+
+    // TODO: figure out why this fails


I'm currently investigating this.

timjacobi · 2017-11-11T17:05:08Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

+      expect(syntheticEvent.isPropagationStopped()).toBe(false);
+      syntheticEvent.stopPropagation();
+      expect(syntheticEvent.isPropagationStopped()).toBe(true);
+      // TODO: Figure out why this is undefined when switching to public API


I'm currently investigating this.

timjacobi · 2017-11-11T17:05:15Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

+      expect(syntheticEvent.isPersistent()).toBe(false);
+      syntheticEvent.persist();
+      expect(syntheticEvent.isPersistent()).toBe(true);
+      // TODO: Figure out why this is undefined when switching to public API


I'm currently investigating this.

The removed `expect` calls verified the correct behaviour based on missing `preventDefault` and `stopPropagation` methods. The was correct as we used plain objects to simulate events. Since we switched to the public API we're using native events which do have these methods.

This was missed when the test was first migrated. When emulating IE8 not only has returnValue to be false. In addition defaultPrevented must not be defined.

timjacobi · 2017-11-11T17:39:00Z

All TODOs resolved. Ready to review again 🙂

gaearon

Thanks! Got more comments.

gaearon · 2017-11-19T14:56:40Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

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


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

gaearon · 2017-11-19T14:56:49Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js


-    expect(syntheticEvent.defaultPrevented).toBe(true);
+      expectedCount++;
+    };


gaearon · 2017-11-19T14:57:14Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

+    instance = ReactDOM.render(<div onClick={eventHandler} />, container);
+
+    var event;
+    event = document.createEvent('Event');


Style nit: let's include assignment into definition?

gaearon · 2017-11-19T15:01:15Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

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


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().

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.

gaearon · 2017-11-19T15:02:42Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

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


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.

gaearon · 2017-11-19T15:02:52Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

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


gaearon · 2017-11-19T15:04:21Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js


-  it('should be able to `persist`', () => {


Is this test missing?

timjacobi · 2017-11-20T11:30:48Z

Totally agree with your comments. Just should have focused this tiny bit more ;-)
I added one question regarding the approach for should be prevented if nativeEvent is prevented otherwise this is up to date with your requests.

gaearon · 2017-11-20T12:37:45Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

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


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

gaearon · 2017-11-20T12:39:24Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

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


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.

gaearon · 2017-11-20T12:41:15Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

+    var expectedCount = 0;
+
+    var eventHandler = syntheticEvent => {
+      syntheticEvent.destructor();


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.

gaearon · 2017-11-20T12:41:30Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

+    var expectedCount = 0;
+
+    var eventHandler = syntheticEvent => {
+      syntheticEvent.destructor();


gaearon · 2017-11-20T12:42:21Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

+
+    var eventHandler = syntheticEvent => {
+      syntheticEvent.destructor();
+      expect((syntheticEvent.type = 'MouseEvent')).toBe('MouseEvent');


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

gaearon · 2017-11-20T12:43:04Z

packages/react-dom/src/events/__tests__/SyntheticEvent-test.js

+    event = document.createEvent('Event');
+    event.initEvent('click', true, true);
+    // Emulate IE8
+    Object.defineProperty(event, 'defaultPrevented', {


This makes sense to me now, thanks for explaining.

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.

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).

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

gaearon · 2017-11-20T14:29:43Z

I think this is good. Thank you!

timjacobi · 2017-11-20T15:07:11Z

No worries an thank you for holding out during my first PR here :-)

* generate synthetics events using public API * rewritten createEvent to use public APIs * removed all references SyntheticEvent.release In order to test under realistic circumstances I had to move the expectations into a callback in mosts tests to overcome the effects of event pooling. * run prettier * remove empty line * don't use ReactTestUtils * run prettier and fix linter issues * remove duplicate test * remove invalid calls to expect The removed `expect` calls verified the correct behaviour based on missing `preventDefault` and `stopPropagation` methods. The was correct as we used plain objects to simulate events. Since we switched to the public API we're using native events which do have these methods. * set event.defaultPrevented to undefined This was missed when the test was first migrated. When emulating IE8 not only has returnValue to be false. In addition defaultPrevented must not be defined. * run all tests and format code * rename instance variable to node * remove backtick * only simulate IE in normalisation test * include assignment in definition * add missing `persist` test * use method instead of field to prevent default * expect properties to be unchanged on persisted event * optimise tests that deal with event persitence * declare and assign `event` on the same line if not reassigned later

timjacobi added 2 commits November 11, 2017 00:57

generate synthetics events using public API

7236c56

* rewritten createEvent to use public APIs * removed all references SyntheticEvent.release In order to test under realistic circumstances I had to move the expectations into a callback in mosts tests to overcome the effects of event pooling.

run prettier

c4cdb2a

facebook-github-bot added the CLA Signed label Nov 11, 2017

timjacobi mentioned this pull request Nov 11, 2017

(WIP) Use only public api for SyntheticEvent-test.js #11332

Closed

remove empty line

8d6f118

gaearon requested changes Nov 11, 2017

View reviewed changes

timjacobi added 2 commits November 11, 2017 16:05

don't use ReactTestUtils

ff7a9ea

run prettier and fix linter issues

fb0e822

timjacobi commented Nov 11, 2017

View reviewed changes

timjacobi added 3 commits November 11, 2017 17:22

remove duplicate test

cf2ac41

set event.defaultPrevented to undefined

b83719b

This was missed when the test was first migrated. When emulating IE8 not only has returnValue to be false. In addition defaultPrevented must not be defined.

run all tests and format code

1758de2

gaearon requested changes Nov 19, 2017

View reviewed changes

timjacobi added 5 commits November 20, 2017 11:16

rename instance variable to node

613e878

remove backtick

0697458

only simulate IE in normalisation test

850d368

include assignment in definition

2005d08

add missing persist test

7341de0

gaearon requested changes Nov 20, 2017

View reviewed changes

timjacobi added 4 commits November 20, 2017 13:18

use method instead of field to prevent default

4fc11e9

expect properties to be unchanged on persisted event

f39999b

optimise tests that deal with event persitence

0421e59

declare and assign event on the same line if not reassigned later

bf69039

gaearon approved these changes Nov 20, 2017

View reviewed changes

gaearon merged commit 669a70d into facebook:master Nov 20, 2017

timjacobi deleted the synthetic-event-public-api branch November 20, 2017 15:06

gaearon mentioned this pull request Nov 20, 2017

Express more tests via public API #11299

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite SyntheticEvent tests using public APIs only #11525

Rewrite SyntheticEvent tests using public APIs only #11525

timjacobi commented Nov 11, 2017 •

edited

Loading

gaearon Nov 11, 2017

gaearon Nov 11, 2017

gaearon Nov 11, 2017

gaearon Nov 11, 2017

timjacobi commented Nov 11, 2017

timjacobi Nov 11, 2017

timjacobi Nov 11, 2017

timjacobi Nov 11, 2017

timjacobi commented Nov 11, 2017

gaearon left a comment

gaearon Nov 19, 2017

gaearon Nov 19, 2017

gaearon Nov 19, 2017

gaearon Nov 19, 2017

timjacobi Nov 20, 2017 •

edited

Loading

gaearon Nov 19, 2017

gaearon Nov 19, 2017

gaearon Nov 19, 2017

timjacobi commented Nov 20, 2017

gaearon Nov 20, 2017

gaearon Nov 20, 2017

gaearon Nov 20, 2017

gaearon Nov 20, 2017

gaearon Nov 20, 2017

gaearon Nov 20, 2017

timjacobi Nov 20, 2017

gaearon Nov 20, 2017

gaearon Nov 20, 2017

gaearon commented Nov 20, 2017

timjacobi commented Nov 20, 2017

Rewrite SyntheticEvent tests using public APIs only #11525

Rewrite SyntheticEvent tests using public APIs only #11525

Conversation

timjacobi commented Nov 11, 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

timjacobi commented Nov 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timjacobi commented Nov 11, 2017

gaearon left a 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

timjacobi Nov 20, 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

timjacobi commented Nov 20, 2017

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

gaearon commented Nov 20, 2017

timjacobi commented Nov 20, 2017

timjacobi commented Nov 11, 2017 •

edited

Loading

timjacobi Nov 20, 2017 •

edited

Loading