fix(@angular-devkit/build-angular): remove double-watch in karma

The Karma file watching was racing with the file writes done by the application builder. Since we already tell Karma when to reun via `.refeshFiles()`, disabling Karma's own file watcher should make things more reliable. This allows removing a weird special-case in the test case and removes the noisy "File chaned" logs generated by Karma. Fixes #28755

Author: hybrist

packages/angular_devkit/build_angular/src/builders/karma/application_builder.ts

@@ -9,6 +9,7 @@
 import { BuildOutputFileType } from '@angular/build';
 import {
  ApplicationBuilderInternalOptions,
+ Result,
  ResultFile,
  ResultKind,
  buildApplicationInternal,
@@ -42,6 +43,7 @@ class ApplicationBuildError extends Error {
 function injectKarmaReporter(
  context: BuilderContext,
  buildOptions: BuildOptions,
+ buildIterator: AsyncIterator<Result>,
  karmaConfig: Config & ConfigOptions,
  subscriber: Subscriber<BuilderOutput>,
 ) {
@@ -64,13 +66,15 @@ function injectKarmaReporter(
 
  private startWatchingBuild() {
  void (async () => {
- for await (const buildOutput of buildApplicationInternal(
- {
- ...buildOptions,
- watch: true,
- },
- context,
- )) {
+ // This is effectively "for await of but skip what's already consumed".
+ let isDone = false; // to mark the loop condition as "not constant".
+ while (!isDone) {
+ const { done, value: buildOutput } = await buildIterator.next();
+ if (done) {
+ isDone = true;
+ break;
+ }
+
  if (buildOutput.kind === ResultKind.Failure) {
  subscriber.next({ success: false, message: 'Build failed' });
  } else if (
@@ -121,12 +125,12 @@ export function execute(
 ): Observable<BuilderOutput> {
  return from(initializeApplication(options, context, karmaOptions, transforms)).pipe(
  switchMap(
- ([karma, karmaConfig, buildOptions]) =>
+ ([karma, karmaConfig, buildOptions, buildIterator]) =>
  new Observable<BuilderOutput>((subscriber) => {
  // If `--watch` is explicitly enabled or if we are keeping the Karma
  // process running, we should hook Karma into the build.
- if (options.watch ?? !karmaConfig.singleRun) {
- injectKarmaReporter(context, buildOptions, karmaConfig, subscriber);
+ if (buildIterator) {
+ injectKarmaReporter(context, buildOptions, buildIterator, karmaConfig, subscriber);
  }
 
  // Complete the observable once the Karma server returns.
@@ -199,7 +203,9 @@ async function initializeApplication(
  webpackConfiguration?: ExecutionTransformer<Configuration>;
  karmaOptions?: (options: ConfigOptions) => ConfigOptions;
  } = {},
-): Promise<[typeof import('karma'), Config & ConfigOptions, BuildOptions]> {
+): Promise<
+ [typeof import('karma'), Config & ConfigOptions, BuildOptions, AsyncIterator<Result> | null]
+> {
  if (transforms.webpackConfiguration) {
  context.logger.warn(
  `This build is using the application builder but transforms.webpackConfiguration was provided. The transform will be ignored.`,
@@ -247,10 +253,14 @@ async function initializeApplication(
  styles: options.styles,
  polyfills: normalizePolyfills(options.polyfills),
  webWorkerTsConfig: options.webWorkerTsConfig,
+ watch: options.watch ?? !karmaOptions.singleRun,
  };
 
  // Build tests with `application` builder, using test files as entry points.
- const buildOutput = await first(buildApplicationInternal(buildOptions, context));
+ const [buildOutput, buildIterator] = await first(
+ buildApplicationInternal(buildOptions, context),
+ { cancel: !buildOptions.watch },
+ );
  if (buildOutput.kind === ResultKind.Failure) {
  throw new ApplicationBuildError('Build failed');
  } else if (buildOutput.kind !== ResultKind.Full) {
@@ -265,28 +275,33 @@ async function initializeApplication(
  karmaOptions.files ??= [];
  karmaOptions.files.push(
  // Serve polyfills first.
- { pattern: `${outputPath}/polyfills.js`, type: 'module' },
+ { pattern: `${outputPath}/polyfills.js`, type: 'module', watched: false },
  // Serve global setup script.
- { pattern: `${outputPath}/${mainName}.js`, type: 'module' },
+ { pattern: `${outputPath}/${mainName}.js`, type: 'module', watched: false },
  // Serve all source maps.
- { pattern: `${outputPath}/*.map`, included: false },
+ { pattern: `${outputPath}/*.map`, included: false, watched: false },
  );
 
  if (hasChunkOrWorkerFiles(buildOutput.files)) {
  karmaOptions.files.push(
  // Allow loading of chunk-* files but don't include them all on load.
- { pattern: `${outputPath}/{chunk,worker}-*.js`, type: 'module', included: false },
+ {
+ pattern: `${outputPath}/{chunk,worker}-*.js`,
+ type: 'module',
+ included: false,
+ watched: false,
+ },
  );
  }
 
  karmaOptions.files.push(
  // Serve remaining JS on page load, these are the test entrypoints.
- { pattern: `${outputPath}/*.js`, type: 'module' },
+ { pattern: `${outputPath}/*.js`, type: 'module', watched: false },
  );
 
  if (options.styles?.length) {
  // Serve CSS outputs on page load, these are the global styles.
- karmaOptions.files.push({ pattern: `${outputPath}/*.css`, type: 'css' });
+ karmaOptions.files.push({ pattern: `${outputPath}/*.css`, type: 'css', watched: false });
  }
 
  const parsedKarmaConfig: Config & ConfigOptions = await karma.config.parseConfig(
@@ -327,7 +342,7 @@ async function initializeApplication(
  parsedKarmaConfig.reporters = (parsedKarmaConfig.reporters ?? []).concat(['coverage']);
  }
 
- return [karma, parsedKarmaConfig, buildOptions];
+ return [karma, parsedKarmaConfig, buildOptions, buildIterator];
 }
 
 function hasChunkOrWorkerFiles(files: Record<string, unknown>): boolean {
@@ -364,9 +379,22 @@ export async function writeTestFiles(files: Record<string, ResultFile>, testDir:
 }
 
 /** Returns the first item yielded by the given generator and cancels the execution. */
-async function first<T>(generator: AsyncIterable<T>): Promise<T> {
+async function first<T>(
+ generator: AsyncIterable<T>,
+ { cancel }: { cancel: boolean },
+): Promise<[T, AsyncIterator<T> | null]> {
+ if (!cancel) {
+ const iterator: AsyncIterator<T> = generator[Symbol.asyncIterator]();
+ const firstValue = await iterator.next();
+ if (firstValue.done) {
+ throw new Error('Expected generator to emit at least once.');
+ }
+
+ return [firstValue.value, iterator];
+ }
+
  for await (const value of generator) {
- return value;
+ return [value, null];
  }
 
  throw new Error('Expected generator to emit at least once.');

packages/angular_devkit/build_angular/src/builders/karma/tests/behavior/rebuilds_spec.ts

@@ -6,12 +6,12 @@
  * found in the LICENSE file at https://angular.dev/license
  */
 
-import { concatMap, count, debounceTime, take, timeout } from 'rxjs';
+import { concatMap, count, debounceTime, distinctUntilChanged, take, timeout } from 'rxjs';
 import { execute } from '../../index';
 import { BASE_OPTIONS, KARMA_BUILDER_INFO, describeKarmaBuilder } from '../setup';
 import { BuilderOutput } from '@angular-devkit/architect';
 
-describeKarmaBuilder(execute, KARMA_BUILDER_INFO, (harness, setupTarget, isApplicationBuilder) => {
+describeKarmaBuilder(execute, KARMA_BUILDER_INFO, (harness, setupTarget) => {
  describe('Behavior: "Rebuilds"', () => {
  beforeEach(async () => {
  await setupTarget(harness);
@@ -33,31 +33,31 @@ describeKarmaBuilder(execute, KARMA_BUILDER_INFO, (harness, setupTarget, isAppli
  async (result) => {
  // Karma run should succeed.
  // Add a compilation error.
- expect(result?.success).toBeTrue();
+ expect(result?.success).withContext('Initial test run should succeed').toBeTrue();
  // Add an syntax error to a non-main file.
  await harness.appendToFile('src/app/app.component.spec.ts', `error`);
  },
  async (result) => {
- expect(result?.success).toBeFalse();
+ expect(result?.success)
+ .withContext('Test should fail after build error was introduced')
+ .toBeFalse();
  await harness.writeFile('src/app/app.component.spec.ts', goodFile);
  },
  async (result) => {
- expect(result?.success).toBeTrue();
+ expect(result?.success)
+ .withContext('Test should succeed again after build error was fixed')
+ .toBeTrue();
  },
  ];
- if (isApplicationBuilder) {
- expectedSequence.unshift(async (result) => {
- // This is the initial Karma run, it should succeed.
- // For simplicity, we trigger a run the first time we build in watch mode.
- expect(result?.success).toBeTrue();
- });
- }
 
  const buildCount = await harness
  .execute({ outputLogsOnFailure: false })
  .pipe(
  timeout(60000),
  debounceTime(500),
+ // There may be a sequence of {success:true} events that should be
+ // de-duplicated.
+ distinctUntilChanged((prev, current) => prev.result?.success === current.result?.success),
  concatMap(async ({ result }, index) => {
  await expectedSequence[index](result);
  }),