ValidateDOMNesting tests(#11299) #11742

imanushree · 2017-12-01T15:02:30Z

I have 2 doubts in this( 1 is an error, other is a clarification). Have marked them as TODO so that its easy to spot.

imanushree · 2017-12-01T15:04:19Z

@gaearon Could you please take a quick look at the lines marked with TODO? I have added my doubts as comments. Thanks in advance :)

gaearon · 2017-12-01T15:52:38Z

packages/react-dom/src/__tests__/validateDOMNesting-test.internal.js

-];
+function expectInvalidNestingWarning(shouldWarn, tags) {
+  // TODO: I get the error <spyOn> : error has already been spied upon although
+  // i am resetting the jest modules before every assertion


Yeah. So, jest.resetModules() only resets requires, forcing next require() to load fresh versions. But it doesn't actually reset global objects like console.

I think it's fine to just put spyOnDev(console, 'error') in the beginning of every test case. That's pretty explicit.

gaearon · 2017-12-01T15:54:20Z

packages/react-dom/src/__tests__/validateDOMNesting-test.internal.js

+      expectInvalidNestingWarning(false, ['div', 'ul', 'li', 'dd', 'li']);
+
+      // TODO: Previously used test scenarios which will fail now because
+      // root element is div. Remove them?


No. Let's change the code in expectInvalidNestingWarning to treat the first item in the array as the root element itself.

For example, for ['div', 'label', 'div'] have it render <label><div /></label> into a DOM div.

Then, for ['body', 'datalist', 'option'] it would render <datalist><option /></datalist> into a body.

imanushree · 2017-12-04T03:17:21Z

@gaearon This PR is ready for your review :)

gaearon

Let's be more specific about the warnings we check for.

gaearon · 2017-12-04T13:30:13Z

packages/react-dom/src/__tests__/validateDOMNesting-test.internal.js

+  while (tags.length) {
+    element = React.createElement(tags.pop(), null, element);
+  }
+  const errorCount = console.error.calls.count();


Let's call console.error.calls.reset() here. Then you can assert a specific count later (presumably 1?)

I see it's not 1 for some tests.
In that case let's pass expectedWarnings into this function as an argument?

e.g.

expectInvalidNestingWarning(true, ['body', 'body'], [ 'validateDOMNesting(...): <body> cannot appear as a child of <body>', 'render(): Rendering components directly into document.body is discouraged' ]);

Then you can assert that we have exactly that number of warnings, and they match the passed ones with toContain().

gaearon · 2017-12-04T13:32:45Z

packages/react-dom/src/__tests__/validateDOMNesting-test.internal.js

