Remove tests in ReactDOMComponent-test depending on internal API #11337

AudyOdi · 2017-10-23T09:55:27Z

Removed inputValueTracking dependency in ReactDOMComponent-test based on list in #11299. I took this approach based on #11309 review. Please let me know if this is not the way we should test it.

gaearon

Doesn't this just create another tracker on top? I'm not sure this actually tells us that the existing tracker works.

I think there might not be a super clear way to implement this. You could just read the tracker from node. _valueTracker instead (and assert it exists). This does test an implementation detail, but at least it’ll be obvious when it breaks. I’m open to better suggestions though.

zhaozhiming · 2017-10-24T04:58:42Z

Hi @gaearon , could just use the container to verify the value? Thanks.

it('should track input values', () => {
      var container = document.createElement('div');
      var inst = ReactDOM.render(
        <input type="text" defaultValue="foo" />,
        container,
      );
-
-     var tracker = inputValueTracking._getTrackerFromNode(inst);
-
-     expect(tracker.getValue()).toEqual('foo');
+     expect(container.firstChild.value).toEqual('foo');
    });

AudyOdi · 2017-10-24T05:45:05Z

@gaearon Thank you for reviewing. Actually I did it the way you mentioned, using node._valueTracker, but then I realized that _valueTracker is also a private API because it begins with _. But if it turns out it's fine to use it, I will do it. I also had the same question with @zhaozhiming because in this file we should test the value of the component that honors defaultValue (sets value accordingly), not the tracker right?

gaearon · 2017-10-24T14:20:28Z

I haven’t dived deeply into what it’s supposed to test. (I’m not very familiar with that part of code.)

But it would be nice if you could try to “break” it on master by doing bad changes to inputValueTracking, and then see if you can still “break” it in your branch. In other words, we want to be sure the test actually covers the code.

AudyOdi · 2017-10-26T04:25:31Z

@gaearon I took your advice to use node._valueTracker, so the getTracker function will just return the existing one instead of creating new tracker. I also added some assertions to make sure the tracker exists and tracks the value. I also assert the value doesn't change if the defaultValue is changed afterwards.

…e being tracked

zhaozhiming · 2017-10-27T06:25:20Z

@AudyOdi Maybe you can take a look at this PR(#11309). The changes have a bit similar.

gaearon · 2017-10-27T15:21:20Z

Yes. I think it would be better to apply the same strategy as in #11309, if possible.

AudyOdi · 2017-10-27T15:54:04Z

based on #11309, should I change the getTracker function to use Object.getOwnPropertyDescriptor too? So it will look like below (I can adjust the name later).

var getTracker = (node) => {
    return Object.getOwnPropertyDescriptor(
        node.constructor.prototype,
        'value',
    ).get;
};

and then in the test case, I still can get the value of the input by using call.

var tracker = getTracker(inst);
tracker.call(node);  // should return the value

this is possible because Object.getOwnPropertyDescriptor has both get and set function.
what do you think about this way? @zhaozhiming @gaearon

zhaozhiming · 2017-10-30T08:11:16Z

@AudyOdi Yes, I think so.

zhaozhiming · 2017-10-30T08:14:27Z

Maybe we can invite @SadPandaBear to look at these changes are right?
Hi @SadPandaBear Could you help us to review this PR? Thanks.

   var ReactTestUtils;
   var ReactDOM;
   var ReactDOMServer;
-  var inputValueTracking;

   function normalizeCodeLocInfo(str) {
     return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
@@ -26,8 +25,6 @@ describe('ReactDOMComponent', () => {
     ReactDOM = require('react-dom');
     ReactDOMServer = require('react-dom/server');
     ReactTestUtils = require('react-dom/test-utils');
-    // TODO: can we express this test with only public API?
-    inputValueTracking = require('../client/inputValueTracking');
   });

   describe('updateDOM', () => {
@@ -833,6 +830,11 @@ describe('ReactDOMComponent', () => {
   describe('mountComponent', () => {
     var mountComponent;

+    var getTrackedValue = Object.getOwnPropertyDescriptor(
+      HTMLInputElement.prototype,
+      'value',
+    ).get;
+
     beforeEach(() => {
       mountComponent = function(props) {
         var container = document.createElement('div');
@@ -1151,19 +1153,13 @@ describe('ReactDOMComponent', () => {
         <input type="text" defaultValue="foo" />,
         container,
       );
-
-      var tracker = inputValueTracking._getTrackerFromNode(inst);
-
-      expect(tracker.getValue()).toEqual('foo');
+      expect(getTrackedValue.call(inst)).toEqual('foo');
     });

     it('should track textarea values', () => {
       var container = document.createElement('div');
       var inst = ReactDOM.render(<textarea defaultValue="foo" />, container);
-
-      var tracker = inputValueTracking._getTrackerFromNode(inst);
-
-      expect(tracker.getValue()).toEqual('foo');
+      expect(getTrackedValue.call(inst)).toEqual('foo');
     });

     it('should throw for children on void elements', () => {

AudyOdi · 2017-10-30T14:59:02Z

@gaearon @zhaozhiming I have submitted new commit that use Object.getOwnPropertyDescriptor for tracking input value and tried to take similar approach with #11309, it's ready to be review. I'm totally open for any feedback.

SadPandaBear · 2017-10-30T15:57:57Z

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

+        node.constructor.prototype,
+        'value',
+      ).get;
+    };


Nice!

Really liked what you did here for the html interfaces. 👍

SadPandaBear · 2017-10-30T15:58:00Z

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

@@ -1151,19 +1154,37 @@ describe('ReactDOMComponent', () => {
        <input type="text" defaultValue="foo" />,
        container,
      );
+      var getTrackedValue = getValueTracker(inst);


I'm not sure if you do have to create an instance for the trackers. See, the solution I made for ReactDOMInput-test actually only had the calls for the input values i wanted to test.

Anyway, I can't see any trouble really about this solution. 😄

I see you are setting the value directly into inst. What happens if you set with the tracker instance you created (like this)? Will the test still pass (considering you write the setUntrackedValue and everything)?

@SadPandaBear Thank you for reviewing. I have confirmed that changing value using tracker instance still pass the test. The reason I create an instance for the tracker is because getValueTracker is being used for both input and textarea element. Not sure about using HTMLInputElement for textarea is the correct way to do it.

Not sure about using HTMLInputElement for textarea is the correct way to do it.

Can you declare two separate helpers (for input and textarea) instead?

@gaearon got it, I will define the helper in each test case instead so that input will use HTMLInputElement, and textarea will use HTMLTextAreaElement

gaearon

Let's use the prototype getter to make sure we're reading the right thing. Since we test both input and textarea, let's read each and keep them in separate variables.

…otype

AudyOdi · 2017-10-31T15:44:13Z

@gaearon I push new commit that move getValueTracker to each test case and use the corresponding prototype to get the tracker. It's ready to be review.

gaearon · 2017-11-03T20:50:17Z

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

+      expect(getValueTracker.call(inst)).toEqual('foo');
+
+      inst.defaultValue = 'new foo';
+      expect(getValueTracker.call(inst)).not.toEqual('new foo');


This check is redundant. You already check for the other value later.

gaearon · 2017-11-03T20:50:25Z

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

@@ -1152,18 +1149,46 @@ describe('ReactDOMComponent', () => {
        container,
      );

-      var tracker = inputValueTracking._getTrackerFromNode(inst);
+      var getValueTracker = Object.getOwnPropertyDescriptor(


This should ideally be done before React is imported.

gaearon · 2017-11-03T20:55:38Z

Leaving this to @jquense to review because I don't understand what the original test is supposed to test.

jquense · 2017-11-03T22:15:15Z

So the original test is supposed to confirm that the input tracking stuff is set up from initial values when you render an input for the first time. Which is to say that the tracking logic ensures change events don't fire twice for the same value.

I'm trying to think of a public side effect that would indicate this isn't working...I think the test case might be that if you initialize an input with value 'foo' then explicitly set the value to 'foo' it should be a noop, e.g. onChange doesn't fire. You'd have to use the same magic that the ChangeEventPlugin tests use tho to set an untracked value. Frankly tho this case is probably covered but the ChangeEventPlugin tests

jquense · 2017-11-03T23:23:44Z

If you wanted to keep it simple, I'd just test that the DOM node has the tracker attached to it and has the same value as the jsx value. Not quite public api only but I'm not really sure anything else is gonna succinctly test the right thing

gaearon · 2017-11-03T23:28:37Z

Why is it important to test that the tracker is attached? Does something break if it's not? :-)

jquense · 2017-11-04T00:20:50Z

You'd en up defeating the change dedupe logic at least... Tbh I don't remember if there is a more severe consequence. I do think this set of tests doesn't really belong here. They make sense when testing the specific implementation of the tracker getting attached here, but ultimately this is change event specific code

gaearon · 2017-11-04T00:28:43Z

Is there any way we can salvage these tests to offer the same coverage they currently offer without actually relying on implementation details? Or can we just delete them?

AudyOdi · 2017-11-06T02:51:37Z

Thanks @jquense and @gaearon. I appreciate the insight. The original test was only testing if the tracker is attached, I added some other tests because I didn't know that it's been covered in ChangeEventPlugin test like you mentioned. I'll wait for you to determine if this should be moved, removed or replace.

gaearon

Let's just remove these test.
I'll add a follow up to #11333 instead.

…ported

AudyOdi · 2017-11-09T12:43:58Z

@gaearon got it. I just pushed new commit that remove the tests and change it back to original test, also move the value tracker definition, for both input and textarea, before React is imported. It's ready to review.

gaearon · 2017-11-10T12:00:38Z

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


  function normalizeCodeLocInfo(str) {
    return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
  }

  beforeEach(() => {
    jest.resetModules();
+
+    getInputValueTracker = Object.getOwnPropertyDescriptor(


I don't understand what this does in the context of this test.

As @jquense explained above, the original purpose of the test is to verify our custom tracker object "sees" the original value.

However this notion of "tracker" is not the same thing that you get from the HTMLInputElement's prototype. I think you got confused by naming used in some other test.

The thing you read from HTMLInputElement prototype is just the jsdom's value accessor itself. So there is no need to test it at all. You're just testing that jsdom works. (But we already know it works as it has its own test suite :-)

gaearon · 2017-11-10T12:02:37Z

I just removed these tests, as they aren't very useful, and we'll cover the relevant scenarios as part of ChangeEventPlugin-test instead. Thanks for your help on this, and sorry the back and forth might have been frustrating.

AudyOdi · 2017-11-10T15:10:07Z

@gaearon No problem! Yes I agree that the tests are not that useful, and a lot of confusion with Object.getOwnPropertyDescriptor itself. Thank you for reviewing this diff and giving a lot of feedbacks. I would love to contribute again in the future. Please let me know if there is a chance to contribute (posting in your twitter really does help!) 😄
also thanks to @zhaozhiming @SadPandaBear @jquense for the help and insight.

…ebook#11337) * Remove inputValueTracking from ReactDOMComponent-test dependency * prettier * use node._valueTracker and add some test cases to make sure that value being tracked * using Object.getOwnPropertyDescriptor to get the tracked value * move getValueTracker to each test case and use its corresponding prototype * remove tests and move the value tracker definition before React is imported * Delete these tests completely

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

gaearon requested changes Oct 23, 2017

View reviewed changes

AudyOdi force-pushed the remove-inputvaluetrackingdep-reactdomcomp branch from 06be042 to 7117f52 Compare October 26, 2017 03:18

AudyOdi added 3 commits October 26, 2017 21:48

Remove inputValueTracking from ReactDOMComponent-test dependency

ae96f21

prettier

cb7b802

use node._valueTracker and add some test cases to make sure that valu…

3e9bfe3

…e being tracked

AudyOdi force-pushed the remove-inputvaluetrackingdep-reactdomcomp branch from 7117f52 to 3e9bfe3 Compare October 26, 2017 14:52

using Object.getOwnPropertyDescriptor to get the tracked value

d480ce7

SadPandaBear reviewed Oct 30, 2017

View reviewed changes

gaearon requested changes Oct 31, 2017

View reviewed changes

move getValueTracker to each test case and use its corresponding prot…

4614636

…otype

gaearon reviewed Nov 3, 2017

View reviewed changes

gaearon requested a review from jquense November 3, 2017 20:55

gaearon requested changes Nov 8, 2017

View reviewed changes

gaearon mentioned this pull request Nov 8, 2017

Use only public API for ChangeEventPlugin-test.js #11333

Merged

remove tests and move the value tracker definition before React is im…

aa75dde

…ported

gaearon reviewed Nov 10, 2017

View reviewed changes

Delete these tests completely

2dadb8d

gaearon changed the title ~~Rewrite ReactDOMComponent-test depending on internal API~~ Remove tests in ReactDOMComponent-test depending on internal API Nov 10, 2017

gaearon merged commit ae7639c into facebook:master Nov 10, 2017

gaearon mentioned this pull request Nov 20, 2017

Express more tests via public API #11299

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove tests in ReactDOMComponent-test depending on internal API #11337

Remove tests in ReactDOMComponent-test depending on internal API #11337

AudyOdi commented Oct 23, 2017

gaearon left a comment

zhaozhiming commented Oct 24, 2017

AudyOdi commented Oct 24, 2017

gaearon commented Oct 24, 2017

AudyOdi commented Oct 26, 2017

zhaozhiming commented Oct 27, 2017

gaearon commented Oct 27, 2017

AudyOdi commented Oct 27, 2017 •

edited

Loading

zhaozhiming commented Oct 30, 2017

zhaozhiming commented Oct 30, 2017 •

edited

Loading

AudyOdi commented Oct 30, 2017

SadPandaBear Oct 30, 2017 •

edited

Loading

SadPandaBear Oct 30, 2017 •

edited

Loading

AudyOdi Oct 31, 2017

gaearon Oct 31, 2017

AudyOdi Oct 31, 2017

gaearon left a comment

AudyOdi commented Oct 31, 2017

gaearon Nov 3, 2017

gaearon Nov 3, 2017

gaearon commented Nov 3, 2017

jquense commented Nov 3, 2017 •

edited

Loading

jquense commented Nov 3, 2017

gaearon commented Nov 3, 2017

jquense commented Nov 4, 2017

gaearon commented Nov 4, 2017

AudyOdi commented Nov 6, 2017

gaearon left a comment

AudyOdi commented Nov 9, 2017

gaearon Nov 10, 2017

gaearon commented Nov 10, 2017

AudyOdi commented Nov 10, 2017

Remove tests in ReactDOMComponent-test depending on internal API #11337

Remove tests in ReactDOMComponent-test depending on internal API #11337

Conversation

AudyOdi commented Oct 23, 2017

gaearon left a comment

Choose a reason for hiding this comment

zhaozhiming commented Oct 24, 2017

AudyOdi commented Oct 24, 2017

gaearon commented Oct 24, 2017

AudyOdi commented Oct 26, 2017

zhaozhiming commented Oct 27, 2017

gaearon commented Oct 27, 2017

AudyOdi commented Oct 27, 2017 • edited Loading

zhaozhiming commented Oct 30, 2017

zhaozhiming commented Oct 30, 2017 • edited Loading

AudyOdi commented Oct 30, 2017

SadPandaBear Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

SadPandaBear Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

AudyOdi Oct 31, 2017

Choose a reason for hiding this comment

gaearon Oct 31, 2017

Choose a reason for hiding this comment

AudyOdi Oct 31, 2017

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

AudyOdi commented Oct 31, 2017

gaearon Nov 3, 2017

Choose a reason for hiding this comment

gaearon Nov 3, 2017

Choose a reason for hiding this comment

gaearon commented Nov 3, 2017

jquense commented Nov 3, 2017 • edited Loading

jquense commented Nov 3, 2017

gaearon commented Nov 3, 2017

jquense commented Nov 4, 2017

gaearon commented Nov 4, 2017

AudyOdi commented Nov 6, 2017

gaearon left a comment

Choose a reason for hiding this comment

AudyOdi commented Nov 9, 2017

gaearon Nov 10, 2017

Choose a reason for hiding this comment

gaearon commented Nov 10, 2017

AudyOdi commented Nov 10, 2017

AudyOdi commented Oct 27, 2017 •

edited

Loading

zhaozhiming commented Oct 30, 2017 •

edited

Loading

SadPandaBear Oct 30, 2017 •

edited

Loading

SadPandaBear Oct 30, 2017 •

edited

Loading

jquense commented Nov 3, 2017 •

edited

Loading