Rewrite ReactDOMComponentTree-test to test behavior using Public API #11383

GordyD · 2017-10-27T04:54:10Z

This is part of #11299

I've tried to identify cases where code within ReactDOMComponentTree is exercised via the public API and have updated accordingly but I'm not entirely sure whether I'm on the right track. I thought I'd PR to get feedback from the community. Looking forward to comments.

gaearon · 2017-10-27T14:10:24Z

I think ideally instead of reading private fields and methods, we should try to test the behavior those fields and methods are used for. This would require some investigation, e.g. where is getClosestInstanceFromNode or __reactEventHandlers fields used. Then come up with a test that looks like application code that would trigger the corresponding behavior and verify its correctness.

Does this make sense?

GordyD · 2017-10-27T19:41:36Z

@gaearon Makes sense. I'll investigate further this weekend. In the mean time, could you provide feedback on the test cases I added to give me an idea of whether I'm on the right track or not.

gaearon · 2017-10-27T23:42:30Z

That was my feedback: while we could do it this way, it's still testing internals more than the actual behavior.

GordyD · 2017-10-28T05:10:35Z

Would this be a plausible test for checking behavior that getInstanceFromNode is used for in renderSubtreeIntoContainer? (albeit only in DEV mode)

  it('finds instance from node to stop rendering over other react rendered components', () => {
    spyOn(console, 'error');
    const component = <div><span>Hello</span></div>;
    const anotherComponent = <div></div>;
    const container = document.createElement('div');
    const instance = ReactDOM.render(component, container);
    ReactDOM.render(anotherComponent, instance);
    expectDev(console.error.calls.count()).toBe(1);
    expectDev(console.error.calls.argsFor(0)[0]).toContain(
      'render(...): Replacing React-rendered children with a new root ' +
      'component. If you intended to update the children of this node, ' +
      'you should instead have the existing children update their state ' +
      'and render the new components instead of calling ReactDOM.render.'
    );
  });

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

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

