Rewrite ReactTreeTraversal-test.js using public APIs #11664

timjacobi · 2017-11-26T01:26:12Z

Hej!

This is a super interesting test to look at since the tests cover 3 different areas when you try to rewrite them using only public APIs. So far I have converted 1/3 of the tests to use public APIs only. The rest I'd like to discuss.

`traverseTwoPhase`

These were fairly straightforward to rewrite. I could even used the same expectedCalls arrays for the expectations on the mock.

The only thing that could be a bit nicer is the calls to eventHandler. I would like to factor out passing the arguments but #11663 prevents me from passing the event phase dynamically.

`traverseEnterLeave`

In order to test them in the same fashion as most other public API tests I would have to dispatch onmouseenter and onmouseleave events against the DOM nodes and see which event handlers React calls. Unfortunately dispatching these event types doesn't have an effect and looking at EnterLeaveEventPlugin.js it seems like enter and leave events are implemented using onmouseover and onmouseout.
The tests seem to verify the capturing and bubbling behaviour which are inherently different between onmouseenter and onmouseover as well as onmouseleave and onmouseout. I would be really thank for some ideas on how we could solve this.

`getFirstCommonAncestor` aka `getLowestCommonAncestor`

This function is only used by the ResponderEventPlugin which is in turn only used by React Native. This doesn't mean we can't test it only with public APIs but we might struggle with that in our JSDOM environment. Again would be great to discuss alternatives :)

gaearon · 2017-11-27T18:53:07Z

Unfortunately dispatching these event types doesn't have an effect and looking at EnterLeaveEventPlugin.js it seems like enter and leave events are implemented using onmouseover and onmouseout.

This sounds right.

I would be really thank for some ideas on how we could solve this.

I'm not sure I understand the problem. The traversal tests verify that we correctly walk the tree for different "types" of walks (enter-leave is one example). It seems right to me that you would test it by listening on React onMouseEnter and onMouseLeave, but dispatch whatever native events necessary. Maybe EnterLeavePlugin-test could be useful as inspiration? Or could you clarify what is the problem you ran into?

This function is only used by the ResponderEventPlugin which is in turn only used by React Native.

If you search for require('react-native-renderer') you'll find a few places where we load its mock. Maybe some of these tests could be helpful?

timjacobi · 2017-11-27T22:23:17Z

Or could you clarify what is the problem you ran into?

My thoughts were the wrong way like "to simulate a mouseenter I have to dispatch a mousesenter"... I have figured out the events to dispatch now and rewritten all enter/leave tests.

I will look into the inner workings of the ResponderEventPlugin tomorrow to understand how I can trigger getLowestCommonAncestor with the values in the current test. The test renderer looks good, I'm sure I can get it to where I need it.

When I added the react-native-renderer to the test though I got the error Invariant Violation: EventPluginRegistry: Cannot inject event plugin ordering more than once. You are likely trying to load more than one copy of React. which makes sense. Is there any trick to unload React or do I have to put the test in a different file?

gaearon · 2017-11-27T22:30:36Z

do I have to put the test in a different file?

This sounds right. In fact maybe you can move the original test for that part into ResponderEventPlugin-test.internal.js which uses internal APIs anyway. We can look at it later at some point.

timjacobi · 2017-11-28T07:14:27Z

Sounds good. It looks like ResponderEventPlugin-test.internal.js needs some care anyway.
I have removed the WIP status now. Should be good to review.

gaearon · 2017-11-28T12:58:39Z

packages/events/__tests__/ResponderEventPlugin-test.internal.js

@@ -1360,4 +1360,88 @@ describe('ResponderEventPlugin', () => {
    run(config, three, nativeEvent);
    expect(ResponderEventPlugin._getResponder()).toBe(null);
  });
