Adding some server rendering unit tests. #9055

aickin · 2017-02-23T21:14:48Z

This PR adds a handful of server rendering unit tests.

With the move to Fiber, it's likely that the server rendering path and client rendering path are going to diverge significantly, and I think it'd be useful to have a more robust unit test suite for server rendering as the work on Fiber SSR begins.

This PR starts to port over tests that I wrote in (since abandoned) PR #6836. This PR only ports over some of the helper functions and 4 of the simplest tests. If this PR is accepted, I plan to submit the rest of the tests in a series of further PRs; I only added four tests here because I wanted to keep this PR as digestible as possible.

Thanks for all your great work!

Note: I ran tests, linting, and flow, but I could not get ./scripts/fiber/record-tests to work. It first complained about not being able to find jest-cli; when I npm installed jest-cli, it complained about an unresolved Promise with the error TypeError: Path must be a string. Received undefined. I expect the CI build will fail on this, and I'd appreciate any pointers to getting it to work.

aickin · 2017-02-26T20:46:21Z

Weirdly, the CI build seems to have succeeded, even though I haven't updated the list of failing fiber tests. I would greatly appreciate any help from the core team in figuring out what's going on there, and in figuring out how to run the update script on my machine.

Simultaneously, I've ported the rest of my SSR tests in a separate local branch; there are about 550 extra tests in total, and they are all passing on Stack. Unsurprisingly, many fail on Fiber, but that's arguably a good thing! If and when this PR is merged, I'll file a series of PRs to add more tests.

sophiebits · 2017-02-28T06:15:38Z

@acdlite fixed the test runner a couple days ago in #9060 – a Jest upgrade broke it, nothing you did. We're not sure why CI didn't catch that. If you rebase over that then it should work locally.

I wouldn't actually expect these tests to generally fail on Fiber. Do you have an example of a failing one handy?

I expect to have time to look at this tomorrow.

… passing lists.

aickin · 2017-02-28T06:54:32Z

@spicyj: Thanks for the tip on the test runner; I merged from master and was able to run the test list update scripts. Committed in 9257660.

I wouldn't actually expect these tests to generally fail on Fiber.

All the itRenders tests are run four times in different rendering situations:

Server render to string
Client render on top of an empty DOM
Client render on top of good server markup
Client render on top of a bad server markup

The last of those (client render on bad markup) is expected to raise a markup mismatch warning, which Fiber can't do yet, so all those tests currently fail.

aickin · 2017-02-28T06:55:06Z

I expect to have time to look at this tomorrow.

Oh, and thanks for that! 👍

… which will soon be merged into master.

zackargyle · 2017-02-28T17:21:04Z

I just want to throw out how excited I am about this. Thank you @aickin for doing this! Really stoked that Fiber SR is getting some attention. ✌🏻

sophiebits

This looks great, just a couple questions inline.

Can we also just put these in a new test file for simplicity? I'm thinking something like ReactDOMServerAndClientAttribute-test although that's a little wordy and probably still not accurate. (I don't care strongly what the name is.)

If you like, we can turn on async/await in the tests; I have no objection. Sounds like this should be pretty easy in babelrc: http://stackoverflow.com/questions/33861267/how-to-set-a-rc-file-conditionally (we set NODE_ENV=test on tests).

sophiebits · 2017-03-01T02:10:55Z

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

+//
+// Since all of the renders in this function are on the client, you can test interactivity,
+// unlike with itRenders.
+function itClientRenders(desc, testFn) {


Any reason these three are split from the other one?

None of the tests in this PR use it, but some of the ones coming later do.

The idea is that itRenders is for tests that only need to inspect the DOM, whereas itClientRenders is for tests that need to test interactivity (things like events, refs, etc.). The latter tests are only possible when ReactDOM.render has been called, which doesn't happen in the server render-only tests.

I tried to explain that in the comment above the function, but obviously it wasn't clear enough. Any suggestions on how to make it clearer?

sophiebits · 2017-03-01T02:11:40Z

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

+  ReactReconcileTransaction = require('ReactReconcileTransaction');
+
+  ExecutionEnvironment = require('ExecutionEnvironment');
+  ExecutionEnvironment.canUseDOM = false;


This should be conditional on whether we are going to do client or server rendering, correct?

You're right; this was an incantation I copied from the old code without thinking. I've changed it in a91c836 to change canUseDom to true before the call to ReactDOM.render in renderToDom (and change it back after). Seems to work right AFAICT.

sophiebits · 2017-03-01T02:13:25Z

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

@@ -549,4 +661,27 @@ describe('ReactDOMServer', () => {
      );
    }).toThrowError(/Cannot assign to read only property.*/);
  });
