Skip to content

perf(aio): improve unit test rebuild time #16250

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.

Sign up for GitHub

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

Merged
petebacondarwin merged 1 commit into from
Apr 24, 2017
Merged

perf(aio): improve unit test rebuild time #16250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion aio/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"start": "yarn check-env && ng serve",
"build": "yarn check-env && yarn setup && ng build -prod -sm",
"lint": "yarn check-env && yarn docs-lint && ng lint",
"test": "yarn check-env && ng test --sourcemap=false",
"test": "yarn check-env && ng test",
"pree2e": "yarn ~~update-webdriver",
"e2e": "yarn check-env && ng e2e --no-webdriver-update",
"setup": "yarn && yarn boilerplate:add && yarn docs && yarn generate-plunkers && yarn generate-zips",
Expand Down
4 changes: 4 additions & 0 deletions aio/src/test-specs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// Find all the tests.
const context = (<any>require).context('./', true, /\.spec\.ts$/);
// And load the modules.
context.keys().map(context);
29 changes: 23 additions & 6 deletions aio/src/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// This file is required by karma.conf.js and loads recursively all the .spec and framework files

// Test dependencies.
import 'zone.js/dist/long-stack-trace-zone';
import 'zone.js/dist/proxy.js';
import 'zone.js/dist/sync-test';
Expand All @@ -12,9 +13,25 @@ import {
platformBrowserDynamicTesting
} from '@angular/platform-browser-dynamic/testing';

// List vendors here to increase test rebuild performance.
import '@angular/common';
import '@angular/common/testing';
import '@angular/core/';
import '@angular/core/testing';
import '@angular/platform-browser';
import '@angular/platform-browser/testing';
import '@angular/platform-browser/animations';
import '@angular/platform-browser-dynamic';
import '@angular/platform-browser-dynamic/testing';
import '@angular/http';
import '@angular/http/testing';
import '@angular/animations';
import '@angular/material';
import '@angular/service-worker';
import 'rxjs'; // tslint:disable-line

// Unfortunately there's no typing for the `__karma__` variable. Just declare it as any.
declare var __karma__: any;
declare var require: any;

// Prevent Karma from running prematurely.
__karma__.loaded = function () {};
Expand All @@ -24,9 +41,9 @@ getTestBed().initTestEnvironment(
BrowserDynamicTestingModule,
platformBrowserDynamicTesting()
);

declare var System: any;
// Then we find all the tests.
const context = require.context('./', true, /\.spec\.ts$/);
// And load the modules.
context.keys().map(context);
// Finally, start Karma to run the tests.
__karma__.start();
System.import('./test-specs.ts')

@gkalpak gkalpak Apr 21, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OOC, why does this help with rebuild times?

@wardbell wardbell Apr 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gkalpak In a side conversation with the PR author he said

it bundles each spec separately so it should provide better performance by only rebuilding the specs that really need it.

And so it does.

The trick of gathering the spec files in a separate file (test-specs.ts) and then importing that here in test.ts seems weird but is essential magic.

@filipesilva filipesilva Apr 21, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what happens in webpack is that rebuilding chunks is fast in and of itself. But if anything changes on a given chunk, sourcemaps are rebuilt and that takes a long time.

This is why often builds stay on 92% optimizing assets and why turning off sourcemaps makes it faster.

What this tweak does is force webpack to make a separate, lazy chunk for all the spec files and dependencies. But we also list dependencies manually, so they don't get duplicated. This way, when sourcemaps are recalculated, it's super fast because it's not doing the vendor libs.

We do this another way in normal builds, but karma webpack does not support the other approach (using CommonsChunkPlugin, codymikol/karma-webpack#22).

@gkalpak gkalpak Apr 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But sourcemaps are already disabled in yarn test, so this shouldn't be the reason for slowness. Does this change mean that we could re-enable sourcemaps in yarn test with minimal overhead?

@filipesilva filipesilva Apr 22, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't noticed they were turned off there. You're right, they can now be turned on with little to no overhead.

@filipesilva filipesilva Apr 22, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amended the PR to also remove the disabled sourcemaps.

// Finally, start Karma to run the tests.
.then(() => __karma__.start());
1 change: 1 addition & 0 deletions aio/src/tsconfig.app.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"exclude": [
"testing/**/*",
"test.ts",
"test-specs.ts",
"**/*.spec.ts"
]
}