Skip to content

Commit

Permalink
fix(@angular-devkit/build-angular): remove double-watch in karma
Browse files Browse the repository at this point in the history 
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

(cherry picked from commit faabbbf)
  • Loading branch information
@jkrems
jkrems committed Nov 5, 2024
1 parent a568f19 commit 43e7aae
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 33 deletions.
70 changes: 49 additions & 21 deletions packages/angular_devkit/build_angular/src/builders/karma/application_builder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import { BuildOutputFileType } from '@angular/build';
import {
ApplicationBuilderInternalOptions,
Result,
ResultFile,
ResultKind,
buildApplicationInternal,
Expand Down Expand Up @@ -42,6 +43,7 @@ class ApplicationBuildError extends Error {
function injectKarmaReporter(
context: BuilderContext,
buildOptions: BuildOptions,
buildIterator: AsyncIterator<Result>,
karmaConfig: Config & ConfigOptions,
subscriber: Subscriber<BuilderOutput>,
) {
Expand All @@ -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 (
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.`,
Expand Down Expand Up @@ -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) {
Expand All @@ -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(
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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.');
Expand Down
24 changes: 12 additions & 12 deletions packages/angular_devkit/build_angular/src/builders/karma/tests/behavior/rebuilds_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
}),
Expand Down

0 comments on commit 43e7aae

Please sign in to comment.