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

Separate apps tests into unit and integration tests #8504

Merged
merged 37 commits into from May 19, 2016

Conversation

Projects
None yet
2 participants
@islemaster
Member

islemaster commented May 18, 2016

Re-opening #8467 against staging-next.

Breaks apps tests into two tests suites: Unit tests (faster, fewer dependencies, less setup/teardown) and integration tests (slower, more dependencies, more setup/teardown). Lots of things change here; I'll try to break down the changes so they're easy to understand.

Changes

Organization

The whole apps/test folder has been rearranged. Here's the new layout:

apps/
  test/
    audio/              # Our audio test page and its assets
    integration/        # Integration tests
    unit/               # Unit tests
    util/               # Utilities common to both tests
    package.json
    runIntegrationTests.js
    runUnitTests.js

As you've probably guessed, the unit tests are in test/unit and the integration tests are in test/integration. Lots of supporting materials have been moved too.

Unit tests treat every file in the unit folder as a test (i.e. test/unit/**/*.js).

Integration tests treat every file in the root of the integration folder as a test (i.e. test/integration/*.js). Integration tests have a lot of supporting materials (assets, levelSolutions, etc) which are in subfolders in the integration directory.

Interface

There are lots of ways to invoke apps tests, and they've changed at most layers. I'll go through the layers one-by-one to explain the changes.

npm scripts

npm test will still lint apps and run all of the apps tests. It now runs in this order: lint, unit, integration.

There are two new commands npm run test:unit and npm run test:integration that will skip linting and run the named test suite alone.

grunt tasks

unitTest and integrationTest are new grunt tasks that do necessary setup and run the named test suite. The only notable difference in setup at this layer is that unitTest does not have to run the newer:copy:static step, which saves a few seconds. grunt test is unchanged, but under the hood it now runs grunt unitTest integrationTest.

The grunt mochaTest task has been deprecated, but not yet removed. Running it will print a deprecation warning and then run grunt test

Internally grunt exec:mochaTest has been replaced by grunt exec:unitTest and grunt exec:integrationTest.

These tasks accept all the same arguments as before. One improvement - the --entry flag now works even when you forget to prefix your entry file with ./.

test scripts

The apps/test/util/runTests.js script has been removed and replaced by apps/test/runUnitTests.js and apps/test/runIntegrationTests.js. These scripts have the same interface and accept the same options as before, but note that the --fast flag is meaningless to the unit test suite.

Separate utilities

I took a lot of code out of testUtils.js. Some of it moved into utility files specifically for the integration tests, and some of it went to other, more specific utility files (see configuredChai.js) and some of it just evaporated because tests could include what they needed directly.

This is useful because we were introducing lots of dependencies to tests that didn't need them just by requiring testUtils.js. As a general practice moving forward, if you need a utility around a particular dependency (like Chai) it should become its own utility file so we avoid lumping dependencies together this way - that will help keep the bundle time down for unit tests.

Test speed

Unfortunately, this makes running all of our apps tests more than 20 seconds slower. This is largely due to building two test bundles instead of one.

Baseline (grunt test):
  Execution Time (2016-05-13 17:06:06 UTC)
  exec:mochaTest  3m 26.3s  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 99%
  Total 3m 28.6s

New time (grunt test)
  Execution Time (2016-05-17 22:58:21 UTC)
  messages:all              2.8s  ▇ 1%
  exec:unitTest          1m 1.1s  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 27%
  exec:integrationTest  2m 44.4s  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 71%
  Total 3m 50.7s

I think that slowdown will be more than compensated for by the iteration speedup when working on unit tests. Building and running the unit tests by themselves now takes about a minute, and when using the --entry option on unit tests being done in 4-5 seconds is common.

There's also a small advantage to running all of the unit tests before all of the integration tests: Assuming we break tests with a fairly normal distribution, we'll learn about breakages faster on average if we run the faster tests first. It might seem like a small thing, but CircleCI failing faster may pay off in the long run.

The real breakthrough on the speed of running tests, though, is going to be incremental builds for the test bundles. That's future work. I'm hoping this big change will make it easier to get that working.

Running tests in CI

We already run apps tests in two chunks on CircleCI. Before, it was non-applab tests followed by applab tests. We did this as a workaround to a problem where we were running out of memory during our unit tests.

Here, we replace "non-applab" and "applab" with "unit" and "integration." Tests are passing on CircleCI and memory use doesn't seem to be a problem - and anyway, we have more than twice as many unit tests as integration tests, and now they load a lot less stuff. We still run them in order, and do up to one retry on the individual tasks.

I think this is valuable from a maintenance point of view, just because it's conceptually a simpler divide. It's also a lot closer to the way we usually run tests in development.

islemaster added some commits May 13, 2016

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster May 18, 2016

Member

I'll put @Bjvanminnen on point for this review, but I'd appreciate eyes from @pcardune and @davidsbailey as well. Thanks everyone!

Member

islemaster commented May 18, 2016

I'll put @Bjvanminnen on point for this review, but I'd appreciate eyes from @pcardune and @davidsbailey as well. Thanks everyone!

@@ -1,5 +1,6 @@
/** @file App Lab-specific Crosshair Overlay */
import React from 'react';

This comment has been minimized.

@Bjvanminnen

Bjvanminnen May 19, 2016

Contributor

Why does this test change result in us having to start importing React in our product code?

@Bjvanminnen

Bjvanminnen May 19, 2016

Contributor

Why does this test change result in us having to start importing React in our product code?

This comment has been minimized.

@islemaster

islemaster May 19, 2016

Member

After moving the unit tests into the test/unit folder and loading test/unit/*/.js as tests, I believe the run order for the unit tests subtly changed. The unit tests that shallow-render this overlay started failing because this component was unable to find React when it was loaded. For some reason, our setExternalGlobals() helper was not fixing the problem.

I'm suspicious that the way module loading works in our transpiled ES6 has something to do with this, but I can't prove it.

We browserify-global-shim React in our apps build as of #7181 (this means any time we require/import 'react' we'll retrieve window.React instead of loading a module). Conveniently, we don't global-shim in tests so this actually loads React, removing the need to set up globals. I propose that, now that we have it shimmed, we should switch to loading React this way everywhere instead of treating it as a global (even though it is provided as one in our real build) - I think @wjordan pointed out that we should be doing this a long time ago, and @pcardune set it up to support the app lab bundle (which also does not shim React).

That said, I'm happy to spend some more time trying to wrangle the globals here if you don't want to introduce this change now.

@islemaster

islemaster May 19, 2016

Member

After moving the unit tests into the test/unit folder and loading test/unit/*/.js as tests, I believe the run order for the unit tests subtly changed. The unit tests that shallow-render this overlay started failing because this component was unable to find React when it was loaded. For some reason, our setExternalGlobals() helper was not fixing the problem.

I'm suspicious that the way module loading works in our transpiled ES6 has something to do with this, but I can't prove it.

We browserify-global-shim React in our apps build as of #7181 (this means any time we require/import 'react' we'll retrieve window.React instead of loading a module). Conveniently, we don't global-shim in tests so this actually loads React, removing the need to set up globals. I propose that, now that we have it shimmed, we should switch to loading React this way everywhere instead of treating it as a global (even though it is provided as one in our real build) - I think @wjordan pointed out that we should be doing this a long time ago, and @pcardune set it up to support the app lab bundle (which also does not shim React).

That said, I'm happy to spend some more time trying to wrangle the globals here if you don't want to introduce this change now.

This comment has been minimized.

@Bjvanminnen

Bjvanminnen May 19, 2016

Contributor

I agree with your proposal. Presumably a related step is to tell our linter to stop assuming React exists as a global (such that you need to import React to use it if you dont want linter warnings).

@Bjvanminnen

Bjvanminnen May 19, 2016

Contributor

I agree with your proposal. Presumably a related step is to tell our linter to stop assuming React exists as a global (such that you need to import React to use it if you dont want linter warnings).

This comment has been minimized.

@islemaster

islemaster May 19, 2016

Member

Yep. I'd like to do a followup PR that global-shims all over the place and sets up our linting to enforce it.

@islemaster

islemaster May 19, 2016

Member

Yep. I'd like to do a followup PR that global-shims all over the place and sets up our linting to enforce it.

@Bjvanminnen

This comment has been minimized.

Show comment
Hide comment
@Bjvanminnen

Bjvanminnen May 19, 2016

Contributor

lgtm. thanks for doing this brad!

Contributor

Bjvanminnen commented May 19, 2016

lgtm. thanks for doing this brad!

@islemaster islemaster merged commit b8a14db into staging-next May 19, 2016

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 88.261%
Details
hound No violations found. Woof!

@islemaster islemaster deleted the separate-integration-tests branch May 19, 2016

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