+
+  it('should determine the first common ancestor correctly', () => {


Since we're not using Responder plugin it's worth adding a comment explaining why the test is here and how it's related to Resonder.

gaearon · 2017-11-28T13:01:41Z

packages/react-dom/src/__tests__/ReactTreeTraversal-test.js

+  describe('Two phase traversal', () => {
+    it('should not traverse when target is outside component boundary', () => {
+      const outerNode = document.createElement('div');
+      document.body.appendChild(outerNode);


The node is appended here to the document. We should remove it to clean up after the test. I usually put the rest of the test in try/finally, and remove it in the finally block.

gaearon · 2017-11-28T13:02:00Z

packages/react-dom/src/__tests__/ReactTreeTraversal-test.js

+      var enterNode = document.createElement('div');
+
+      document.body.appendChild(leaveNode);
+      document.body.appendChild(enterNode);


In fact it may be easier to always add these nodes in beforeEach, and then always remove them in afterEach. Some tests will use them, some won't.

gaearon · 2017-11-28T13:03:29Z

packages/react-dom/src/__tests__/ReactTreeTraversal-test.js

+      var leaveNode = document.createElement('div');
+      var enterNode = document.getElementById('P_P1_C1__DIV');
+
+      document.body.appendChild(leaveNode); // From the window or outside of the React sandbox.


If we change nodes to always be added, tests like this will have to remove one instead.

Could you elaborate? I don't follow.

Never mind, I misread the tests.

timjacobi · 2017-11-28T15:57:07Z

Updated according to comments :)

gaearon · 2017-11-28T16:02:34Z

packages/react-dom/src/__tests__/ReactTreeTraversal-test.js

+  <div
+    id={id + '__DIV'}
+    onClickCapture={e =>
+      eventHandler(e.currentTarget.id, 'captured', e.type, ARG)


The logic behind choosing to put either ARG or ARG2 here is not clear to me.

In the original tests, it was specific to a test. We're traversing enter/leave, and calling "from" path with ARG and "to" path with ARG2.

Here, we instead encoded ARG and ARG2 right into components. I don't think this is right. This seems to couple them with specific test cases below. So it might be hard or impossible to add other completely valid tests which test the opposite enter/leave paths. This didn't use to be a problem.

I'm not sure what the solution is; just pointing out an issue I see.

Yeah I'm not entirely happy with that either. Will think about a more elegant solution.

I wonder if we're good to just remove the ARGs since we're now tracking the event type which we haven't done before. If I get it righttraverseEnterLeave(from, to, fn, argFrom, argTo) will be called with argFrom and argTo set to the events that are dispatched on the target anyway so I feel like extracting the target id and event type will be strict enough.

Yes this sounds right

these were used for testing the internal API to simulate synthetic events passed to traverseEnterLeave. since we're now dealing with actual synthetic events we can remove them.

timjacobi · 2017-11-29T13:17:20Z

@gaearon the App Veyor test failures seem unrelated.

gaearon · 2017-11-29T13:42:16Z

Yeah, I didn't realize it would start appearing on PRs..

gaearon · 2017-11-29T14:04:23Z

This is good. Thank you!

timjacobi · 2017-11-29T15:12:07Z

No worries. Was great again!

* rewrite two phase traversal tests with public APIs * rewrite enter/leave tests * lift render into beforeEach, organise variables * move getLowestCommonAncestor test * remove internal tree traversal test * fix linter errors * move creation of outer nodes into {before,after}Each * explain why getLowestCommonAncestor test was moved * remove unnessecary ARG and ARG2 token these were used for testing the internal API to simulate synthetic events passed to traverseEnterLeave. since we're now dealing with actual synthetic events we can remove them. * run prettier

rewrite two phase traversal tests with public APIs

324641f

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

timjacobi added 2 commits November 27, 2017 21:20

rewrite enter/leave tests

c186e01

lift render into beforeEach, organise variables

e889dc4

timjacobi added 2 commits November 28, 2017 07:07

move getLowestCommonAncestor test

c96f658

remove internal tree traversal test

7c47c07

timjacobi changed the title ~~[WIP] Rewrite ReactTreeTraversal-test.js using public APIs~~ Rewrite ReactTreeTraversal-test.js using public APIs Nov 28, 2017

fix linter errors

185e5e0

gaearon reviewed Nov 28, 2017

View reviewed changes

timjacobi added 2 commits November 28, 2017 13:58

move creation of outer nodes into {before,after}Each

2847074

explain why getLowestCommonAncestor test was moved

5f7bcd2

gaearon reviewed Nov 28, 2017

View reviewed changes

gaearon mentioned this pull request Nov 28, 2017

Express more tests via public API #11299

Closed

26 tasks

timjacobi added 2 commits November 28, 2017 22:27

remove unnessecary ARG and ARG2 token

ee2992e

these were used for testing the internal API to simulate synthetic events passed to traverseEnterLeave. since we're now dealing with actual synthetic events we can remove them.

run prettier

fab27fb

gaearon approved these changes Nov 29, 2017

View reviewed changes

gaearon merged commit c1b2a34 into facebook:master Nov 29, 2017

timjacobi deleted the tree-traversal-public-api branch November 29, 2017 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite ReactTreeTraversal-test.js using public APIs #11664

Rewrite ReactTreeTraversal-test.js using public APIs #11664

timjacobi commented Nov 26, 2017 •

edited

gaearon commented Nov 27, 2017 •

edited

timjacobi commented Nov 27, 2017 •

edited

gaearon commented Nov 27, 2017

timjacobi commented Nov 28, 2017

gaearon Nov 28, 2017

gaearon Nov 28, 2017

gaearon Nov 28, 2017

gaearon Nov 28, 2017

gaearon Nov 28, 2017

timjacobi Nov 28, 2017

gaearon Nov 28, 2017

timjacobi commented Nov 28, 2017

gaearon Nov 28, 2017

timjacobi Nov 28, 2017

timjacobi Nov 28, 2017

gaearon Nov 28, 2017

timjacobi commented Nov 29, 2017

gaearon commented Nov 29, 2017

gaearon commented Nov 29, 2017

timjacobi commented Nov 29, 2017

Rewrite ReactTreeTraversal-test.js using public APIs #11664

Rewrite ReactTreeTraversal-test.js using public APIs #11664

Conversation

timjacobi commented Nov 26, 2017 • edited

traverseTwoPhase

traverseEnterLeave

getFirstCommonAncestor aka getLowestCommonAncestor

gaearon commented Nov 27, 2017 • edited

timjacobi commented Nov 27, 2017 • edited

gaearon commented Nov 27, 2017

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

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

timjacobi commented Nov 29, 2017

gaearon commented Nov 29, 2017

gaearon commented Nov 29, 2017

timjacobi commented Nov 29, 2017

timjacobi commented Nov 26, 2017 •

edited

`traverseTwoPhase`

`traverseEnterLeave`

`getFirstCommonAncestor` aka `getLowestCommonAncestor`

gaearon commented Nov 27, 2017 •

edited

timjacobi commented Nov 27, 2017 •

edited