Rewrite DOMPropertyOperations-test in terms of public API by gaearon · Pull Request #10281 · facebook/react

gaearon · 2017-07-26T13:30:49Z

This removes a dependency on Stack-only ReactDOMComponentTree.precacheNode so that we can remove it when Stack is gone. For the most part the tests were easy to convert but I’ll point out a few things I wasn’t sure about.

gaearon · 2017-07-26T13:33:04Z

src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js

-      expect(stubNode.hasAttribute('data-foo')).toBe(false);
-    });
-
-    it('should use mutation method where applicable', () => {


I removed this test. It seems like the only mutation method we use is value, and AFAIK there are plenty tests about it specifically. Internally at FB we don’t inject custom properties with mutation methods either.

👍. Long term, I believe we can remove mutation method all together if we circle back to #10150

gaearon · 2017-07-26T13:33:49Z

src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js

-      expect(stubNode.hasAttribute('hidden')).toBe(false);
-    });
-
-    it('should remove property properly even with different name', () => {


I removed this test. I couldn’t find any cases where property name would differ in practice—neither in master nor internally at FB. If we add such case we should add an integration test for that specific property instead.

Yeah, this doesn't happen for any prop that must be assigned as a DOM property. The only thing we alias are a few HTML attribute names and a bunch of SVG attribute names.

gaearon · 2017-07-26T13:35:20Z

src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js

-      expect(stubNode.value).toBe('');
-    });
-
-    it('should not leave all options selected when deleting multiple', () => {


I couldn’t figure out how to write this test in terms of public API. I tried rendering <select multiple> with both value and defaultValue, and then rendering <select> without multiple but couldn’t get this test to pass. If I need to bring it back, can somebody please explain how? Or maybe this is better suited for DOM fixtures.

cc @syranide who wrote this test originally in #1510

Not quite sure what you're asking. The key thing is that multiple individual options will still have selected set to true in the DOM when removing multiple as an attribute in some browsers. So you need to render with multiple and multiple options selected, then remove multiple and verify that only one option is set in the DOM. The logical way would probably be using an uncontrolled select (because that also avoids controlled behavior potentially fixing it).

Hmm. I tried rendering uncontrolled select and then rendering it without multiple, and these assertions failed. I'll try again.

@gaearon Doesn't that mean the current implementation is broken then? (EDIT: if we care about fixing it)

Unfortunately I don't know 🤷‍♀️ We have the old unit test but it's not clear to me whether it is testing the right thing.

The key thing is that multiple individual options will still have selected set to true in the DOM when removing multiple as an attribute in some browsers.

If this behavior is inconsistent, and JSDOM does not exhibit it, do we need to make this a DOM Test Fixture and explore the surface area of browsers that demonstrate it?

gaearon · 2017-07-26T13:37:30Z

src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js

-      DOMPropertyOperations.deleteValueForProperty(stubNode, 'value');
-      // JSDOM does not behave correctly for attributes/properties
-      //expect(stubNode.getAttribute('value')).toBe('foo');
-      expect(stubNode.value).toBe('');


The diff got a bit messed up. The new test is:

var container = document.createElement('div'); spyOn(console, 'error'); ReactDOM.render( <input type="text" value="foo" onChange={function() {}} />, container, ); ReactDOM.render( <input type="text" onChange={function() {}} />, container, ); expect(container.firstChild.getAttribute('value')).toBe('foo'); expect(console.error.calls.count()).toBe(1); expect(console.error.calls.argsFor(0)[0]).toContain( 'A component is changing a controlled input of type text to be uncontrolled', );

But I’m not sure this is right. As you can see, the last line in the old test:

// JSDOM does not behave correctly for attributes/properties //expect(stubNode.getAttribute('value')).toBe('foo'); expect(stubNode.value).toBe('');

was actually not true anymore. However uncommented line works:

expect(container.firstChild.getAttribute('value')).toBe('foo');

I’m not sure what’s going on here, and whether my test reproduces this case, or not.
What is this really testing?

Answering to myself: this test was written in #6228, seemingly to fix #6219. Perhaps I can use fiddle from that issue to recreate the original problem.

The new test seems wrong to me, the second render should be <input ... value="" /> to be truthful to the original test-case (it's about preserving the initial attribute value). But AFAIK this changed way back when you made the attribute always reflect the property for input value? So this should fail.

Why did the test pass then, if it’s wrong? Is it because we were testing internals rather than public API?

In other words, I think my test is not faithful to the original test, but is faithful to the actual behavior in 15.

I think this test might be a victim of changes in how value is assigned over the churn trying to fix bugs in controlled inputs. Your change is a good one, but I wonder it is covered by the round of tests starting at:

https://github.com/facebook/react/blob/master/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js#L873

gaearon · 2017-07-26T14:40:21Z

@syranide I'd appreciate if you could help with the two cases above. I’m trying to figure out how to write correct tests for them that only use ReactDOM.render but haven’t managed so far.

syranide · 2017-08-02T07:34:21Z

Sorry. As far as I understand and remember it all; in some browsers <select multiple> when removing multiple will keep all options still selected despite not allowing multiple. So the question is what we care about. Do we care incorrect options being selected in the DOM for uncontrolled selects? Do we care about incorrect options in DOM being selected for controlled selects? Can this break uncontrolled selects if not normalized (reporting wrong value to onChange)? Can this break controlled selects (showing wrong value, reporting wrong value to onChange)? So one thing is that we need to settle on expected behavior, from last time I was involved in this and related stuff I'm strongly in-favor of treating <select> and <select multiple> as effectively separate elements, so that switching between them means internally recreating them (but I'm not sure if this was cemented or not). In that case then this entire problem goes away and all you need to test for is that selects are recreated when toggling multiple, same goes for toggling controlled (and same for changing input type, but that is separate).

As for the input-test I'm not sure what to do, I know what it wants to test. But AFAIK it should not hold anymore (because you intentionally changed that behavior), BUT IIRC you guys decided to backtrack on the logic for syncing properties and attributes in some way. So yeah, I'm not 100% what to do about it.

gaearon · 2017-08-05T18:38:00Z

The thing I'm struggling with about the <select> test is I can’t seem to get wrong behavior even in old version. For example, even React 0.10 seems to “deselect” multiple options: https://jsfiddle.net/cs3twpz2/.

If it works just due to the browser (as opposed to React being smart) then I don’t understand what is the value in keeping this test (since it effectively tests jsdom).

nhunzaker · 2017-08-05T18:49:20Z

~~@gaearon I noticed a difference in select behavior as well. It might be related. I have a PR out for it in #10142~~

Sorry. I might have misunderstood.

gaearon · 2017-08-05T18:52:32Z

Note I didn’t see a difference—but I probably was testing the wrong thing. All I’m trying to figure out in this thread is whether the test was worth keeping. For now I settled on removing it because I don’t understand how exactly to replicate it with public API. And I’d rather have no test than test that codifies wrong or useless behavior since it could mislead people in the future.

I don't understand what the jsdom comments are about since they pass.

gaearon · 2017-08-05T18:56:52Z

@syranide @aweary @nhunzaker @jquense

Can you please help me out with this? I’d really like to get this test rewrite in. We need to start getting rid of those internal tests that only verify private APIs and don’t test the whole picture.

I tried to faithfully reproduce the current behavior. I had to change the thing <input> test was verifying (since behavior seems to have changed). I removed the <select> test because it wasn’t clear to me what it was testing.

Can we get it in like this? Or please help me understand how to verify current behavior for these tests.

nhunzaker · 2017-08-05T18:59:34Z

src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js

+      var container = document.createElement('div');
+      ReactDOM.render(<div title="Tip!" />, container);
+      expect(container.firstChild.title).toBe('Tip!');
    });


