Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Nov 10, 2017
Merged

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

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ae96f21
Remove inputValueTracking from ReactDOMComponent-test dependency
AudyOdi Oct 20, 2017
cb7b802
prettier
AudyOdi Oct 23, 2017
3e9bfe3
use node._valueTracker and add some test cases to make sure that valu…
AudyOdi Oct 26, 2017
d480ce7
using Object.getOwnPropertyDescriptor to get the tracked value
AudyOdi Oct 30, 2017
4614636
move getValueTracker to each test case and use its corresponding prot…
AudyOdi Oct 31, 2017
aa75dde
remove tests and move the value tracker definition before React is im…
AudyOdi Nov 9, 2017
2dadb8d
Delete these tests completely
gaearon Nov 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 28 additions & 7 deletions packages/react-dom/src/__tests__/ReactDOMComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ describe('ReactDOMComponent', () => {
var ReactTestUtils;
var ReactDOM;
var ReactDOMServer;
var inputValueTracking;

function normalizeCodeLocInfo(str) {
return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
Expand All @@ -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', () => {
Expand Down Expand Up @@ -832,6 +829,12 @@ describe('ReactDOMComponent', () => {

describe('mountComponent', () => {
var mountComponent;
var getValueTracker = (node: HTMLInputElement | HTMLTextAreaElement) => {
return Object.getOwnPropertyDescriptor(
node.constructor.prototype,
'value',
).get;
};
Copy link
Contributor

@SadPandaBear SadPandaBear Oct 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

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


beforeEach(() => {
mountComponent = function(props) {
Expand Down Expand Up @@ -1151,19 +1154,37 @@ describe('ReactDOMComponent', () => {
<input type="text" defaultValue="foo" />,
container,
);
var getTrackedValue = getValueTracker(inst);
Copy link
Contributor

@SadPandaBear SadPandaBear Oct 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

@AudyOdi AudyOdi Oct 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

@gaearon gaearon Oct 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@AudyOdi AudyOdi Oct 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

expect(getTrackedValue.call(inst)).toEqual('foo');

var tracker = inputValueTracking._getTrackerFromNode(inst);
inst.defaultValue = 'new foo';
expect(getTrackedValue.call(inst)).not.toEqual('new foo');
expect(getTrackedValue.call(inst)).toEqual('foo');

expect(tracker.getValue()).toEqual('foo');
inst.value = 'bar';
expect(getTrackedValue.call(inst)).toEqual('bar');

// even if the value changes to empty string, should not return to defaultValue
inst.value = '';
expect(getTrackedValue.call(inst)).toEqual('');
});

it('should track textarea values', () => {
var container = document.createElement('div');
var inst = ReactDOM.render(<textarea defaultValue="foo" />, container);
var getTrackedValue = getValueTracker(inst);
expect(getTrackedValue.call(inst)).toEqual('foo');

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

var tracker = inputValueTracking._getTrackerFromNode(inst);
inst.value = 'bar';
expect(getTrackedValue.call(inst)).toEqual('bar');

expect(tracker.getValue()).toEqual('foo');
// even if the value changes to empty string, should not return to defaultValue
inst.value = '';
expect(getTrackedValue.call(inst)).toEqual('');
});

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