Run 90% of tests on compiled bundles (both development and production) #11633

gaearon · 2017-11-22T21:41:03Z

Adds yarn test-build and yarn test-build-prod that run on CI.
You can also run them locally after yarn build <what you need> --type=NODE.

Tests ending with -test.internal.js are blacklisted. They fail due to internal imports (e.g. shared/ReactFeatureFlags, mocks of local files, or direct file references). Some of them we can fix, but some will need to stay .internal forever which seems okay to me as long as it‘s not many.

Note this means that if a non-internal test imports an internal file, it will not pass CI. My suggestion for cases when you need an internal file (most commonly a feature flag or a mock) is to add a second X-test.internal.js file that only contains the "bundle-unfriendly" code. This way we can still run as many tests as we can (and even move tests between two files as some of them lose dependency on internals, e.g. if feature flag is removed).

This is all very new, and it's likely we'll uncover some pain points. I can't predict what those will be.
Let's try and see? We can always disable on CI if it's too painful.

Introduces a more consistent naming scheme.

Only files ending with -test.public.js are opted in (so far we don't have any).

GCC seems to remove `new` from `new Error()` which broke our proxy.

This lets us test more bundles.

gaearon · 2017-11-23T02:46:58Z

About half the tests are enabled. Note that there's a ton of tests in DOMServerIntegration (which is currently disabled because it uses a feature flag). Once we fix that we should get a whole lot more passing.

Development bundles:

Production bundles:

CI passes too. 🙂

nhunzaker

Not really my area, but very cool!

nhunzaker · 2017-11-23T03:12:04Z

scripts/jest/config.source.js

+  setupFiles: [require.resolve('./setupEnvironment.js')],
+  setupTestFrameworkScriptFile: require.resolve('./setupTests.js'),
+  testRegex: '/__tests__/.*(\\.js|\\.coffee|[^d]\\.ts)$',
+  moduleFileExtensions: ['js', 'json', 'node', 'coffee', 'ts'],


What is .node? I don't see it anywhere (other than .node.js)

Dunno. Used to be there. Maybe because it's one of Jest defaults? I can remove.

trueadm

I'm not a big fan of the private.js naming scheme. It just feels like a little overloaded. What if the private tests files were in a different directory?

I feel like these are actually different kinds of test and like industry practice – we should probably label these as what they are, so instead of __tests__, we could call it __unit__ and __acceptance__ or something.

Otherwise, this PR is awesome and we really need tests on our bundles, so nice one :)

Also the CI seems to be failing – but you probably know that.

gaearon · 2017-11-23T12:28:17Z

we could call it unit and acceptance or something.

Some counter arguments:

"Private tests" should look "wrong". It should be obvious you shouldn't be adding more of them unless you really know what you're doing (e.g. adding a feature flag). Existing tests that are private (but don't have to be) should make you feel guilty. Legitimising them with a name like "unit" makes them seem like a good thing to write.
The main distinction is whether they can run on bundles, not unit vs integration in traditional sense. When we fix the last dozen files, we won't have any unit tests at all. But even integration tests sometimes can't run on bundles (e.g. have to use an internal or mock something). Maybe there's some middle ground in naming though.
Related to the previous point, it should be easy to movie individual test cases between "public" and "private" versions of the same test file. Our goal is to maximize the number of test cases we run against bundles. So if a single case suddenly needs a feature flag it should be obvious where to put it (a "private" file next to its normal case). Separating them by directory will make it less obvious (and probably discourage people from moving individual tests back and forth).

gaearon · 2017-11-23T12:29:34Z

Maybe .test.js and .test-internal.js.

trueadm · 2017-11-23T12:42:10Z

@gaearon I think the naming is confusing me more. To me a private test seems like it's for testing internal private APIs/implementations. I think this comes from public vs private classes and methods etc. If this confused me, I'm sure it will confuse others too.

Okay, I'm going to suggest something radical but I think it actually makes more sense given what we're aiming to getting to: we move all public bundle tests into their own package. Call it react-tests or something, but this makes it clear that these tests are clearly public and test only bundles as there will be no local importing of files. Private tests can stay in __tests__ next to the source until we get around the moving them.

…rnal.js Some of these are using private APIs. Some have other issues.

gaearon · 2017-11-23T13:57:47Z

Updated to use .internal.js as convention for the time being.

This enables us to further split it down. Good both for parallelization and extracting public parts.

This enables them to opt other DOMServerIntegration tests into bundle testing.

It was way too slow to run all these in sequence.

We used to do this to simulate testing separate bundles. But now we actually *do* test bundles. So there is no need for this, as it makes tests slower.

gaearon · 2017-11-23T15:29:48Z

82% tests now running on bundles.

Also add test-prod-build as alias for test-build-prod because I keep messing them up.

This fixes other issues and finally lets us run ReactNoop tests against a prod bundle.

Now that GCC generator issue is fixed, we can do this. I split ErrorLogging test separately because it does mocking. Other error handling tests don't need it.

gaearon · 2017-11-23T16:21:39Z

2312 of 2565: we have 90% tests running!

gaearon · 2017-11-23T17:24:46Z

This uncovered an issue with SSR warnings that print something different with flat bundles due to shared event system injection. I am changing the logic to be consistent in the next commit.

With flat bundles, we couldn't produce a good warning for <div onclick={}> on SSR because it doesn't use the event system. However the issue was not visible in normal Jest runs because the event plugins have been injected by the time the test ran. To solve this, I am explicitly passing whether event system is available as an argument to the hook. This makes the behavior consistent between source and bundle tests. Then I change the tests to document the actual logic and _attempt_ to show a nice message (e.g. we know for sure `onclick` is a bad event but we don't know the right name for it on the server so we just say a generic message about camelCase naming convention).

facebook#11633) * Extract Jest config into a separate file * Refactor Jest scripts directory structure Introduces a more consistent naming scheme. * Add yarn test-bundles and yarn test-prod-bundles Only files ending with -test.public.js are opted in (so far we don't have any). * Fix error decoding for production bundles GCC seems to remove `new` from `new Error()` which broke our proxy. * Build production version of react-noop-renderer This lets us test more bundles. * Switch to blacklist (exclude .private.js tests) * Rename tests that are currently broken against bundles to *-test.internal.js Some of these are using private APIs. Some have other issues. * Add bundle tests to CI * Split private and public ReactJSXElementValidator tests * Remove internal deps from ReactServerRendering-test and make it public * Only run tests directly in __tests__ This lets us share code between test files by placing them in __tests__/utils. * Remove ExecutionEnvironment dependency from DOMServerIntegrationTest It's not necessary since Stack. * Split up ReactDOMServerIntegration into test suite and utilities This enables us to further split it down. Good both for parallelization and extracting public parts. * Split Fragment tests from other DOMServerIntegration tests This enables them to opt other DOMServerIntegration tests into bundle testing. * Split ReactDOMServerIntegration into different test files It was way too slow to run all these in sequence. * Don't reset the cache twice in DOMServerIntegration tests We used to do this to simulate testing separate bundles. But now we actually *do* test bundles. So there is no need for this, as it makes tests slower. * Rename test-bundles* commands to test-build* Also add test-prod-build as alias for test-build-prod because I keep messing them up. * Use regenerator polyfill for react-noop This fixes other issues and finally lets us run ReactNoop tests against a prod bundle. * Run most Incremental tests against bundles Now that GCC generator issue is fixed, we can do this. I split ErrorLogging test separately because it does mocking. Other error handling tests don't need it. * Update sizes * Fix ReactMount test * Enable ReactDOMComponent test * Fix a warning issue uncovered by flat bundle testing With flat bundles, we couldn't produce a good warning for <div onclick={}> on SSR because it doesn't use the event system. However the issue was not visible in normal Jest runs because the event plugins have been injected by the time the test ran. To solve this, I am explicitly passing whether event system is available as an argument to the hook. This makes the behavior consistent between source and bundle tests. Then I change the tests to document the actual logic and _attempt_ to show a nice message (e.g. we know for sure `onclick` is a bad event but we don't know the right name for it on the server so we just say a generic message about camelCase naming convention).

connorjclark · 2018-01-10T06:55:08Z

I'm curious what sort of issues this testing uncovers. I hope @gaearon or others could update this thread with how this has helped!

gaearon · 2018-01-10T15:24:30Z

It enables things like #11967.

facebook-github-bot added the CLA Signed label Nov 22, 2017

gaearon force-pushed the bundle-tests branch from 34c2a2f to e2278a8 Compare November 22, 2017 21:53

gaearon added 2 commits November 23, 2017 01:20

Extract Jest config into a separate file

ed4b018

Refactor Jest scripts directory structure

e944d7e

Introduces a more consistent naming scheme.

gaearon force-pushed the bundle-tests branch 2 times, most recently from 0ba8587 to 6bd253d Compare November 23, 2017 01:28

Add yarn test-bundles and yarn test-prod-bundles

18dbd55

Only files ending with -test.public.js are opted in (so far we don't have any).

gaearon force-pushed the bundle-tests branch from 6bd253d to 18dbd55 Compare November 23, 2017 01:30

gaearon added 2 commits November 23, 2017 01:44

Fix error decoding for production bundles

8e6e601

GCC seems to remove `new` from `new Error()` which broke our proxy.

Build production version of react-noop-renderer

ef4b581

This lets us test more bundles.

gaearon force-pushed the bundle-tests branch from 3752760 to cd73c78 Compare November 23, 2017 02:35

gaearon changed the title ~~Run some tests on bundles~~ Run a subset of tests on production bundles Nov 23, 2017

gaearon changed the title ~~Run a subset of tests on production bundles~~ Run a subset of tests on compiled bundles (both development and production) Nov 23, 2017

gaearon mentioned this pull request Nov 23, 2017

Run some unit tests on compiled bundles #9955

Closed

gaearon requested review from bvaughn, trueadm and clemmy November 23, 2017 02:47

nhunzaker approved these changes Nov 23, 2017

View reviewed changes

trueadm approved these changes Nov 23, 2017

View reviewed changes

gaearon added 4 commits November 23, 2017 13:46

Switch to blacklist (exclude .private.js tests)

6d03d8d

Rename tests that are currently broken against bundles to *-test.inte…

936e500

…rnal.js Some of these are using private APIs. Some have other issues.

Add bundle tests to CI

7409c6a

Split private and public ReactJSXElementValidator tests

f05ff6b

gaearon force-pushed the bundle-tests branch from d902803 to ac377fc Compare November 23, 2017 13:57

Remove internal deps from ReactServerRendering-test and make it public

95a356e

gaearon force-pushed the bundle-tests branch from ac377fc to 0a90485 Compare November 23, 2017 14:07

gaearon added 3 commits November 23, 2017 14:36

Split up ReactDOMServerIntegration into test suite and utilities

0df06f4

This enables us to further split it down. Good both for parallelization and extracting public parts.

Split Fragment tests from other DOMServerIntegration tests

8a35e3b

This enables them to opt other DOMServerIntegration tests into bundle testing.

Split ReactDOMServerIntegration into different test files

96ccaf1

It was way too slow to run all these in sequence.

gaearon force-pushed the bundle-tests branch from cc809ed to 96ccaf1 Compare November 23, 2017 15:13

Don't reset the cache twice in DOMServerIntegration tests

3fb1d4a

We used to do this to simulate testing separate bundles. But now we actually *do* test bundles. So there is no need for this, as it makes tests slower.

Rename test-bundles* commands to test-build*

c14cae5

Also add test-prod-build as alias for test-build-prod because I keep messing them up.

gaearon force-pushed the bundle-tests branch from 0dec5e6 to c14cae5 Compare November 23, 2017 15:37

gaearon added 5 commits November 23, 2017 16:02

Use regenerator polyfill for react-noop

0ecf429

This fixes other issues and finally lets us run ReactNoop tests against a prod bundle.

Run most Incremental tests against bundles

40ea898

Now that GCC generator issue is fixed, we can do this. I split ErrorLogging test separately because it does mocking. Other error handling tests don't need it.

Update sizes

111bab1

Fix ReactMount test

ef69681

Enable ReactDOMComponent test

4da70e7

gaearon changed the title ~~Run a subset of tests on compiled bundles (both development and production)~~ Run 90% of tests on compiled bundles (both development and production) Nov 23, 2017

gaearon force-pushed the bundle-tests branch from fca40fc to 69f7126 Compare November 23, 2017 17:31

gaearon force-pushed the bundle-tests branch from 69f7126 to 8888c46 Compare November 23, 2017 17:34

gaearon merged commit fa7a97f into facebook:master Nov 23, 2017

gaearon deleted the bundle-tests branch November 23, 2017 17:45

This was referenced Nov 25, 2017

Don't reset error codes on CI build #11655

Merged

Investigate CI failure (possible race condition) #11657

Closed

elas7 mentioned this pull request Nov 6, 2021

Delete unused Circle CI scripts. #22710

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run 90% of tests on compiled bundles (both development and production) #11633

Run 90% of tests on compiled bundles (both development and production) #11633

gaearon commented Nov 22, 2017 •

edited

gaearon commented Nov 23, 2017

nhunzaker left a comment

nhunzaker Nov 23, 2017

gaearon Nov 23, 2017

trueadm left a comment •

edited

gaearon commented Nov 23, 2017

gaearon commented Nov 23, 2017

trueadm commented Nov 23, 2017 •

edited

gaearon commented Nov 23, 2017

gaearon commented Nov 23, 2017

gaearon commented Nov 23, 2017

gaearon commented Nov 23, 2017

connorjclark commented Jan 10, 2018

gaearon commented Jan 10, 2018

Run 90% of tests on compiled bundles (both development and production) #11633

Run 90% of tests on compiled bundles (both development and production) #11633

Conversation

gaearon commented Nov 22, 2017 • edited

gaearon commented Nov 23, 2017

nhunzaker left a comment

Choose a reason for hiding this comment

nhunzaker Nov 23, 2017

Choose a reason for hiding this comment

gaearon Nov 23, 2017

Choose a reason for hiding this comment

trueadm left a comment • edited

Choose a reason for hiding this comment

gaearon commented Nov 23, 2017

gaearon commented Nov 23, 2017

trueadm commented Nov 23, 2017 • edited

gaearon commented Nov 23, 2017

gaearon commented Nov 23, 2017

gaearon commented Nov 23, 2017

gaearon commented Nov 23, 2017

connorjclark commented Jan 10, 2018

gaearon commented Jan 10, 2018

gaearon commented Nov 22, 2017 •

edited

trueadm left a comment •

edited

trueadm commented Nov 23, 2017 •

edited