What is the advantage to using ReactDOM.render over TestUtils.renderIntoDocument?

None. Just seems a bit more flexible.

nhunzaker · 2017-08-05T19:03:15Z

src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js

+      ReactDOM.render(<div role="#" />, container);
+      expect(container.firstChild.getAttribute('role')).toBe('#');
+      expect(container.firstChild.role).toBeUndefined();
    });


Just thinking: I didn't see a test that covers attributes that require property assignment. I think it would be covered by a test that uses multiple with a select, but the behavior is probably covered by ReactDOMSelect-test.

gaearon · 2017-08-05T19:03:43Z

To be clear my intention is not to find best solutions for these cases.

It is to ensure the existing behavior is still codified in tests when rewritten via public API. So that we don't accidentally regress.

nhunzaker · 2017-08-05T19:04:19Z

src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js

+      ReactDOM.render(<div disabled={true} />, container);
+      expect(container.firstChild.getAttribute('disabled')).toBe('');
+      ReactDOM.render(<div disabled={false} />, container);
+      expect(container.firstChild.getAttribute('disabled')).toBe(null);


Should this test cover null too? Is that overkill?

nhunzaker · 2017-08-05T19:10:11Z

src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js

+      var container = document.createElement('div');
+      ReactDOM.render(<div title="foo" />, container);
+      expect(container.firstChild.getAttribute('title')).toBe('foo');
+      expect(container.firstChild.title).toBe('foo');


This is existing test behavior, so take it or leave it, but I think line 143 is essentially testing that JSDOM syncs up the title property with the title attribute. Could line 143 go away?

Same deal with line 146.

nhunzaker · 2017-08-05T19:19:04Z

