Stop karma gracefully, test watch mode, and check syntactic diagnostics on rebuilds

package.json

@@ -103,6 +103,7 @@
     "istanbul": "^0.4.5",
     "jasmine": "^2.6.0",
     "jasmine-spec-reporter": "^3.2.0",
+    "karma": "~3.1.1",
     "karma-jasmine-html-reporter": "^0.2.2",
     "license-checker": "^20.1.0",
     "minimatch": "^3.0.4",

packages/angular_devkit/build_angular/package.json

@@ -77,7 +77,7 @@
     "jquery": "^3.3.1",
     "jasmine-core": "~2.8.0",
     "jasmine-spec-reporter": "~4.2.1",
-    "karma": "~3.0.0",
+    "karma": "~3.1.1",
     "karma-chrome-launcher": "~2.2.0",
     "karma-coverage-istanbul-reporter": "~2.0.0",
     "karma-jasmine": "~1.1.0",

packages/angular_devkit/build_angular/src/angular-cli-files/plugins/karma.ts

@@ -9,13 +9,15 @@
 // TODO: cleanup this file, it's copied as is from Angular CLI.
 
 import * as path from 'path';
-import * as fs from 'fs';
 import * as glob from 'glob';
 import * as webpack from 'webpack';
 const webpackDevMiddleware = require('webpack-dev-middleware');
 
-import { AssetPattern } from '../../browser/schema';
 import { KarmaWebpackFailureCb } from './karma-webpack-failure-cb';
