Jest + test renderer helpers for concurrent mode #13751

acdlite · 2018-09-29T02:02:00Z

Most of our concurrent React tests use the noop renderer. But most of those tests don't test the renderer API, and could instead be written with the test renderer. We should switch to using the test renderer whenever possible, because that's what we expect product devs and library authors to do. If test renderer is sufficient for writing most React core tests, it should be sufficient for others, too. (The converse isn't true but we should aim to dogfood test renderer as much as possible.)

This PR adds a new package, jest-react (thanks @cpojer). I've moved our existing Jest matchers into that package and added some new ones.

I'm not expecting to figure out the final API in this PR. My goal is to land something good enough that we can start dogfooding in www.

TODO: Continue migrating Suspense tests, decide on better API names

sizebot · 2018-09-29T02:11:25Z

Details of bundled changes.

Comparing: 36c5d69...07619df

scheduler

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
scheduler.development.js	n/a	n/a	0 B	19.17 KB	0 B	5.74 KB	UMD_DEV
scheduler.production.min.js	n/a	n/a	0 B	3.16 KB	0 B	1.53 KB	UMD_PROD

jest-react

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
jest-react.development.js	n/a	n/a	0 B	8.86 KB	0 B	2.97 KB	NODE_DEV
jest-react.production.min.js	n/a	n/a	0 B	3.57 KB	0 B	1.62 KB	NODE_PROD
JestReact-dev.js	n/a	n/a	0 B	5.77 KB	0 B	1.77 KB	FB_WWW_DEV
JestReact-prod.js	n/a	n/a	0 B	4.98 KB	0 B	1.58 KB	FB_WWW_PROD

Generated by 🚫 dangerJS

bvaughn

Didn't really read the ReactSuspenseWithTestRenderer-test case too closely, just left a thought about the matchers.

bvaughn · 2018-10-01T15:39:20Z

packages/jest-react/src/JestReact.js

+ * LICENSE file in the root directory of this source tree.
+ */
+
+import jestDiff from 'jest-diff';


This import is not used.

bvaughn · 2018-10-01T15:39:57Z

packages/react-reconciler/src/__tests__/ReactSuspenseWithTestRenderer-test.internal.js

+    ReactFeatureFlags.enableSuspense = true;
+    React = require('react');
+    ReactTestRenderer = require('react-test-renderer');
+    JestReact = require('jest-react');


This isn't actually used anywhere within this test.

bvaughn · 2018-10-01T15:42:30Z

packages/jest-react/README.md

@@ -0,0 +1,3 @@
+# `jest-react`
+
+Jest matchers and utilities for testing React components.


We might want to add future clarity about whether this package is meant for generic React testing or only for testing with react-test-renderer. (I assume the latter.)

Is react-test-renderer not for generic React testing? :D

No. People will see this and then it's meant for use with Enzyme.

What's an Enzyme? :D

But yeah good point :(

bvaughn · 2018-10-01T15:44:06Z

packages/jest-react/src/JestReact.js

+  return captureAssertion(() => expect(actualYields).toEqual(expectedYields));
+}
+
+export function toClearYields(ReactTestRenderer, expectedYields) {


I still think it's weird that this matcher accepts the ReactTestRenderer rather than a renderer instance.

You can yield from anywhere, not just a root.

It's more a matter of consistency. I expect people will accidentally use expect(renderer).toClearYields because that's what they do everywhere else

Ok I'll make it throw with a helpful message

bvaughn · 2018-10-01T15:49:46Z

packages/jest-react/src/JestReact.js

+export function toMatchRenderedOutput(renderer, expectedJSX) {
+  const actualJSON = renderer.toJSON();
+
+  let actualJSX;


This is...interesting. I guess it allows you to avoid the inline require, but it also seems more complex and less powerful than what I originally wrote. Why the change?

For example, the following test would work with this matcher:

const ExampleComponent = ({ foo }) => foo; const renderer = ReactTestRenderer.create(<ExampleComponent foo="bar" />); expect(renderer).toMatchRenderedOutput('bar');

But this test wouldn't:

const ExampleComponent = ({ foo }) => <div>{foo}</div>; const renderer = ReactTestRenderer.create(<ExampleComponent foo="bar" />); expect(renderer).toMatchRenderedOutput(<div>bar</div>);

And so I wouldn't be able to use this matcher for the tests in ErrorBoundaryReconciliation.

The tricky bit is that ReactTestRenderer.create will run the renderer synchronously on a new root, so it will cause React to interrupt the previous root and switch to a new one. That makes it unsuitable for some tests, and it’s a non-obvious/surprising behavior when it happens.

Also why wouldn't that test work? It should! I just used strings in the Suspense test because it was less to type.

That's fair. I think it's reasonable

I'm concerned that what we have with this PR isn't flexible enough to be very useful with many "real" components.

Oh whoops, missed your second comment.

Also why wouldn't that test work?

Lots of undefined property access errors. If you think it should be possible for it to work with more complex test cases, then I can help dig into how we'd fix it.

Yeah it's probably just a bug. I'll add more tests as I go.

Cool. In that case, it would be great to update ErrorBoundaryReconciliation to use the new matcher.

Yeah it was just a bug :) Updated ErrorBoundaryReconciliation-test

@cpojer

Most of our concurrent React tests use the noop renderer. But most of those tests don't test the renderer API, and could instead be written with the test renderer. We should switch to using the test renderer whenever possible, because that's what we expect product devs and library authors to do. If test renderer is sufficient for writing most React core tests, it should be sufficient for others, too. (The converse isn't true but we should aim to dogfood test renderer as much as possible.) This PR adds a new package, jest-react (thanks @cpojer). I've moved our existing Jest matchers into that package and added some new ones. I'm not expecting to figure out the final API in this PR. My goal is to land something good enough that we can start dogfooding in www. TODO: Continue migrating Suspense tests, decide on better API names

bvaughn

Accept to unblock. Let's tweak the README to make the scope of this package clearer. Also fix the CI failure.

- Errors if user attempts to flush when log of yields is not empty - Throws if argument passed to toClearYields is not ReactTestRenderer

- toFlushAll -> toFlushAndYield - toFlushAndYieldThrough -> - toClearYields -> toHaveYielded Also added toFlushWithoutYielding

@cpojer

* Jest + test renderer helpers for concurrent mode Most of our concurrent React tests use the noop renderer. But most of those tests don't test the renderer API, and could instead be written with the test renderer. We should switch to using the test renderer whenever possible, because that's what we expect product devs and library authors to do. If test renderer is sufficient for writing most React core tests, it should be sufficient for others, too. (The converse isn't true but we should aim to dogfood test renderer as much as possible.) This PR adds a new package, jest-react (thanks @cpojer). I've moved our existing Jest matchers into that package and added some new ones. I'm not expecting to figure out the final API in this PR. My goal is to land something good enough that we can start dogfooding in www. TODO: Continue migrating Suspense tests, decide on better API names * Add additional invariants to prevent common errors - Errors if user attempts to flush when log of yields is not empty - Throws if argument passed to toClearYields is not ReactTestRenderer * Better method names - toFlushAll -> toFlushAndYield - toFlushAndYieldThrough -> - toClearYields -> toHaveYielded Also added toFlushWithoutYielding * Fix jest-react exports * Tweak README

@cpojer

* Jest + test renderer helpers for concurrent mode Most of our concurrent React tests use the noop renderer. But most of those tests don't test the renderer API, and could instead be written with the test renderer. We should switch to using the test renderer whenever possible, because that's what we expect product devs and library authors to do. If test renderer is sufficient for writing most React core tests, it should be sufficient for others, too. (The converse isn't true but we should aim to dogfood test renderer as much as possible.) This PR adds a new package, jest-react (thanks @cpojer). I've moved our existing Jest matchers into that package and added some new ones. I'm not expecting to figure out the final API in this PR. My goal is to land something good enough that we can start dogfooding in www. TODO: Continue migrating Suspense tests, decide on better API names * Add additional invariants to prevent common errors - Errors if user attempts to flush when log of yields is not empty - Throws if argument passed to toClearYields is not ReactTestRenderer * Better method names - toFlushAll -> toFlushAndYield - toFlushAndYieldThrough -> - toClearYields -> toHaveYielded Also added toFlushWithoutYielding * Fix jest-react exports * Tweak README

@cpojer

* Jest + test renderer helpers for concurrent mode Most of our concurrent React tests use the noop renderer. But most of those tests don't test the renderer API, and could instead be written with the test renderer. We should switch to using the test renderer whenever possible, because that's what we expect product devs and library authors to do. If test renderer is sufficient for writing most React core tests, it should be sufficient for others, too. (The converse isn't true but we should aim to dogfood test renderer as much as possible.) This PR adds a new package, jest-react (thanks @cpojer). I've moved our existing Jest matchers into that package and added some new ones. I'm not expecting to figure out the final API in this PR. My goal is to land something good enough that we can start dogfooding in www. TODO: Continue migrating Suspense tests, decide on better API names * Add additional invariants to prevent common errors - Errors if user attempts to flush when log of yields is not empty - Throws if argument passed to toClearYields is not ReactTestRenderer * Better method names - toFlushAll -> toFlushAndYield - toFlushAndYieldThrough -> - toClearYields -> toHaveYielded Also added toFlushWithoutYielding * Fix jest-react exports * Tweak README

@cpojer

* Jest + test renderer helpers for concurrent mode Most of our concurrent React tests use the noop renderer. But most of those tests don't test the renderer API, and could instead be written with the test renderer. We should switch to using the test renderer whenever possible, because that's what we expect product devs and library authors to do. If test renderer is sufficient for writing most React core tests, it should be sufficient for others, too. (The converse isn't true but we should aim to dogfood test renderer as much as possible.) This PR adds a new package, jest-react (thanks @cpojer). I've moved our existing Jest matchers into that package and added some new ones. I'm not expecting to figure out the final API in this PR. My goal is to land something good enough that we can start dogfooding in www. TODO: Continue migrating Suspense tests, decide on better API names * Add additional invariants to prevent common errors - Errors if user attempts to flush when log of yields is not empty - Throws if argument passed to toClearYields is not ReactTestRenderer * Better method names - toFlushAll -> toFlushAndYield - toFlushAndYieldThrough -> - toClearYields -> toHaveYielded Also added toFlushWithoutYielding * Fix jest-react exports * Tweak README

acdlite requested review from sophiebits and bvaughn September 29, 2018 02:02

facebook-github-bot added the CLA Signed label Sep 29, 2018

acdlite force-pushed the jest-react branch from 3a20e73 to a4143f4 Compare September 29, 2018 02:03

klamping mentioned this pull request Sep 29, 2018

React PR Test Report Listing klamping/react-webdriverio-tests#1

Open

bvaughn reviewed Oct 1, 2018

View reviewed changes

acdlite force-pushed the jest-react branch from a4143f4 to e51c258 Compare October 3, 2018 16:14

acdlite changed the title ~~[WIP] Jest + test renderer helpers for concurrent mode~~ Jest + test renderer helpers for concurrent mode Oct 3, 2018

acdlite force-pushed the jest-react branch from e51c258 to 7de88f1 Compare October 3, 2018 16:16

bvaughn approved these changes Oct 3, 2018

View reviewed changes

Add additional invariants to prevent common errors

9bccbbf

- Errors if user attempts to flush when log of yields is not empty - Throws if argument passed to toClearYields is not ReactTestRenderer

acdlite force-pushed the jest-react branch from 7de88f1 to 9bccbbf Compare October 3, 2018 18:00

Better method names

2e8a8cf

- toFlushAll -> toFlushAndYield - toFlushAndYieldThrough -> - toClearYields -> toHaveYielded Also added toFlushWithoutYielding

acdlite force-pushed the jest-react branch from 825d2b3 to 2e8a8cf Compare October 3, 2018 21:10

Fix jest-react exports

24ce840

acdlite mentioned this pull request Oct 3, 2018

[react-test-renderer] toJSX #13231

Closed

acdlite force-pushed the jest-react branch 4 times, most recently from c981713 to 180f925 Compare October 4, 2018 00:14

Tweak README

07619df

acdlite force-pushed the jest-react branch from 180f925 to 07619df Compare October 4, 2018 00:26

acdlite merged commit 96bcae9 into facebook:master Oct 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jest + test renderer helpers for concurrent mode #13751

Jest + test renderer helpers for concurrent mode #13751

acdlite commented Sep 29, 2018

sizebot commented Sep 29, 2018 •

edited

scheduler

jest-react

bvaughn left a comment

bvaughn Oct 1, 2018

bvaughn Oct 1, 2018

bvaughn Oct 1, 2018

acdlite Oct 3, 2018

bvaughn Oct 3, 2018

acdlite Oct 3, 2018

bvaughn Oct 1, 2018

acdlite Oct 3, 2018

bvaughn Oct 3, 2018 •

edited

acdlite Oct 3, 2018

bvaughn Oct 1, 2018

acdlite Oct 1, 2018

acdlite Oct 1, 2018 •

edited

bvaughn Oct 1, 2018

bvaughn Oct 1, 2018

acdlite Oct 1, 2018

bvaughn Oct 1, 2018

acdlite Oct 3, 2018 •

edited

bvaughn Oct 3, 2018

bvaughn left a comment

		@@ -0,0 +1,3 @@
		# `jest-react`

		Jest matchers and utilities for testing React components.

Jest + test renderer helpers for concurrent mode #13751

Jest + test renderer helpers for concurrent mode #13751

Conversation

acdlite commented Sep 29, 2018

sizebot commented Sep 29, 2018 • edited

scheduler

jest-react

bvaughn 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

bvaughn Oct 3, 2018 • edited

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

acdlite Oct 1, 2018 • edited

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

acdlite Oct 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn left a comment

Choose a reason for hiding this comment

sizebot commented Sep 29, 2018 •

edited

bvaughn Oct 3, 2018 •

edited

acdlite Oct 1, 2018 •

edited

acdlite Oct 3, 2018 •

edited