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

dleitee · 2017-10-23T02:52:33Z

Related to #11299 – Express more tests via public API

Hey!! I'm not sure if my code is correct, but I would like to know if I can continue in this way or there is something that I need change.

Thanks 😄

facebook-github-bot · 2017-10-23T02:52:40Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2017-10-23T03:00:05Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

dleitee · 2017-10-23T16:42:13Z

I've updated this branch removing the ReactTestUtils.SimulateNative.
Are there some problems to use the Event class?

I can't remove the ReactTestUtils.SimulateNative here (https://github.com/facebook/react/pull/11332/files#diff-288fef2ade58f58c8d1d12571c4c80beR87) because these params doesn't exist on EventImpl from jsdom.

gaearon · 2017-10-26T20:01:51Z

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

+        cancelable: true,
+        ...nativeEvent,
+      };
+      var event = new Event([eventType, defaultNativeEvent]);


Why is there an array here? Shouldn't it be just new Event(eventType, defaultNativeEvent)?

It would be nice to rename defaultNativeEvent and nativeEvent to eventInit and defaultEventInit, to match how spec calls these arguments.

Why is there an array here? Shouldn't it be just new Event(eventType, defaultNativeEvent)?

I was using arrays because I've seen the Event's constructor on jsdom and the first argument is args and it is an array: https://github.com/tmpvar/jsdom/blob/2d51af302581a57ee5b9b65595f1714d669b7ea2/lib/jsdom/living/events/Event-impl.js#L7

but no problem to change it :D

It would be nice to rename defaultNativeEvent and nativeEvent to eventInit and defaultEventInit, to match how spec calls these arguments.

Ok

gaearon · 2017-10-26T20:04:13Z

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

+    document.body.appendChild(container);
+
+    var event = createEvent('click', {srcElement: instance});
+    var elem = ReactDOM.findDOMNode(instance);


instance is the DOM element here. You can remove findDOMNodes.

gaearon · 2017-10-26T20:12:07Z

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

+    var event = createEvent('click', {srcElement: instance});
+    var elem = ReactDOM.findDOMNode(instance);
+    elem.dispatchEvent(event);
+    expect(click).toBeCalledWith(elem);


I don't think this test works. If I comment out || nativeEvent.srcElement in getEventTarget.js the test still passes, whereas on master commenting it out fails the test. This means that dispatchEvent() probably fills .target it on the native event and thus it's not a problem.

I'm not sure how to work around that really. Maybe there's no way.

gaearon · 2017-10-26T20:12:52Z

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

  it('should be able to `preventDefault`', () => {
-    var nativeEvent = {};
-    var syntheticEvent = createEvent(nativeEvent);
+    var click = jest.fn();


Can we call this mock? It's a bit confusing to see click() calls like we're clicking on something.

gaearon · 2017-10-26T20:13:48Z

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

-    expect(syntheticEvent.defaultPrevented).toBe(true);
+    var instance = ReactDOM.render(<div onClick={onClick} />, container);

-    expect(nativeEvent.returnValue).toBe(false);


It seems like we lost these two assertions on e.defaultPrevented and e.nativeEvent.returnValue? Why?

@gaearon there is no value on e.nativeEvent.returnValue, I think it's not testable.

what do you think?

could I use it: ReactTestUtils.SimulateNative.click(instance, {returnValue: false});

gaearon · 2017-10-26T20:14:34Z

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

-    expect(nativeEvent.returnValue).toBe(false);
+    document.body.appendChild(container);
+
+    var event = createEvent('click', {srcElement: instance});


Only the first test was checking for srcElement. There shouldn't be a need to add this to every test.

gaearon · 2017-10-26T20:15:35Z

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

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

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


I don't see this being tested (e.nativeEvent.cancelBubble)

gaearon · 2017-10-26T20:15:43Z

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

-    expect(nativeEvent.cancelBubble).toBe(true);
+    document.body.appendChild(container);
+
+    var event = createEvent('click', {srcElement: instance});


Same re: srcElement.

gaearon · 2017-10-26T20:15:54Z

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

-    expect(syntheticEvent.isPersistent()).toBe(false);
-    syntheticEvent.persist();
-    expect(syntheticEvent.isPersistent()).toBe(true);
+    var event = createEvent('click', {srcElement: instance});


gaearon · 2017-10-31T14:21:11Z

Looks like tests are still failing. Do you plan to continue working on this?

dleitee · 2017-10-31T15:50:50Z

@gaearon yes, I've planned to continue it today and tomorrow.

gaearon · 2017-11-03T20:44:47Z

Friendly ping :-)

gaearon · 2017-11-08T22:10:33Z

Second ping. If you're too busy, it's okay—just let us know and somebody can take it from here.

gaearon · 2017-11-10T12:53:39Z

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


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


In #11332 (comment), I didn't ask to remove srcElement from all tests like you did in 3b3db3f.

I was saying that only one test intentionally tested for srcElement support (that's why the test existed). That's why after refactoring, there should also be one test that verifies this.

This test verified that this line can fall back to nativeEvent.srcElement. After the changes, the new test doesn't check this.

gaearon · 2017-11-10T16:41:24Z

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

-    expect(nativeEvent.returnValue).toBe(false);
+    var event = createEvent('click');
+    instance.dispatchEvent(event);
+    expect(click.mock.calls[0][0]).toBe(false);


This is not testing anything in React. You are creating a jsdom event (equivalent to a browser event) and then check that jsdom was dispatched. Even if the entire React source code was deleted, this test would still pass.

gaearon · 2017-11-10T16:43:01Z

For the reference, #11365 and #11299 might help demonstrate the suggested approach for these kinds of tests.

timjacobi · 2017-11-11T01:08:36Z

I have submitted a PR in #11525

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

@dleitee

Thank you for your help! This PR is getting stale and since you're not responding, we're going to continue with #11525 instead. I appreciate your effort and hope you can contribute in the future!

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

dleitee force-pushed the synthetic-event-public-api branch from aaed094 to baaf16f Compare October 23, 2017 16:22

dleitee added 2 commits October 23, 2017 14:41

use public api for SyntheticEvent-test

fbe7fd5

fix typo

9453a9d

dleitee force-pushed the synthetic-event-public-api branch from baaf16f to 9453a9d Compare October 23, 2017 16:42

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

rename vars and remove array from Event constructor

0a68476

dleitee force-pushed the synthetic-event-public-api branch from 7d6f12e to 0a68476 Compare October 26, 2017 22:02

dleitee added 4 commits October 26, 2017 20:05

use instance instead of ReactDOM.findDOMNode

02d2981

remove srcElement from all tests

3b3db3f

add assert to e.defaultPrevented

4e5d5cd

add assert to e.nativeEvent.returnValue

6e8de79

gaearon reviewed Nov 10, 2017

View reviewed changes

gaearon requested changes Nov 10, 2017

View reviewed changes

gaearon closed this Nov 11, 2017


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

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

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

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

Uh oh!

Conversation

dleitee commented Oct 23, 2017

Uh oh!

facebook-github-bot commented Oct 23, 2017

Uh oh!

facebook-github-bot commented Oct 23, 2017

Uh oh!

dleitee commented Oct 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Oct 31, 2017

Uh oh!

dleitee commented Oct 31, 2017

Uh oh!

gaearon commented Nov 3, 2017

Uh oh!

gaearon commented Nov 8, 2017

Uh oh!

gaearon Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Nov 10, 2017

Uh oh!

timjacobi commented Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Nov 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gaearon Nov 10, 2017 •

edited

Loading

timjacobi commented Nov 11, 2017 •

edited

Loading