Rewrite setInnerHTML tests to use Public API. #11385

arielsilvestri · 2017-10-27T13:10:34Z

This rewrites the setInnerHTML tests to utilize dangerouslySetInnerHTML to ensure that nodes with and without the innerHTML attribute are properly set!

gaearon · 2017-10-27T14:03:42Z

packages/react-dom/src/client/__tests__/setInnerHTML-test.js

-      setInnerHTML(node, html);
-      expect(node.innerHTML).toBe(html);
+      const container = document.createElement('div');
+      const component = ReactDOM.render(


Let's keep this called node? It's not a component.

Sure-habit taking over from how I do this on the day-to-day. My bad!

gaearon · 2017-10-27T14:04:21Z

packages/react-dom/src/client/__tests__/setInnerHTML-test.js


-      setInnerHTML(nodeProxy, secondHtml);
+      let component = ReactDOM.render(


It's not a component, it's a node. I know old tests are written this way but they're wrong. Let's use correct naming in new tests.

Sure, can do!

gaearon · 2017-10-27T14:04:37Z

packages/react-dom/src/client/__tests__/setInnerHTML-test.js

+        node,
+      );
+      ReactDOM.unmountComponentAtNode(
+        ReactDOM.findDOMNode(component).parentNode,


There's no need to findDOMNode because it is already a node.

Good catch!

arielsilvestri · 2017-10-29T14:06:43Z

@gaearon Updated. Realized the last set of tests didn't need those variable assignments, since it's checking for container calls that we're spying on.

gaearon · 2017-10-29T14:09:11Z

packages/react-dom/src/client/__tests__/setInnerHTML-test.js


-describe('setInnerHTML', () => {


Let's also rename test file?

arielsilvestri · 2017-10-29T15:03:54Z

@gaearon yarn flow is producing a ton of Required module not found failures. This did not occur before my last merge, and rolling my local branch back before that merge reduces the amount of failures to one, in ReactNoop.js. Any chance something was merged recently that could be causing this? Not sure if anything I did could affect this.

gaearon · 2017-10-31T14:29:44Z

yarn flow is producing a ton of Required module not found failures.

Did you run yarn after merging master into your branch?

arielsilvestri · 2017-10-31T17:44:24Z

@gaearon I believe I did, but when I get home I'll take a look and try again. Sorry for the delays.

Merging Master

arielsilvestri · 2017-11-01T04:51:41Z

@gaearon Had to blow out the yarn.lock after the master merge-some things weren't updating properly.

Noticed that another failure was due to prettier not picking up my file using the yarn prettier command like the contribution guide mentions. yarn prettier-all, which is what the console in Circle CI suggested, did pick up my file and fix it up. Should some documentation be updated?

gaearon · 2017-11-03T20:56:49Z

yarn.lock

@@ -10,12 +10,12 @@ JSONStream@^1.0.3:
    through ">=2.2.7 <3"


Please remove this change. It shouldn't be in this PR.

Conflicts: packages/react-dom/src/client/__tests__/setInnerHTML-test.js yarn.lock

arielsilvestri · 2017-11-04T16:10:38Z

@gaearon Updated.

gaearon · 2017-11-04T17:15:10Z

packages/react-dom/src/client/__tests__/setInnerHTML-test.js

-      // Create a mock node that looks like an SVG in IE (without innerHTML)
-      node = document.createElementNS(Namespaces.svg, 'svg');
-
-      nodeProxy = new Proxy(node, {


Removing this causes the test to no longer verify the SVG IE code path. You can test it yourself by commenting out the related parts in setInnerHTML: the test passes despite them being broken.

gaearon · 2017-11-04T17:20:20Z

packages/react-dom/src/client/__tests__/dangerouslySetInnerHTML-test.js

+        <g dangerouslySetInnerHTML={{__html: firstHtml}} />,
+        container,
+      );
+      ReactDOM.unmountComponentAtNode(container);


This is odd. Why do we need this?

Seems to defeat the purpose of the test (which is to verify setInnerHTML itself cleans up old nodes).

I think this is mistakingly testing that unmounting removes a child, and mounting into the same container later adds it.

Instead of testing that inside an already mounted container, operations setting dangerous HTML on SVG node, if it doesn't have innerHTML (as in IE11), will use appendChild / removeChild.

gaearon · 2017-11-04T18:04:11Z

I rewrote the SVG test because it didn't test the right thing. The other ones were good.
Thank you so much!

arielsilvestri · 2017-11-04T20:20:18Z

@gaearon I appreciate that-thank you! I should have asked you about that test, as it was a little confusing to rewrite, before taking that approach. Can you just give me a little info on how I should have been approaching that one? Thanks!

gaearon · 2017-11-04T22:34:51Z

For future readers, here is the function we're testing:

react/packages/react-dom/src/client/setInnerHTML.js

Lines 22 to 40 in 7432013

    
           var setInnerHTML = createMicrosoftUnsafeLocalFunction(function(node, html) { 
        
             // IE does not have innerHTML for SVG nodes, so instead we inject the 
        
             // new markup in a temp node and then move the child nodes across into 
        
             // the target node 
        
             if (node.namespaceURI === Namespaces.svg && !('innerHTML' in node)) { 
        
               reusableSVGContainer = 
        
                 reusableSVGContainer || document.createElement('div'); 
        
               reusableSVGContainer.innerHTML = '<svg>' + html + '</svg>'; 
        
               var svgNode = reusableSVGContainer.firstChild; 
        
               while (node.firstChild) { 
        
                 node.removeChild(node.firstChild); 
        
               } 
        
               while (svgNode.firstChild) { 
        
                 node.appendChild(svgNode.firstChild); 
        
               } 
        
             } else { 
        
               node.innerHTML = html; 
        
             } 
        
           });

Here are the tests before:

react/packages/react-dom/src/client/__tests__/setInnerHTML-test.js

Lines 29 to 66 in 21d0c11

    
           beforeEach(() => { 
        
             // Create a mock node that looks like an SVG in IE (without innerHTML) 
        
             node = document.createElementNS(Namespaces.svg, 'svg'); 
        
             nodeProxy = new Proxy(node, { 
        
               has: (target, prop) => { 
        
                 return prop === 'innerHTML' ? false : prop in target; 
        
               }, 
        
             }); 
        
             spyOn(node, 'appendChild').and.callThrough(); 
        
             spyOn(node, 'removeChild').and.callThrough(); 
        
           }); 
        
           it('sets innerHTML on it', () => { 
        
             var html = '<circle></circle><rect></rect>'; 
        
             setInnerHTML(nodeProxy, html); 
        
             expect(node.appendChild.calls.argsFor(0)[0].outerHTML).toBe( 
        
               '<circle></circle>', 
        
             ); 
        
             expect(node.appendChild.calls.argsFor(1)[0].outerHTML).toBe( 
        
               '<rect></rect>', 
        
             ); 
        
           }); 
        
           it('clears previous children', () => { 
        
             var firstHtml = '<rect></rect>'; 
        
             var secondHtml = '<circle></circle>'; 
        
             setInnerHTML(nodeProxy, firstHtml); 
        
             setInnerHTML(nodeProxy, secondHtml); 
        
             expect(node.removeChild.calls.argsFor(0)[0].outerHTML).toBe( 
        
               '<rect></rect>', 
        
             ); 
        
             expect(node.innerHTML).toBe('<circle></circle>'); 
        
           });

We need to rewrite the in terms of public API.

First of all, let's look at the branch it's testing:

react/packages/react-dom/src/client/setInnerHTML.js

Lines 26 to 36 in 7432013

    
           if (node.namespaceURI === Namespaces.svg && !('innerHTML' in node)) { 
        
             reusableSVGContainer = 
        
               reusableSVGContainer || document.createElement('div'); 
        
             reusableSVGContainer.innerHTML = '<svg>' + html + '</svg>'; 
        
             var svgNode = reusableSVGContainer.firstChild; 
        
             while (node.firstChild) { 
        
               node.removeChild(node.firstChild); 
        
             } 
        
             while (svgNode.firstChild) { 
        
               node.appendChild(svgNode.firstChild); 
        
             }

To verify that your test didn't check the right thing, I just commented out the code in that branch. The test still passed. Therefore, it wasn't being tested. I'm not sure I understand how you came to that test implementation so I can't dive into that. But I can share how I approached it.

The existing test describes the problem here:

react/packages/react-dom/src/client/setInnerHTML.js

Lines 23 to 25 in 7432013

    
           // IE does not have innerHTML for SVG nodes, so instead we inject the 
        
           // new markup in a temp node and then move the child nodes across into 
        
           // the target node

So our job is to:

Simulate a situation where innerHTML doesn’t exist on SVG nodes, making this condition true:

react/packages/react-dom/src/client/setInnerHTML.js

Line 26 in 7432013

if (node.namespaceURI === Namespaces.svg && !('innerHTML' in node)) {

Verify that setInnerHTML can still replace its HTML content without innerHTML.

The original tests mocked appendChild on a DOM element passed to setInnerHTML for this:

react/packages/react-dom/src/client/__tests__/setInnerHTML-test.js

Lines 47 to 49 in 51c101f

    
           expect(node.appendChild.calls.argsFor(0)[0].outerHTML).toBe( 
        
             '<circle></circle>', 
        
           );

But we can't pass an arbitrary element to it anymore. setInnerHTML is called by React for the node that has dangerouslySetInnerHTML on it, and that node itself is created by React. We can't somehow turn it into a proxy.

However, it's not even important that appendChild in particular is getting called. It's important that something happens and the result is equivalent to setting innerHTML. In other words, it's enough to verify that the content has been updated. We don't really care how, as we test the observable behavior, not implementation details.

So how do we force React to not "see" innerHTML on a node? It does 'innerHTML' in node check here:

react/packages/react-dom/src/client/setInnerHTML.js

Line 26 in 51c101f

if (node.namespaceURI === Namespaces.svg && !('innerHTML' in node)) {

so seems like the only way to work around it is to delete innerHTML. I looked it up, and DOM elements inherit innerHTML from Element.prototype. Therefore, we can force this code path to execute by deleting innerHTML from Element.prototype (and thus making the check false). That's what I did:

react/packages/react-dom/src/client/__tests__/dangerouslySetInnerHTML-test.js

Line 41 in 12165eb

delete Element.prototype.innerHTML;

However, this breaks this line of code in our SVG innerHTML "polyfill":

react/packages/react-dom/src/client/setInnerHTML.js

Line 29 in 51c101f

reusableSVGContainer.innerHTML = '<svg>' + html + '</svg>';

That's because IE is only missing support for innerHTML on SVG nodes but it uses innerHTML on a div to work around it. So basically I need div.innerHTML to keep working.

It seemed natural that I could just put the innerHTML descriptor I deleted on Element.prototype, onto HTMLDivElement.prototype:

react/packages/react-dom/src/client/__tests__/dangerouslySetInnerHTML-test.js

Lines 37 to 46 in 12165eb

    
           innerHTMLDescriptor = Object.getOwnPropertyDescriptor( 
        
             Element.prototype, 
        
             'innerHTML', 
        
           ); 
        
           delete Element.prototype.innerHTML; 
        
           Object.defineProperty( 
        
             HTMLDivElement.prototype, 
        
             'innerHTML', 
        
             innerHTMLDescriptor, 
        
           );

This way only divs would have innerHTML, and our polyfill wouldn't break. This is somewhat testing implementation details (now changing div to another HTML tag would break tests) but I don't think it's likely we'd ever want to change that, so this is acceptable.

The above is enough to crudely simulate what IE is doing. It's not exactly right but it's enough to test the behavior we need to test for, and it's unlikely we'd need to get more precise here. (Considering jsdom isn't exactly the same environment anyway, e.g. our version of jsdom doesn't even recognize SVG tags properly.)

Finally, how do we test that the content gets replaced? We just assert that we see the content we expect to see, before and after the change:

react/packages/react-dom/src/client/__tests__/dangerouslySetInnerHTML-test.js

Lines 84 to 85 in 12165eb

    
           const rect = container.firstChild.firstChild; 
        
           expect(rect.tagName).toBe('rect');

react/packages/react-dom/src/client/__tests__/dangerouslySetInnerHTML-test.js

Lines 90 to 91 in 12165eb

    
           const circle = container.firstChild.firstChild; 
        
           expect(circle.tagName).toBe('circle');

This is enough to verify we somehow set the content, which is all we needed.

Finally, it is important to undo any changes to globals (even if the test fails). Since otherwise it makes the test non-deterministic, and can cause other tests to fail:

react/packages/react-dom/src/client/__tests__/dangerouslySetInnerHTML-test.js

Lines 49 to 56 in 12165eb

    
           afterEach(() => { 
        
             delete HTMLDivElement.prototype.innerHTML; 
        
             Object.defineProperty( 
        
               Element.prototype, 
        
               'innerHTML', 
        
               innerHTMLDescriptor, 
        
             ); 
        
           });

I hope this helps!

arielsilvestri · 2017-11-04T22:42:42Z

Very much so. Thanks!

arielsilvestri · 2017-11-04T22:51:15Z

@gaearon In regards to why I took my approach, what I ended up doing was trying to make each test pass without changing too much from the original implementation. From what I looked up and what I was using, I didn't think I had to change too much, and as I was coming up with what I was refactoring, I had situations where I would get failing tests. I never thought to comment out what I already had in my setup, therefore I never noticed that I had a false positive in this instance.

gaearon · 2017-11-04T22:58:26Z

Yeah, that makes sense. These are not often trivial changes though :-) When rewriting unit tests into acceptance/integration tests, sometimes you have to rethink the whole approach and in that case it's easier to think about what situation it was written to test rather than what the original test was trying to do.

arielsilvestri · 2017-11-04T23:08:05Z

@gaearon Totally fair. Thanks for the thoughtful feedback!

My main takeaway here though is that I definitely felt like, at the time, the test read weird to me and I was confused about whether or not I was properly testing a situation involving SVG nodes, and I could have asked a question to get a little help with it.

gaearon · 2017-11-05T00:19:02Z

I agree the existing one was pretty weird 😛 I think I remember some context on it from the past when I've merged some bugfixes to that code. IMO the most helpful thing to understand the test is to git blame it and find the PR that introduced it. That usually explains the motivation and sometimes provides a more realistic failing example (which unfortunately didn't become a unit test at the time).

arielsilvestri · 2017-11-05T03:41:50Z

Good advice. I'll be sure to take a look at previous examples/PRs in the future when I make future contributions :)

* Rewrite setInnerHTML tests to use Public API. * Rename variables and drop unnecessary variable assignments. * Rename testfile to dangerouslySetInnerHTML-test.js * Properly prettify test file. * Rewrite SVG tests to verify we recover from missing innerHTML

Rewrite setInnerHTML tests to use Public API.

29821ba

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

gaearon reviewed Oct 27, 2017

View reviewed changes

Jonathan Silvestri added 2 commits October 29, 2017 10:03

Rename variables and drop unnecessary variable assignments.

69e7c54

Merge branch 'master' into master

53b7b4a

gaearon reviewed Oct 29, 2017

View reviewed changes

Jonathan Silvestri added 3 commits November 1, 2017 00:19

Rename testfile to dangerouslySetInnerHTML-test.js

3d73094

Merge pull request #1 from facebook/master

ef5a516

Merging Master

Properly prettify test file.

ad14f12

gaearon reviewed Nov 3, 2017

View reviewed changes

Merge branch 'master' of https://github.com/facebook/react

1d9641c

Conflicts: packages/react-dom/src/client/__tests__/setInnerHTML-test.js yarn.lock

gaearon reviewed Nov 4, 2017

View reviewed changes

Rewrite SVG tests to verify we recover from missing innerHTML

12165eb

gaearon merged commit 366600d into facebook:master Nov 4, 2017

gaearon mentioned this pull request Nov 4, 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 setInnerHTML tests to use Public API. #11385

Rewrite setInnerHTML tests to use Public API. #11385

arielsilvestri commented Oct 27, 2017

gaearon Oct 27, 2017

arielsilvestri Oct 27, 2017

gaearon Oct 27, 2017

arielsilvestri Oct 27, 2017

gaearon Oct 27, 2017

arielsilvestri Oct 27, 2017

arielsilvestri commented Oct 29, 2017

gaearon Oct 29, 2017

arielsilvestri Oct 29, 2017

arielsilvestri commented Oct 29, 2017 •

edited

Loading

gaearon commented Oct 31, 2017

arielsilvestri commented Oct 31, 2017

arielsilvestri commented Nov 1, 2017

gaearon Nov 3, 2017

arielsilvestri commented Nov 4, 2017

gaearon Nov 4, 2017

gaearon Nov 4, 2017

gaearon Nov 4, 2017

gaearon Nov 4, 2017 •

edited

Loading

gaearon commented Nov 4, 2017

arielsilvestri commented Nov 4, 2017

gaearon commented Nov 4, 2017 •

edited

Loading

arielsilvestri commented Nov 4, 2017

arielsilvestri commented Nov 4, 2017

gaearon commented Nov 4, 2017 •

edited

Loading

arielsilvestri commented Nov 4, 2017

gaearon commented Nov 5, 2017

arielsilvestri commented Nov 5, 2017


		setInnerHTML(nodeProxy, secondHtml);
		let component = ReactDOM.render(


		describe('setInnerHTML', () => {

Rewrite setInnerHTML tests to use Public API. #11385

Rewrite setInnerHTML tests to use Public API. #11385

Conversation

arielsilvestri commented Oct 27, 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

arielsilvestri commented Oct 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielsilvestri commented Oct 29, 2017 • edited Loading

gaearon commented Oct 31, 2017

arielsilvestri commented Oct 31, 2017

arielsilvestri commented Nov 1, 2017

Choose a reason for hiding this comment

arielsilvestri commented Nov 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon Nov 4, 2017 • edited Loading

Choose a reason for hiding this comment

gaearon commented Nov 4, 2017

arielsilvestri commented Nov 4, 2017

gaearon commented Nov 4, 2017 • edited Loading

arielsilvestri commented Nov 4, 2017

arielsilvestri commented Nov 4, 2017

gaearon commented Nov 4, 2017 • edited Loading

arielsilvestri commented Nov 4, 2017

gaearon commented Nov 5, 2017

arielsilvestri commented Nov 5, 2017

arielsilvestri commented Oct 29, 2017 •

edited

Loading

gaearon Nov 4, 2017 •

edited

Loading

gaearon commented Nov 4, 2017 •

edited

Loading

gaearon commented Nov 4, 2017 •

edited

Loading