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.

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

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

Merged
merged 23 commits into from Nov 23, 2017

Conversation

Projects
None yet
5 participants
@gaearon
Member

gaearon commented Nov 22, 2017

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.

gaearon added some commits Nov 22, 2017

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.

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

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

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 23, 2017

Member

it's happening

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:

screen shot 2017-11-23 at 02 45 21

Production bundles:

screen shot 2017-11-23 at 02 42 23

CI passes too. 🙂

Member

gaearon commented Nov 23, 2017

it's happening

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:

screen shot 2017-11-23 at 02 45 21

Production bundles:

screen shot 2017-11-23 at 02 42 23

CI passes too. 🙂

@gaearon gaearon requested review from bvaughn, trueadm and clemmy Nov 23, 2017

@nhunzaker

Not really my area, but very cool!

Show outdated Hide outdated scripts/jest/config.source.js
@trueadm

trueadm approved these changes Nov 23, 2017 edited

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

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 23, 2017

Member

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).
Member

gaearon commented Nov 23, 2017

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

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 23, 2017

Member

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

Member

gaearon commented Nov 23, 2017

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

@trueadm

This comment has been minimized.

Show comment
Hide comment
@trueadm

trueadm Nov 23, 2017

Contributor

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

Contributor

trueadm commented Nov 23, 2017

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

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 23, 2017

Member

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

Member

gaearon commented Nov 23, 2017

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

gaearon added some commits Nov 23, 2017

Only run tests directly in __tests__
This lets us share code between test files by placing them in __tests__/utils.
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.
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 23, 2017

Member

82% tests now running on bundles.

screen shot 2017-11-23 at 3 29 21 pm

Member

gaearon commented Nov 23, 2017

82% tests now running on bundles.

screen shot 2017-11-23 at 3 29 21 pm

gaearon added some commits Nov 23, 2017

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.

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

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 23, 2017

Member

2312 of 2565: we have 90% tests running!

Member

gaearon commented Nov 23, 2017

2312 of 2565: we have 90% tests running!

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 23, 2017

Member

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.

Member

gaearon commented Nov 23, 2017

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.

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

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

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@gaearon gaearon deleted the gaearon:bundle-tests branch Nov 23, 2017

HeroProtagonist added a commit to HeroProtagonist/react that referenced this pull request Nov 26, 2017

Run 90% of tests on compiled bundles (both development and production) (
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).

raphamorim added a commit to raphamorim/react that referenced this pull request Nov 27, 2017

Run 90% of tests on compiled bundles (both development and production) (
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).

Ethan-Arrowood added a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017

Run 90% of tests on compiled bundles (both development and production) (
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).
@Hoten

This comment has been minimized.

Show comment
Hide comment
@Hoten

Hoten Jan 10, 2018

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

Hoten commented Jan 10, 2018

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

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 10, 2018

Member

It enables things like #11967.

Member

gaearon commented Jan 10, 2018

It enables things like #11967.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment