Rewrite ReactDOMInput-test depending on internal API (#11299) #11309

SadPandaBear · 2017-10-20T23:35:11Z

Should help on #11299.

I hope these changes may help with the public APIs on the other tests (especially inputValueTracking-test).

I'm pretty sure it needs more refactoring and I would appreciate if you could help me pointing the most whopping ones.

As @sebmarkbage stated, ReactTestUtils should be considered a non-public API, since it reaches into internals in a way.

sebmarkbage

We're trying to test what happens when the browser changes its internal value of a field. Which it can do without invoking the setter.

What you could do is extract the native setter before we can mutate it. E.g. in beforeEach before the requires.

setUntrackedValue = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'value').set;

Then later use:

setUntrackedValue.call(node, 'giraffe');

Note the use of .call.

sebmarkbage · 2017-10-21T02:39:24Z

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

-    var nativeEvent = document.createEvent('Event');
-    nativeEvent.initEvent('change', true, true);
-    node.dispatchEvent(nativeEvent);
+    ReactTestUtils.SimulateNative.change(node, {target: {value: 'giraffe'}});


How do we know that this actually does anything at all? I'm not sure this actually tests the behavior we're trying to test. Since this is a no-change test.

Correct me if I'm wrong, but ReactTestUtils.SimulateNative.change actually needs to have an element listening to an event to be applied. Any change dispatched to this element won't have any consequences (e.g the value will still be the same). So, judging that in this case, where there's no event at all, makes more sense this way.

You can check a similar case here.

@sebmarkbage Thank you for the hint with setUntrackedValue.call(node, 'giraffe');!
It's way more flexible too! Working on right now!

SadPandaBear · 2017-10-21T13:33:57Z

Doubt: Will the changes with setUntrackedValue.call be applied for all the node.value settings tests or just the ones using native event dispatchers?

sebmarkbage · 2017-10-22T22:52:33Z

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

-    nativeEvent.initEvent('change', true, true);
-    node.dispatchEvent(nativeEvent);
+    setUntrackedValue.call(node, 'giraffe');
+    ReactTestUtils.SimulateNative.change(node);


Hm. What happens if you use the native event dispatching like it was before? Does the test still pass? If so, we should ideally use that mechanism instead since it doesn't rely on by-passing the document event listeners.

ReactTestUtils.SimulateNative is reaching into internals in a way. That's exactly what we'd like to avoid with these changes.

Totally agree, I'm gonna check it out right now and post here the results.

@sebmarkbage Yes, the tests are passing indeed.

Plus, I think using something like the node.dispatchEvent(nativeEvent); seems to be a very flexible way to obtain the expected results without reaching those internals.

Btw I verified that using SimulateNative would have still (incorrectly) passed the test if I removed this line. Whereas with native dispatch it is covered (and fails as it should).

gaearon · 2017-10-26T18:06:29Z

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

-        // I don't know how to simulate a change event on a text box.
-        this.a.dispatchEvent(changeEvent);
-        this.b.focus();
+        dispatchEventOnNode(this.a, 'change');


This has lost the focus() call. It is important. I verified that removing the fix in 84b8bbd (where this test was introduced) fails on master, but doesn't fail here anymore.

@gaearon Wow, I really wasn't aware of that fix. Thank you! 😃

gaearon · 2017-10-26T18:06:35Z

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

-    setUntrackedValue(instance.a, 'giraffe');
-
-    instance.a.dispatchEvent(inputEvent);
+    setUntrackedValue.call(instance.a, 'giraffe');


Same, but for blur().

They are necessary to cover for the fix in facebook#8240.

outdated

gaearon

This looks good to me now. I verified that reverting the fixes added with those tests also breaks the tests now.

gaearon · 2017-10-26T18:22:44Z

Sweet. Thanks so much. Do you want to help out reviewing other related PRs now that you have some experience with the kind of issues that come up testing these corner cases?

SadPandaBear · 2017-10-26T18:31:48Z

Yes, I would appreciate to help out!

Actually, I'm sure we got time, but we must verify if there's any inactive individual assigned with a test in order to liberate it so more people can come out and help with the issue

gaearon · 2017-10-26T18:35:18Z

Yeah, I plan to reach out to folks who took other tests within a few days. Need to review existing ones first :-)

…facebook#11309) * Rewrite tests depending on internal API * Remove focus being called when there was no blur event function before * Remove triggering function and just let ReactTestUtils take care * Use native events * Remove duplicate * Simulate native event when changing value on reentrant * Change wasn't being called * Use Simulate only * Use React event handlers on test * Move commentary * Lint commit * Use native event * Comment native event dispatching * Prettier * add setUntrackedValue * Prettier-all * Use dispatchEvent instead of ReactTestUtils Simulates; * Prettier * Fix lint * Remove useless arg * Wrap event dispatcher into function * Remove deprecated Event * Remove unused change event dispatcher * Fix merge * Prettier * Add missing focus/blur calls They are necessary to cover for the fix in facebook#8240.

SadPandaBear added 14 commits October 20, 2017 16:26

Rewrite tests depending on internal API

5ec6f4e

Remove focus being called when there was no blur event function before

7dbf11c

Remove triggering function and just let ReactTestUtils take care

563fadc

Use native events

33ac2be

Remove duplicate

1f3ac81

Simulate native event when changing value on reentrant

9fa1c33

Change wasn't being called

e2d2515

Use Simulate only

9a9776d

Use React event handlers on test

b3f8c2a

Move commentary

997ca86

Lint commit

ac99475

Use native event

c61cbbe

Comment native event dispatching

b8e759a

Prettier

c36dc08

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

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

9945e98

sebmarkbage previously requested changes Oct 21, 2017

View reviewed changes

add setUntrackedValue

ecac152

Prettier-all

8358d99

gaearon changed the title ~~Rewrite tests depending on internal API (#11299)~~ Rewrite ReactDOMInput-test depending on internal API (#11299) Oct 21, 2017

gaearon mentioned this pull request Oct 21, 2017

Express more tests via public API #11299

Closed

26 tasks

sebmarkbage reviewed Oct 22, 2017

View reviewed changes

SadPandaBear added 6 commits October 22, 2017 23:19

Use dispatchEvent instead of ReactTestUtils Simulates;

2893573

Prettier

df91f89

Fix lint

680ec64

Remove useless arg

fccfbf2

Wrap event dispatcher into function

a0631f0

Remove deprecated Event

628061b

AudyOdi mentioned this pull request Oct 23, 2017

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

Merged

SadPandaBear and others added 5 commits October 23, 2017 14:45

Remove unused change event dispatcher

599834a

Merge branch 'master' into master

54115e2

Fix merge

bf6d015

Prettier

10fe883

Merge branch 'master' into master

676300b

gaearon reviewed Oct 26, 2017

View reviewed changes

Add missing focus/blur calls

cbee3ca

They are necessary to cover for the fix in facebook#8240.

gaearon approved these changes Oct 26, 2017

View reviewed changes

gaearon merged commit 55b3172 into facebook:master Oct 26, 2017

gaearon mentioned this pull request Oct 26, 2017

Use only public API for EnterLeaveEventPlugin Tests #11316

Merged

gaearon mentioned this pull request Oct 27, 2017

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

Merged

GordyD mentioned this pull request Nov 13, 2017

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

Merged

Rewrite ReactDOMInput-test depending on internal API (#11299) #11309

Rewrite ReactDOMInput-test depending on internal API (#11299) #11309

Uh oh!

Conversation

SadPandaBear commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebmarkbage left a comment • 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!

sebmarkbage Oct 21, 2017

Choose a reason for hiding this comment

Uh oh!

SadPandaBear Oct 21, 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!

SadPandaBear Oct 21, 2017

Choose a reason for hiding this comment

Uh oh!

SadPandaBear commented Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebmarkbage Oct 22, 2017

Choose a reason for hiding this comment

Uh oh!

SadPandaBear Oct 23, 2017

Choose a reason for hiding this comment

Uh oh!

SadPandaBear Oct 23, 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!

gaearon Oct 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!

gaearon Oct 26, 2017

Choose a reason for hiding this comment

Uh oh!

SadPandaBear Oct 26, 2017

Choose a reason for hiding this comment

Uh oh!

gaearon Oct 26, 2017

Choose a reason for hiding this comment

Uh oh!

gaearon left a comment

Choose a reason for hiding this comment

Uh oh!

gaearon commented Oct 26, 2017

Uh oh!

SadPandaBear commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Oct 26, 2017

Uh oh!

Uh oh!

SadPandaBear commented Oct 20, 2017 •

edited

Loading

sebmarkbage left a comment •

edited

Loading

SadPandaBear Oct 21, 2017 •

edited

Loading

SadPandaBear commented Oct 21, 2017 •

edited

Loading

SadPandaBear Oct 23, 2017 •

edited

Loading

gaearon Oct 26, 2017 •

edited

Loading

SadPandaBear commented Oct 26, 2017 •

edited

Loading