+
+  describe('basic rendering', function() {
+    itRenders('should render a blank div', render =>


nit: can we rename this function or the tests so that "it renders should render a blank div" is an actual sentence/phrase?

No problem. Changed in a91c836.

sophiebits · 2017-03-01T02:15:05Z

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

+    );
+
+    itRenders('should render a self-closing tag', render =>
+      render(<br />).then(e => expect(e.tagName.toLowerCase()).toBe('br')));


(I think tagName is guaranteed to be uppercase so this could just be expect(e.tagName).toBe('BR')?)

Good call. Modified all three tests in a91c836. Thanks!

@spicyj

…eworked wording of test names, corrected use of canUseDom, and simplified tests against tagName. Thanks for the help, @spicyj!

aickin · 2017-03-01T17:48:59Z

Thanks, @spicyj! I think I've addressed all the comments in your review in a91c836. I named the file ReactDOMServerIntegration-test.js because it's more of an integration-y test, but I'm not in love with the name at all; open to any suggestions.

I'm very interested in enabling async/await, but I'll put it in a separate PR so as to not slow this PR down.

Thank you for your time and thoughtful review!

sophiebits

Looks great, thanks. I think it would be nice to assert on the warning/error message text but we can solve that in a future PR. Maybe it can just take expectedErrors as an array?

sophiebits · 2017-03-01T21:44:54Z

Actually, for async/await – let's do the configuration in this file

react/scripts/jest/preprocessor.js

Line 29 in 3788654

var babelOptions = {

and only do it if the file path matches __tests__ so we don't accidentally introduce async/await into our actual bundle code. (Probably unlikely but I'd rather be safe.)

aickin · 2017-03-01T21:45:20Z

I think it would be nice to assert on the warning/error message text... Maybe it can just take expectedErrors as an array?

I was also thinking that's probably a good idea, or maybe assert on a substring/regex of the text. I'd be happy to do that a bit later.

aickin · 2017-03-01T21:46:32Z

Actually, for async/await – let's do the configuration in this file ...

Darn it; I almost had the async PR filed before you asked for this... ;)

Seriously though: good suggestion and thanks for it.

sophiebits · 2017-03-01T21:47:37Z

Sorry. :)

aickin added 2 commits February 23, 2017 13:02

Added a handful of SSR unit tests, ported from a previous pull request.

437d029

Fixing linting errors

7c85408

facebook-github-bot added CLA Signed GH Review: review-needed labels Feb 23, 2017

Fixed a test helper function to properly report errors.

c1d9787

aickin added 2 commits February 27, 2017 22:26

Merge branch 'master' into server-render-unit-tests

5531bf3

Un-nested the new rendering tests. Updated the fiber test passing/not…

9257660

… passing lists.

Edited to comply with the react/jsx-space-before-closing eslint rule,…

08b4936

… which will soon be merged into master.

sophiebits requested changes Mar 1, 2017

View reviewed changes

Response to code review from @spicyj. Moved tests to separate file, r…

a91c836

…eworked wording of test names, corrected use of canUseDom, and simplified tests against tagName. Thanks for the help, @spicyj!

sebmarkbage removed the GH Review: review-needed label Mar 1, 2017

sophiebits approved these changes Mar 1, 2017

View reviewed changes

sophiebits merged commit 3788654 into facebook:master Mar 1, 2017

aickin mentioned this pull request Mar 1, 2017

Add async/await to unit tests; modify server rendering tests to use async/await. #9089

Merged

This was referenced Mar 20, 2017

Add more SSR unit tests for elements and children. #9221

Merged

Adding new SSR tests for context, refs, and reviving markup. #9257

Merged

Adding SSR test for form fields. #9264

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding some server rendering unit tests. #9055

Adding some server rendering unit tests. #9055

aickin commented Feb 23, 2017

aickin commented Feb 26, 2017

sophiebits commented Feb 28, 2017

aickin commented Feb 28, 2017

aickin commented Feb 28, 2017

zackargyle commented Feb 28, 2017

sophiebits left a comment

sophiebits Mar 1, 2017

aickin Mar 1, 2017

sophiebits Mar 1, 2017

aickin Mar 1, 2017

sophiebits Mar 1, 2017

aickin Mar 1, 2017

sophiebits Mar 1, 2017

aickin Mar 1, 2017

aickin commented Mar 1, 2017

sophiebits left a comment

sophiebits commented Mar 1, 2017

aickin commented Mar 1, 2017

aickin commented Mar 1, 2017

sophiebits commented Mar 1, 2017

Adding some server rendering unit tests. #9055

Adding some server rendering unit tests. #9055

Conversation

aickin commented Feb 23, 2017

aickin commented Feb 26, 2017

sophiebits commented Feb 28, 2017

aickin commented Feb 28, 2017

aickin commented Feb 28, 2017

zackargyle commented Feb 28, 2017

sophiebits 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

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

aickin commented Mar 1, 2017

sophiebits left a comment

Choose a reason for hiding this comment

sophiebits commented Mar 1, 2017

aickin commented Mar 1, 2017

aickin commented Mar 1, 2017

sophiebits commented Mar 1, 2017