+
+function expectInvalidNestingWarning(shouldWarn, tags) {
+  let element = null;
+  const container = document.createElement(tags.splice(0, 1));


I think it's a bit confusing that this function can mutate its argument. Let's copy tags at the beginning of this function?

gaearon · 2017-12-04T13:35:03Z

packages/react-dom/src/__tests__/validateDOMNesting-test.internal.js

+  const errorCount = console.error.calls.count();
+  ReactDOM.render(element, container);
+  if (shouldWarn) {
+    expect(console.error.calls.count() > errorCount).toEqual(true);


Related to previous comment: this should assert on a specific number.

We should also assert on a specific message. It's fine if it's not 100% specific: you can assert on a part of the message with toContain().

imanushree · 2017-12-06T06:25:46Z

@gaearon Have made the changes. Please take a look :)

gaearon · 2017-12-06T12:18:09Z

packages/react-dom/src/__tests__/validateDOMNesting-test.internal.js

-      });
-    }
-  });
-
  it('allows valid nestings', () => {
    if (__DEV__) {


Let’s remove DEV check here in tests. We want the code to run in any case. However we want to put the assertions about warnings inside a DEV check since in production they aren’t expected to warn.

@gaearon I believe the reason why the DEV checks are right at the beginning of the tests is because ALL the assertions are related to warnings, which will not be testable in prod as we wont be seeing any. So no point in letting any of the code execute in prod.

I used the __DEV__ checks at the beginning of the tests because some functions attached to validateDOMNesting are actually undefined in production so the tests would crash. That's not a problem when tests are written against the public API.

which will not be testable in prod as we wont be seeing any. So no point in letting any of the code execute in prod.

There is still value in executing this code in PROD. In particular, we want to verify that the code doesn't crash. For example, imagine we made a mistake and called validateDOMNesting.updatedAncestorInfo in production environment. It would throw. The tests should verify that nothing bad happens by stressing that code path.

In this particular case it's true that if there was a big issue like this, other tests would probably fail too. But we still want to have coverage like this. I hope it makes sense!

gaearon · 2017-12-06T19:09:06Z

Looks like you need to run yarn prettier?

gaearon · 2017-12-06T19:11:35Z

You can run production tests with:

yarn test-prod --watch validateDOMNesting

You'll see that it fails because console.error is not mocked in production. To fix the issue, you need to surround both the reset() call and the warning count/message checks into __DEV__ blocks. Just running ReactDOM.render() is enough because that stresses the production code path, but indeed we don't expect any warnings. (And since we didn't mock console.error in production, any unexpected warning would fail the test anyway, so we don't need to check even for 0 warnings.)

* Rewrite tests using only public API. * Modified the tests to prevent duplication of code. * Code review changes implemented. * Removed the .internal from the test file name as its now written using public APIs.

imanushree · 2017-12-07T04:50:05Z

@gaearon Thanks a lot for explaining why letting the code run in prod is useful. It makes a lot of sense!!! I have made the changes. Please take a look :)

gaearon · 2017-12-07T11:09:26Z

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

+
+function expectInvalidNestingWarning(shouldWarn, tagsList, warningsList = []) {
+  let element = null;
+  let tags = tagsList;


This doesn’t copy the array, it only creates a reference to it. Therefore the splice call later mutates it.

It would be clearer IMO if we didn’t mutate the input. Otherwise, for example, declaring an array as a variable in the test and passing it two times wouldn’t work. We don’t do this but it could be potentially confusing in the future.

To solve it we need to copy the array on this line.

Now that we pass warnings, we don't need to pass a boolean.

gaearon · 2017-12-07T18:39:02Z

I added some extra coverage to ensure we include the component stack traces into the warning messages. I needed to convert createElement calls to JSX since the stacks are composed in part by the JSX transform.

gaearon · 2017-12-07T18:46:01Z

Thanks for doing this!

facebook-github-bot added the CLA Signed label Dec 1, 2017

gaearon reviewed Dec 1, 2017

View reviewed changes

imanushree force-pushed the validateDOMNesting-test branch from 5a76ca2 to 993f736 Compare December 4, 2017 03:12

imanushree changed the title ~~[WIP] ValidateDOMNesting tests(#11299)~~ ValidateDOMNesting tests(#11299) Dec 4, 2017

gaearon requested changes Dec 4, 2017

View reviewed changes

imanushree force-pushed the validateDOMNesting-test branch 2 times, most recently from ffb5e71 to a20d835 Compare December 6, 2017 06:13

gaearon requested changes Dec 6, 2017

View reviewed changes

imanushree force-pushed the validateDOMNesting-test branch from a20d835 to fe444f5 Compare December 6, 2017 18:52

ValidateDOMNesting tests(facebook#11299)

90eda25

* Rewrite tests using only public API. * Modified the tests to prevent duplication of code. * Code review changes implemented. * Removed the .internal from the test file name as its now written using public APIs.

imanushree force-pushed the validateDOMNesting-test branch from fe444f5 to 90eda25 Compare December 7, 2017 04:45

gaearon reviewed Dec 7, 2017

View reviewed changes

gaearon added 3 commits December 7, 2017 18:22

Remove mutation

25f9199

Remove unnecessary argument

834a162

Now that we pass warnings, we don't need to pass a boolean.

Move things around a bit, and add component stack assertions

3be5a2e

gaearon approved these changes Dec 7, 2017

View reviewed changes

gaearon merged commit 8256823 into facebook:master Dec 7, 2017

gaearon mentioned this pull request Dec 7, 2017

Express more tests via public API #11299

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ValidateDOMNesting tests(#11299) #11742

ValidateDOMNesting tests(#11299) #11742

imanushree commented Dec 1, 2017

imanushree commented Dec 1, 2017

gaearon Dec 1, 2017

gaearon Dec 1, 2017 •

edited

Loading

imanushree commented Dec 4, 2017

gaearon left a comment

gaearon Dec 4, 2017

gaearon Dec 4, 2017

gaearon Dec 4, 2017

gaearon Dec 4, 2017

imanushree commented Dec 6, 2017

gaearon Dec 6, 2017

imanushree Dec 6, 2017

gaearon Dec 6, 2017

gaearon commented Dec 6, 2017

gaearon commented Dec 6, 2017

imanushree commented Dec 7, 2017

gaearon Dec 7, 2017

gaearon commented Dec 7, 2017

gaearon commented Dec 7, 2017

ValidateDOMNesting tests(#11299) #11742

ValidateDOMNesting tests(#11299) #11742

Conversation

imanushree commented Dec 1, 2017

imanushree commented Dec 1, 2017

Choose a reason for hiding this comment

gaearon Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

imanushree commented Dec 4, 2017

gaearon left a 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

imanushree commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Dec 6, 2017

gaearon commented Dec 6, 2017

imanushree commented Dec 7, 2017

Choose a reason for hiding this comment

gaearon commented Dec 7, 2017

gaearon commented Dec 7, 2017

gaearon Dec 1, 2017 •

edited

Loading