src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js

+      expect(console.error.calls.argsFor(0)[0]).toContain(
+        'A component is changing a controlled input of type text to be uncontrolled',
      );
    });


If this test is giving us trouble via unit testing, I think we should drop it and consider writing a DOM fixture.

Ah, please lump this into the chat above about selects.

nhunzaker

I think this is accurate. I'm curious if the input value is covered by ReactDOMInput-test, and if the select test should be a fixture, but otherwise 👍

nhunzaker · 2017-08-05T19:23:25Z

src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js

+      expect(console.error.calls.count()).toBe(1);
+      expect(console.error.calls.argsFor(0)[0]).toContain(
+        'A component is changing a controlled input of type text to be uncontrolled',
      );


is this providing any additional coverage over the uncontrolled/controlled switch tests in `ReactDOMInput?

gaearon · 2017-08-05T19:38:31Z

To be clear I deleted the select test. Do you think I should bring it back in some form?

nhunzaker · 2017-08-05T19:39:24Z

No. I think deleting it is the right call.

syranide · 2017-08-05T21:04:03Z

The thing I'm struggling with about the <select> test is I can’t seem to get wrong behavior even in old version. For example, even React 0.10 seems to “deselect” multiple options: https://jsfiddle.net/cs3twpz2/.

It's not a React-bug, it's a browser bug. IIRC only older IE, possibly IE9-IE10.

syranide · 2017-08-05T21:05:34Z

EDIT: But I think testing that the defaultValue is correctly set when toggling multiple is a good replacement.

gaearon · 2017-08-05T21:06:49Z

But is it possible to write a test using jsdom and public API to verify React is able to work around this browser bug, even though jsdom does not exhibit it? For example maybe we can set some mocks on some DOM properties to verify React somehow forces them to be right?

gaearon · 2017-08-05T21:07:31Z

Thanks. Can you elaborate on what it should be set to?

syranide · 2017-08-05T21:31:57Z

But is it possible to write a test using jsdom and public API to verify React is able to work around this browser bug, even though jsdom does not exhibit it? For example maybe we can set some mocks on some DOM properties to verify React somehow forces them to be right?

If we can decide on "toggling multiple is equal to recreating the component", then the bug goes away and you also get a consistent logical behavior IMHO. This can be tested.

Thanks. Can you elaborate on what it should be set to?

As above, as the component should be recreated when toggling multipleand the selected options (in the DOM) afterwards should always be equal to defaultValue (should not be necessary to test controlled components) regardless of what was selected before the change. Bug goes away, consistent behavior across all browsers too (browsers are not consistent in which option is selected when going from multiple to single).

I'm not sure if ReactDOM currently has this behavior, but IMHO it makes sense and how it should be. But I think it does...

Makes sense?

gaearon · 2017-08-07T19:12:33Z

I’m going to land this as is for now. I’ll file a task to follow up with the <select multiple> test.

Rewrite DOMPropertyOperations-test in terms of public API

5587271

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

gaearon requested review from aweary, jquense, nhunzaker and sophiebits July 26, 2017 13:31

gaearon commented Jul 26, 2017

View reviewed changes

Add more assertions

005f835

I don't understand what the jsdom comments are about since they pass.

nhunzaker reviewed Aug 5, 2017

View reviewed changes

nhunzaker approved these changes Aug 5, 2017

View reviewed changes

nhunzaker reviewed Aug 5, 2017

View reviewed changes

Address review feedback

8e5c758

gaearon merged commit 4e4653d into facebook:master Aug 7, 2017

gaearon mentioned this pull request Aug 7, 2017

Add tests for <select multiple> => <select> transition #10401

Closed

gaearon mentioned this pull request Oct 20, 2017

Express more tests via public API #11299

Closed

26 tasks

Comments

Conversation

gaearon commented Jul 26, 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!

gaearon Jul 26, 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!

Choose a reason for hiding this comment

Uh oh!

syranide Jul 26, 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!

Choose a reason for hiding this comment

Uh oh!

gaearon Jul 26, 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!

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 Jul 26, 2017

Uh oh!

syranide commented Aug 2, 2017

Uh oh!

gaearon commented Aug 5, 2017

Uh oh!

nhunzaker commented Aug 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Aug 5, 2017

Uh oh!

gaearon commented Aug 5, 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!

gaearon commented Aug 5, 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!

nhunzaker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon Jul 26, 2017 •

edited

Loading

syranide Jul 26, 2017 •

edited

Loading

gaearon Jul 26, 2017 •

edited

Loading

nhunzaker commented Aug 5, 2017 •

edited

Loading

syranide commented Aug 5, 2017 •

edited

Loading

syranide commented Aug 5, 2017 •

edited

Loading