+      a = null;
+      _onChange = e => {
+        const node = e.currentTarget;
+        expect(node.value).toEqual(finishValue);


The problem with tests like this is that if _onChange never fires, the test passes (since none of the expectations were called). We need to move those out.

Thanks for the comment @gaearon . I'll update to move assertions out of the event handler - this is much simpler.

Though FWIW from my understanding of async tests in Jest if done() is never called, due to _onChange not firing, then it would fail - albeit the failure message would not be the most helpful.

GordyD · 2017-11-02T22:13:00Z

@gaearon I've updated the diff, can I get another review?

gaearon · 2017-11-10T16:55:56Z

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

+    const inputEvent = new Event('input', {
+      bubbles: true,
+    });
+    setUntrackedInputValue.call(elem, value);


Can you remind me why this is necessary?

In order to simulate the behavior of trying to switch a component from being uncontrolled to controlled we need to assign a new value to the input without it being tracked - I found this approach after reading through the comments of #11309.

gaearon · 2017-11-10T16:57:13Z

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

    ReactDOMServer = require('react-dom/server');
+    document.innerHTML = '';


How do you feel about following this approach instead? Feels a tiny but more obvious what's going on and why IMO. d7745d6

I like it. Tests are more focussed on testing and we move set up to where it should be - thanks for the pointer. I'll adopt this approach.

outdated

gaearon · 2017-11-10T16:59:44Z

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

-    class Component extends React.Component {
+  it('finds closest instance for node when an event happens', () => {
+    const nonReactElemID = 'aID';
+    const innerHTML = {__html: `<div id="${nonReactElemID}"></div>`};


GordyD · 2017-11-13T05:32:08Z

I've updated the tests to adopt the set up and tear down approach demonstrated in d7745d6.

After making changes I watched what happened as I commented out different branches of code in ReactDOMComponentTree to manually verify that the updated tests exercise and provide coverage for the module.

Notable Observations:

commenting out lines 26-28 of getClosestInstanceFromNode does not cause any failing tests, but that is expected due to the logic on line 32 - I validated this code path is exercised by the test though by logging here
commenting out line 47 in getClosestInstanceFromNode does not result in failing tests, but again because of the logic following this is to be expected - I validated this code path is exercised by the test by logging here
commenting out lines 49-51 in getClosestInstanceFromNode does not result in failing tests -tests do not currently exercise this code path
commenting out lines lines 66 or 69 in getInstanceFromNode do not result in any failing tests

All other blocks commented out resulted in failing tests.

I'm not too sure at this point how to add/update the tests to provide coverage for lines 49-51, 66 and 69 within ReactDOMComponentTree - I'll continue to give it some thought this week.

gaearon · 2017-11-13T11:46:14Z

It would be nice to try to cover these too. On the other hand, if the original tests didn't cover those either (can you check?) maybe it's okay to merge as is :-)

gaearon · 2017-11-13T16:49:41Z

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

+    expect(currentTargetID).toBe(closestInstanceID);
+  });
+
+  it('finds a controlled instance from node and gets its current fiber props', () => {


If you want to test getFiberCurrentPropsFromNode, I think there are more assertions you might need to make. For example I can currently replace its implementation with

return node[internalInstanceKey].memoizedProps;

and the test still passes. However, this is a wrong implementation (node[internalInstanceKey] can potentially point to an "alternate" fiber that is not "current". Current and alternate swap places on updates.)

Can we make this test more solid by tracing what happens with "current" props? e.g. maybe they're used in some way where alternate props would've been wrong. You can git blame when getFiberCurrentPropsFromNode was added to maybe find the cases that were broken before it.

Thanks for the detail, I'll check out the blame and track down ways to make the test more robust.

GordyD · 2017-11-13T17:37:45Z

Checking out master and then commenting out those lines (49-51, 66, 69) do not result in failures - so it doesn't look like these lines are covered in the original test.

Another thing of note I just realized - commenting out body of getNodeFromInstance (line 80) does not result in the test that currently exists for this function! I'm going to look into re-writing the test for this so that we do exercise it.

gaearon · 2017-11-13T18:07:37Z

I appreciate the effort by the way. This is one of the harder ones. Hope you don't feel discouraged that it's not so fast.

GordyD · 2017-11-13T18:25:43Z

Not at all, it's interesting to dig into - I'm learning a lot.

P.S I might be a little slower to update this week as I'm on call, will update latest by the weekend.

gaearon · 2017-11-13T19:09:05Z

I'll be on vacation this week so sounds good :-)

- Part of facebook#11299 - I've tried to identify cases where code within ReactDOMComponentTree is exercised and have updated accordingly but I'm not entirely sure whether I'm on the right track. I thought I'd PR to get feedback from the community. Looking forward to comments.

…xhibited after use of getInstanceFromNode

…upon methods in ReactDOMComponentTree

- Use beforeEach and afterEach to set up and tear down container element for use in each test - Move any functions specific to one test to within test body (improves readability imo)

…rrentPropsFromNode - After researching usage of getNodeFromInstance we can test getNodeFromInstance dispatching some events and asserting the id of the currentTarget - After checking git blame for getFiberCurrentPropsFromNode and reading through facebook#8607 I found a test that we can simplify to assert behavior of the function by ensuring event handler props are updated from the fiber props. Swapping out the implementation of this function with `return node[internalInstanceKey].memoizedProps` results in a failure.

GordyD · 2017-11-19T07:58:26Z

I've replaced the original test for getNodeFromInstance to exercise the function. I've also added a new test to test the implementation of getFiberCurrentPropsFromNode after following your pointer @gaearon and checking out #8607. I found a test case that we could simplify to test behavior exhibited reliant upon its implementation. Swapping it out with node[internalInstanceKey].memoizedProps causes this case to fail.

gaearon · 2017-11-19T14:08:36Z

You are awesome 👏

…acebook#11383) * Rewrite ReactDOMComponentTree-test to test behavior using Public API - Part of facebook#11299 - I've tried to identify cases where code within ReactDOMComponentTree is exercised and have updated accordingly but I'm not entirely sure whether I'm on the right track. I thought I'd PR to get feedback from the community. Looking forward to comments. * Prettier and lint changes * Remove testing of internals and add test cases for testing behavior exhibited after use of getInstanceFromNode * [RFC] Update testing approach to verify exhibited behavior dependent upon methods in ReactDOMComponentTree * Remove tests from event handlers and use sync tests * Prettier changes * Rename variables to be more semantic * Prettier updates * Update test following review - Use beforeEach and afterEach to set up and tear down container element for use in each test - Move any functions specific to one test to within test body (improves readability imo) * Add coverage for getNodeFromInstance and implementation of getFiberCurrentPropsFromNode - After researching usage of getNodeFromInstance we can test getNodeFromInstance dispatching some events and asserting the id of the currentTarget - After checking git blame for getFiberCurrentPropsFromNode and reading through facebook#8607 I found a test that we can simplify to assert behavior of the function by ensuring event handler props are updated from the fiber props. Swapping out the implementation of this function with `return node[internalInstanceKey].memoizedProps` results in a failure.

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

GordyD force-pushed the ReactDOMComponentTree-test branch 2 times, most recently from 257c334 to e5a8f23 Compare October 28, 2017 19:07

gaearon previously requested changes Oct 31, 2017

View reviewed changes

GordyD force-pushed the ReactDOMComponentTree-test branch from e5a8f23 to d58071f Compare November 1, 2017 05:56

DaJoker78 approved these changes Nov 1, 2017

View reviewed changes

GordyD force-pushed the ReactDOMComponentTree-test branch 2 times, most recently from 2433ef3 to f5e404c Compare November 9, 2017 00:38

gaearon reviewed Nov 10, 2017

View reviewed changes

GordyD force-pushed the ReactDOMComponentTree-test branch from 08c7107 to d0d2c4b Compare November 13, 2017 04:21

gaearon reviewed Nov 13, 2017

View reviewed changes

GordyD added 6 commits November 18, 2017 23:49

Prettier and lint changes

cd0e370

Remove testing of internals and add test cases for testing behavior e…

33476ff

…xhibited after use of getInstanceFromNode

[RFC] Update testing approach to verify exhibited behavior dependent …

3dc9361

…upon methods in ReactDOMComponentTree

Remove tests from event handlers and use sync tests

293d6e9

Prettier changes

6f31b61

GordyD added 4 commits November 18, 2017 23:49

Rename variables to be more semantic

e773fa4

Prettier updates

c98f56b

Update test following review

e233d0e

- Use beforeEach and afterEach to set up and tear down container element for use in each test - Move any functions specific to one test to within test body (improves readability imo)

GordyD force-pushed the ReactDOMComponentTree-test branch from d0d2c4b to 84aa535 Compare November 19, 2017 07:51

gaearon merged commit aa0b741 into facebook:master Nov 19, 2017

gaearon mentioned this pull request Nov 19, 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 ReactDOMComponentTree-test to test behavior using Public API #11383

Rewrite ReactDOMComponentTree-test to test behavior using Public API #11383

GordyD commented Oct 27, 2017

gaearon commented Oct 27, 2017

GordyD commented Oct 27, 2017

gaearon commented Oct 27, 2017

GordyD commented Oct 28, 2017 •

edited

Loading

gaearon Oct 31, 2017

GordyD Nov 1, 2017

GordyD commented Nov 2, 2017

gaearon Nov 10, 2017

GordyD Nov 13, 2017

gaearon Nov 10, 2017

GordyD Nov 13, 2017

gaearon Nov 10, 2017

GordyD commented Nov 13, 2017 •

edited

Loading

gaearon commented Nov 13, 2017

gaearon Nov 13, 2017

GordyD Nov 13, 2017

GordyD commented Nov 13, 2017 •

edited

Loading

gaearon commented Nov 13, 2017

GordyD commented Nov 13, 2017

gaearon commented Nov 13, 2017

GordyD commented Nov 19, 2017

gaearon commented Nov 19, 2017

		ReactDOMServer = require('react-dom/server');
		document.innerHTML = '';

Rewrite ReactDOMComponentTree-test to test behavior using Public API #11383

Rewrite ReactDOMComponentTree-test to test behavior using Public API #11383

Conversation

GordyD commented Oct 27, 2017

gaearon commented Oct 27, 2017

GordyD commented Oct 27, 2017

gaearon commented Oct 27, 2017

GordyD commented Oct 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GordyD commented Nov 2, 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

GordyD commented Nov 13, 2017 • edited Loading

gaearon commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GordyD commented Nov 13, 2017 • edited Loading

gaearon commented Nov 13, 2017

GordyD commented Nov 13, 2017

gaearon commented Nov 13, 2017

GordyD commented Nov 19, 2017

gaearon commented Nov 19, 2017

GordyD commented Oct 28, 2017 •

edited

Loading

GordyD commented Nov 13, 2017 •

edited

Loading

GordyD commented Nov 13, 2017 •

edited

Loading