Use only public API for EnterLeaveEventPlugin Tests #11316

accordeiro · 2017-10-21T22:47:11Z

This PR is related to issue #11299 – Express more tests via public API

I've managed to rewrite the EnterLeavePlugin tests using public API, but I'm not sure if I'm on the right path. What I did was basically analyze what was being done inside EnterLeaveEventPlugin.js and move the needed parts to the test (which luckily led to a much simpler code).

Does this make sense? Happy to make any necessary changes or follow a different path if need be.

Thanks for the opportunity of contributing! :)

gaearon

This tests that jsdom works, but doesn't actually test our code. If you make a mistake in EnterLeavePlugin, this test will still pass because it repeats rather than exercises that code path.

You would need to figure out how to make that code path actually happen. For example you could trigger a native event with ReactTestUtils.SimulateNative.mouseOver() and then make an assertion on the e event object in a <div onMouseOver={...} /> handler.

You can see another test here that is doing something similar. Hope this helps.

accordeiro · 2017-10-22T21:41:49Z

Hi, @gaearon – thanks for the thorough feedback! :)

I've just pushed a new commit, trying to follow the path you've suggested. I've noticed two things:

When using ReactTestUtils.SimulateNative, the event's relatedTarget was set to undefined, so I had to set the relatedTarget manually, which made the last assertion a bit redundant. Am I missing something, or do you think this test makes sense anyway?
It seems that triggering the event via ReactTestUtils.SimulateNative isn't actually exercising the EnterLeavePlugin.js codepath (I've put some logs on it to validate this assumption), while the second test on this file – which uses ReactTestUtils.simulateNativeEventOnNode– actually exercises it. Should I rewrite the test to use this other event triggering function?

Thanks!

gaearon · 2017-10-26T18:34:37Z

I think I was wrong actually. SimulateNative is also not great because it bypasses the real browser events (which is what we want to test). Ideally we should just call dispatchEvent on nodes, like #11309 did.

gaearon

Please rewrite both to use dispatchEvent.

#11309 can serve as an inspiration. (You won't need the "value setter" part in it, just the event dispatching.)

accordeiro · 2017-10-26T22:28:24Z

OK, that makes sense – I've rewritten both tests to use dispatchEvent :)

outdated

gaearon · 2017-10-26T22:36:21Z

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

-      div,
-    );
-    expect(extracted.length).toBe(2);
+    simulateMouseEvent(component, 'mouseover', iframe.contentWindow);


Shouldn't it be simulated on the component?
(which should be renamed to node btw)

I believe the original test did that.

gaearon · 2017-10-26T22:39:07Z

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

-    expect(enter.relatedTarget).toBe(iframe.contentWindow);
+    expect(events.length).toBe(1);
+    expect(events[0].target).toBe(component);
+    expect(events[0].relatedTarget).toBe(iframe.contentWindow);


Which part of the code are this testing here? Are we confident that removing it causes test to fail?

gaearon

I think the first test is wrong because it doesn't test EnterLeavePlugin anymore.

EnterLeavePlugin uses mouseover event subscription to determine which elements are entered and which are left. But we should be testing it by verifying React's onMouseEnter and onMouseLeave events, not onMouseOver itself.

gaearon · 2017-10-26T22:49:45Z

Specifically you can see the first test was added in a2ecee5.

This tells you which section of code it’s supposed to be testing. You can see that removing these changes doesn’t make the test fail, but it should.