+import { statsErrorsToString } from '../utilities/stats';
+import { getWebpackStatsConfig } from '../models/webpack-configs/stats';
+import { createConsoleLogger } from '@angular-devkit/core/node';
+import { logging } from '@angular-devkit/core';
 
 /**
  * Enumerate needed (but not require/imported) dependencies from this file
@@ -62,7 +64,7 @@ const init: any = (config: any, emitter: any, customFileHandlers: any) => {
     )
   }
   const options = config.buildWebpack.options;
-  const projectRoot = config.buildWebpack.projectRoot as string;
+  const logger: logging.Logger = config.buildWebpack.logger || createConsoleLogger();
   successCb = config.buildWebpack.successCb;
   failureCb = config.buildWebpack.failureCb;
 
@@ -93,7 +95,9 @@ const init: any = (config: any, emitter: any, customFileHandlers: any) => {
   // Add webpack config.
   const webpackConfig = config.buildWebpack.webpackConfig;
   const webpackMiddlewareConfig = {
-    logLevel: 'error', // Hide webpack output because its noisy.
+    // Hide webpack output because its noisy.
+    logLevel: 'error',
+    stats: false,
     watchOptions: { poll: options.poll },
     publicPath: '/_karma_webpack_/',
   };
@@ -147,9 +151,9 @@ const init: any = (config: any, emitter: any, customFileHandlers: any) => {
   try {
     compiler = webpack(webpackConfig);
   } catch (e) {
-    console.error(e.stack || e);
+    logger.error(e.stack || e)
     if (e.details) {
-      console.error(e.details);
+      logger.error(e.details)
     }
     throw e;
   }
@@ -175,9 +179,17 @@ const init: any = (config: any, emitter: any, customFileHandlers: any) => {
   }
 
   let lastCompilationHash: string | undefined;
+  const statsConfig = getWebpackStatsConfig();
   compiler.hooks.done.tap('karma', (stats: any) => {
-    // Refresh karma only when there are no webpack errors, and if the compilation changed.
-    if (stats.compilation.errors.length === 0 && stats.hash != lastCompilationHash) {
+    if (stats.compilation.errors.length > 0) {
+      const json = stats.toJson(config.stats);
+      // Print compilation errors.
+      logger.error(statsErrorsToString(json, statsConfig));
+      lastCompilationHash = undefined;
+      // Emit a failure build event if there are compilation errors.
+      failureCb && failureCb();
+    } else if (stats.hash != lastCompilationHash) {
+      // Refresh karma only when there are no webpack errors, and if the compilation changed.
       lastCompilationHash = stats.hash;
       emitter.refreshFiles();
     }

packages/angular_devkit/build_angular/src/karma/index.ts

@@ -97,6 +97,7 @@ export class KarmaBuilder implements Builder<KarmaBuilderSchema> {
           // When this workaround is removed, user projects need to be updated to use a Karma
           // version that has a fix for this issue.
           toJSON: () => { },
+          logger: this.context.logger,
         };
 
         // TODO: inside the configs, always use the project root and not the workspace root.
@@ -108,15 +109,14 @@ export class KarmaBuilder implements Builder<KarmaBuilderSchema> {
 
         // Complete the observable once the Karma server returns.
         const karmaServer = new karma.Server(karmaOptions, () => obs.complete());
-        karmaServer.start();
+        const karmaStartPromise = karmaServer.start();
 
         // Cleanup, signal Karma to exit.
         return () => {
-          // Karma does not seem to have a way to exit the server gracefully.
-          // See https://github.com/karma-runner/karma/issues/2867#issuecomment-369912167
-          // TODO: make a PR for karma to add `karmaServer.close(code)`, that
-          // calls `disconnectBrowsers(code);`
-          // karmaServer.close();
+          // Karma only has the `stop` method start with 3.1.1, so we must defensively check.
+          if (karmaServer.stop && typeof karmaServer.stop === 'function') {
+            return karmaStartPromise.then(() => karmaServer.stop());
+          }
         };
       })),
     );

packages/angular_devkit/build_angular/test/browser/rebuild_spec_large.ts

@@ -233,7 +233,7 @@ describe('Browser Builder rebuilds', () => {
     // Add a major static analysis error on a non-main file to the initial build.
     host.replaceInFile('./src/app/app.component.ts', `'app-root'`, `(() => 'app-root')()`);
 
-    const overrides = { watch: true, aot: true, forkTypeChecker: false };
+    const overrides = { watch: true, aot: true };
     const logger = new TestLogger('rebuild-aot-errors');
     const staticAnalysisError = 'Function expressions are not supported in decorators';
     const syntaxError = 'Declaration or statement expected.';
@@ -298,7 +298,7 @@ describe('Browser Builder rebuilds', () => {
       'src/app/imported-styles.css': 'p {color: #f00;}',
     });
 
-    const overrides = { watch: true, aot: true, forkTypeChecker: false };
+    const overrides = { watch: true, aot: true };
     let buildNumber = 0;
 
     runTargetSpec(host, browserTargetSpec, overrides, DefaultTimeout * 3).pipe(

packages/angular_devkit/build_angular/test/karma/rebuilds_spec_large.ts

@@ -6,68 +6,102 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import { runTargetSpec } from '@angular-devkit/architect/testing';
-import { debounceTime, take, tap } from 'rxjs/operators';
+import { DefaultTimeout, runTargetSpec } from '@angular-devkit/architect/testing';
+import { Subject } from 'rxjs';
+import { debounceTime, delay, take, takeUntil, takeWhile, tap } from 'rxjs/operators';
 import { host, karmaTargetSpec } from '../utils';
 
 
-// Karma watch mode is currently bugged:
-// - errors print a huge stack trace
-// - karma does not have a way to close the server
-//   gracefully (https://github.com/karma-runner/karma/issues/3149)
-// TODO: fix these before 6.0 final.
-xdescribe('Karma Builder watch mode', () => {
+describe('Karma Builder watch mode', () => {
   beforeEach(done => host.initialize().toPromise().then(done, done.fail));
   afterEach(done => host.restore().toPromise().then(done, done.fail));
 
-  it('works', (done) => {
+  it('works', async () => {
     const overrides = { watch: true };
-    runTargetSpec(host, karmaTargetSpec, overrides).pipe(
+    const res = await runTargetSpec(host, karmaTargetSpec, overrides).pipe(
       debounceTime(500),
       tap((buildEvent) => expect(buildEvent.success).toBe(true)),
       take(1),
-    ).toPromise().then(done, done.fail);
-  }, 30000);
+    ).toPromise();
+
+    expect(res).toEqual({ success: true });
+  });
 
   it('recovers from compilation failures in watch mode', (done) => {
     const overrides = { watch: true };
-    let buildNumber = 0;
+    let buildCount = 0;
+    let phase = 1;
 
-    runTargetSpec(host, karmaTargetSpec, overrides).pipe(
+    runTargetSpec(host, karmaTargetSpec, overrides, DefaultTimeout * 3).pipe(
       debounceTime(500),
       tap((buildEvent) => {
-        buildNumber += 1;
-        switch (buildNumber) {
+        buildCount += 1;
+        switch (phase) {
           case 1:
             // Karma run should succeed.
             // Add a compilation error.
             expect(buildEvent.success).toBe(true);
-            host.writeMultipleFiles({
-              'src/app/app.component.spec.ts': '<p> definitely not typescript </p>',
-            });
+            // Add an syntax error to a non-main file.
+            host.appendToFile('src/app/app.component.spec.ts', `]]]`);
+            phase = 2;
             break;
 
           case 2:
             // Karma run should fail due to compilation error. Fix it.
             expect(buildEvent.success).toBe(false);
-            host.writeMultipleFiles({ 'src/foo.spec.ts': '' });
+            host.replaceInFile('src/app/app.component.spec.ts', `]]]`, '');
+            phase = 3;
             break;
 
           case 3:
             // Karma run should succeed again.
             expect(buildEvent.success).toBe(true);
-            break;
-
-          default:
+            phase = 4;
             break;
         }
       }),
-      take(3),
-    ).toPromise().then(done, done.fail);
-  }, 30000);
+      takeWhile(() => phase < 4),
+    ).toPromise().then(
+      () => done(),
+      () => done.fail(`stuck at phase ${phase} [builds: ${buildCount}]`),
+    );
+  });
 
   it('does not rebuild when nothing changed', (done) => {
-    // Start the server in watch mode, wait for the first build to finish, touch
-    // test.js without changing it, wait 5s then exit unsuscribe, verify only one event was emitted.
-  }, 30000);
+    const overrides = { watch: true };
+    let buildCount = 0;
+    let phase = 1;
+
+    const stopSubject = new Subject();
+    const stop$ = stopSubject.asObservable().pipe(delay(5000));
+
+    runTargetSpec(host, karmaTargetSpec, overrides, DefaultTimeout * 3).pipe(
+      debounceTime(500),
+      tap((buildEvent) => {
+        buildCount += 1;
+        switch (phase) {
+          case 1:
+            // Karma run should succeed.
+            // Add a compilation error.
+            expect(buildEvent.success).toBe(true);
+            // Touch the file.
+            host.appendToFile('src/app/app.component.spec.ts', ``);
+            // Signal the stopper, which delays emission by 5s.
+            // If there's no rebuild within that time then the test is successful.
+            stopSubject.next();
+            phase = 2;
+            break;
+
+          case 2:
+            // Should never trigger this second build.
+            expect(true).toBeFalsy('Should not trigger second build.');
+            break;
+        }
+      }),
+      takeUntil(stop$),
+    ).toPromise().then(
+      () => done(),
+      () => done.fail(`stuck at phase ${phase} [builds: ${buildCount}]`),
+    );
+  });
 });

packages/ngtools/webpack/src/angular_compiler_plugin.ts

@@ -39,7 +39,7 @@ import { Compiler, compilation } from 'webpack';
 import { time, timeEnd } from './benchmark';
 import { WebpackCompilerHost, workaroundResolve } from './compiler_host';
 import { resolveEntryModuleFromMain } from './entry_resolver';
-import { gatherDiagnostics, hasErrors } from './gather_diagnostics';
+import { DiagnosticMode, gatherDiagnostics, hasErrors } from './gather_diagnostics';
 import { TypeScriptPathsPlugin } from './paths-plugin';
 import { WebpackResourceLoader } from './resource_loader';
 import {
@@ -284,6 +284,7 @@ export class AngularCompilerPlugin {
     if (options.forkTypeChecker !== undefined) {
       this._forkTypeChecker = options.forkTypeChecker;
     }
+    // this._forkTypeChecker = false;
 
     // Add custom platform transformers.
     if (options.platformTransformers !== undefined) {
@@ -1038,6 +1039,8 @@ export class AngularCompilerPlugin {
     time('AngularCompilerPlugin._emit');
     const program = this._program;
     const allDiagnostics: Array<ts.Diagnostic | Diagnostic> = [];
+    const diagMode = (this._firstRun || !this._forkTypeChecker) ?
+      DiagnosticMode.All : DiagnosticMode.Syntactic;
 
     let emitResult: ts.EmitResult | undefined;
     try {
@@ -1051,10 +1054,8 @@ export class AngularCompilerPlugin {
           timeEnd('AngularCompilerPlugin._emit.ts.getOptionsDiagnostics');
         }
 
-        if ((this._firstRun || !this._forkTypeChecker) && this._program) {
-          allDiagnostics.push(...gatherDiagnostics(this._program, this._JitMode,
-            'AngularCompilerPlugin._emit.ts'));
-        }
+        allDiagnostics.push(...gatherDiagnostics(tsProgram, this._JitMode,
+          'AngularCompilerPlugin._emit.ts', diagMode));
 
         if (!hasErrors(allDiagnostics)) {
           if (this._firstRun || sourceFiles.length > 20) {
@@ -1098,10 +1099,8 @@ export class AngularCompilerPlugin {
           timeEnd('AngularCompilerPlugin._emit.ng.getNgOptionDiagnostics');
         }
 
-        if ((this._firstRun || !this._forkTypeChecker) && this._program) {
-          allDiagnostics.push(...gatherDiagnostics(this._program, this._JitMode,
-            'AngularCompilerPlugin._emit.ng'));
-        }
+        allDiagnostics.push(...gatherDiagnostics(angularProgram, this._JitMode,
+          'AngularCompilerPlugin._emit.ng', diagMode));
 
         if (!hasErrors(allDiagnostics)) {
           time('AngularCompilerPlugin._emit.ng.emit');