From 43e7aae2284ff15e0282c9d9597c4f31cf1f60a4 Mon Sep 17 00:00:00 2001 From: Jan Krems Date: Tue, 5 Nov 2024 08:35:18 -0800 Subject: [PATCH] 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 https://github.com/angular/angular-cli/issues/28755 (cherry picked from commit faabbbf910f1eabc881eb910ec7d8295c574952d) --- .../src/builders/karma/application_builder.ts | 70 +++++++++++++------ .../karma/tests/behavior/rebuilds_spec.ts | 24 +++---- 2 files changed, 61 insertions(+), 33 deletions(-) diff --git a/packages/angular_devkit/build_angular/src/builders/karma/application_builder.ts b/packages/angular_devkit/build_angular/src/builders/karma/application_builder.ts index f82a3b0da1b8..551589fa9c5c 100644 --- a/packages/angular_devkit/build_angular/src/builders/karma/application_builder.ts +++ b/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, karmaConfig: Config & ConfigOptions, subscriber: Subscriber, ) { @@ -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 { return from(initializeApplication(options, context, karmaOptions, transforms)).pipe( switchMap( - ([karma, karmaConfig, buildOptions]) => + ([karma, karmaConfig, buildOptions, buildIterator]) => new Observable((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; karmaOptions?: (options: ConfigOptions) => ConfigOptions; } = {}, -): Promise<[typeof import('karma'), Config & ConfigOptions, BuildOptions]> { +): Promise< + [typeof import('karma'), Config & ConfigOptions, BuildOptions, AsyncIterator | 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): boolean { @@ -364,9 +379,22 @@ export async function writeTestFiles(files: Record, testDir: } /** Returns the first item yielded by the given generator and cancels the execution. */ -async function first(generator: AsyncIterable): Promise { +async function first( + generator: AsyncIterable, + { cancel }: { cancel: boolean }, +): Promise<[T, AsyncIterator | null]> { + if (!cancel) { + const iterator: AsyncIterator = 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.'); diff --git a/packages/angular_devkit/build_angular/src/builders/karma/tests/behavior/rebuilds_spec.ts b/packages/angular_devkit/build_angular/src/builders/karma/tests/behavior/rebuilds_spec.ts index a89f2dbd7c7a..e740b7adfcd6 100644 --- a/packages/angular_devkit/build_angular/src/builders/karma/tests/behavior/rebuilds_spec.ts +++ b/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); }),