You can also check that with the original test file, reverting those changes makes it hang. Which is not ideal (I'd prefer it fail 😛 ) but does demonstrate that we had coverage for this.

gaearon · 2017-10-31T14:14:54Z

Gentle ping, are you interested in following up on this?

accordeiro · 2017-10-31T15:10:30Z

Yes, of course – sorry for not following up on this before. I'll try to make the changes tomorrow at latest, is this ok? :)

accordeiro · 2017-10-31T23:06:12Z

Hi @gaearon – I've just pushed some changes.

I've checked that deleting the code that you referenced here now makes the test fail. I've also tried setting the win var on the current implementation of EnterLeavePlugin.js to null makes the test fail.

I'm still having a hard time testing the onMouseEnter React event. I've tried dispatching both the mouseout and the mouseenter events (from opposite origins, naturally) and none of them have actually called the onMouseEnter function on the node. Is this something I should be worried about?

Using onMouseLeave + dispatching the mouseleave MouseEvent [like on the current diff] worked like a charm, though.

Please let me know if I'm on the right direction.

gaearon · 2017-11-03T18:41:41Z

I'm still having a hard time testing the onMouseEnter React event. I've tried dispatching both the mouseout and the mouseenter events (from opposite origins, naturally) and none of them have actually called the onMouseEnter function on the node. Is this something I should be worried about?

You can see by the source that the only events React actually listens to in order to implement onMouseEnter (or onMouseLeave) is mouseover and mouseout. Does this help?

gaearon · 2017-11-03T19:17:44Z

Thanks! This looks awesome.
I also added coverage for the onMouseEnter case which wasn't being tested by the new code.

* Use only public API for EnterLeaveEventPlugin Tests (facebook#11299) * Trigger native event to test EnterLeaveEventPlugin (facebook#11299) * Rewrite EnterLeaveEventPlugin tests to use dispatchEvent * Update EnterLeaveEventPlugin test to use OnMouseLeave event * Add coverage for onMouseEnter too

Use only public API for EnterLeaveEventPlugin Tests (facebook#11299)

e11fef5

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

gaearon requested changes Oct 22, 2017

View reviewed changes

Trigger native event to test EnterLeaveEventPlugin (facebook#11299)

09f45f2

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

Rewrite EnterLeaveEventPlugin tests to use dispatchEvent

20e8a09

Merge branch 'master' into public-api-tests

e2b2398

gaearon previously approved these changes Oct 26, 2017

View reviewed changes

gaearon reviewed Oct 26, 2017

View reviewed changes

gaearon requested changes Oct 26, 2017

View reviewed changes

accordeiro added 2 commits October 31, 2017 18:49

Merge branch 'master' into public-api-tests

b486a09

Update EnterLeaveEventPlugin test to use OnMouseLeave event

c8b0b9c

gaearon added 2 commits November 3, 2017 18:42

Merge branch 'master' into public-api-tests

b76d575

Add coverage for onMouseEnter too

2aca0d1

gaearon approved these changes Nov 3, 2017

View reviewed changes

gaearon merged commit 43a1e0d into facebook:master Nov 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use only public API for EnterLeaveEventPlugin Tests #11316

Use only public API for EnterLeaveEventPlugin Tests #11316

accordeiro commented Oct 21, 2017

gaearon left a comment

accordeiro commented Oct 22, 2017

gaearon commented Oct 26, 2017

gaearon left a comment

accordeiro commented Oct 26, 2017

gaearon Oct 26, 2017

gaearon Oct 26, 2017

gaearon left a comment

gaearon commented Oct 26, 2017

gaearon commented Oct 31, 2017

accordeiro commented Oct 31, 2017

accordeiro commented Oct 31, 2017

gaearon commented Nov 3, 2017

gaearon commented Nov 3, 2017

Use only public API for EnterLeaveEventPlugin Tests #11316

Use only public API for EnterLeaveEventPlugin Tests #11316

Conversation

accordeiro commented Oct 21, 2017

gaearon left a comment

Choose a reason for hiding this comment

accordeiro commented Oct 22, 2017

gaearon commented Oct 26, 2017

gaearon left a comment

Choose a reason for hiding this comment

accordeiro commented Oct 26, 2017

gaearon Oct 26, 2017

Choose a reason for hiding this comment

gaearon Oct 26, 2017

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

gaearon commented Oct 26, 2017

gaearon commented Oct 31, 2017

accordeiro commented Oct 31, 2017

accordeiro commented Oct 31, 2017

gaearon commented Nov 3, 2017

gaearon commented Nov 3, 2017