Skip to content

refactor(@angular-devkit/build-angular): simplify how specs are found and loaded #23939

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.

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
merged 1 commit into from
Sep 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,73 @@
* found in the LICENSE file at https://angular.io/license
*/

import assert from 'assert';
import { PathLike, constants, promises as fs } from 'fs';
import glob, { hasMagic } from 'glob';
import { basename, dirname, extname, join, relative } from 'path';
import { promisify } from 'util';
import type { Compilation, Compiler } from 'webpack';
import { addError } from '../../utils/webpack-diagnostics';

const globPromise = promisify(glob);

/**
* The name of the plugin provided to Webpack when tapping Webpack compiler hooks.
*/
const PLUGIN_NAME = 'angular-find-tests-plugin';

export interface FindTestsPluginOptions {
include?: string[];
workspaceRoot: string;
projectSourceRoot: string;
}

export class FindTestsPlugin {
private compilation: Compilation | undefined;

constructor(private options: FindTestsPluginOptions) {}

apply(compiler: Compiler): void {
const { include = ['**/*.spec.ts'], projectSourceRoot, workspaceRoot } = this.options;
const webpackOptions = compiler.options;
const entry =
typeof webpackOptions.entry === 'function' ? webpackOptions.entry() : webpackOptions.entry;

let originalImport: string[] | undefined;

// Add tests files are part of the entry-point.
webpackOptions.entry = async () => {
const specFiles = await findTests(include, workspaceRoot, projectSourceRoot);

if (!specFiles.length) {
assert(this.compilation, 'Compilation cannot be undefined.');
addError(
this.compilation,
`Specified patterns: "${include.join(', ')}" did not match any spec files.`,
);
}

const entrypoints = await entry;
const entrypoint = entrypoints['main'];
if (!entrypoint.import) {
throw new Error(`Cannot find 'main' entrypoint.`);
}

originalImport ??= entrypoint.import;
entrypoint.import = [...originalImport, ...specFiles];

return entrypoints;
};

compiler.hooks.thisCompilation.tap(PLUGIN_NAME, (compilation) => {
this.compilation = compilation;
compilation.contextDependencies.add(projectSourceRoot);
});
}
}

// go through all patterns and find unique list of files
export async function findTests(
async function findTests(
patterns: string[],
workspaceRoot: string,
projectSourceRoot: string,
Expand All @@ -37,6 +95,10 @@ async function findMatchingTests(
): Promise<string[]> {
// normalize pattern, glob lib only accepts forward slashes
let normalizedPattern = normalizePath(pattern);
if (normalizedPattern.charAt(0) === '/') {
normalizedPattern = normalizedPattern.substring(1);
}

const relativeProjectRoot = normalizePath(relative(workspaceRoot, projectSourceRoot) + '/');

// remove relativeProjectRoot to support relative paths from root
Expand All @@ -54,12 +116,13 @@ async function findMatchingTests(
const fileExt = extname(normalizedPattern);
// Replace extension to `.spec.ext`. Example: `src/app/app.component.ts`-> `src/app/app.component.spec.ts`
const potentialSpec = join(
projectSourceRoot,
dirname(normalizedPattern),
`${basename(normalizedPattern, fileExt)}.spec${fileExt}`,
);

if (await exists(join(projectSourceRoot, potentialSpec))) {
return [normalizePath(potentialSpec)];
if (await exists(potentialSpec)) {
return [potentialSpec];
}
}
}
Expand All @@ -68,6 +131,7 @@ async function findMatchingTests(
cwd: projectSourceRoot,
root: projectSourceRoot,
nomount: true,
absolute: true,
});
}

Expand Down
59 changes: 16 additions & 43 deletions packages/angular_devkit/build_angular/src/builders/karma/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,8 @@ import { purgeStaleBuildCache } from '../../utils/purge-cache';
import { assertCompatibleAngularVersion } from '../../utils/version';
import { generateBrowserWebpackConfigFromContext } from '../../utils/webpack-browser-config';
import { getCommonConfig, getStylesConfig } from '../../webpack/configs';
import { SingleTestTransformLoader } from '../../webpack/plugins/single-test-transform';
import { Schema as BrowserBuilderOptions, OutputHashing } from '../browser/schema';
import { findTests } from './find-tests';
import { FindTestsPlugin } from './find-tests-plugin';
import { Schema as KarmaBuilderOptions } from './schema';

export type KarmaConfigOptions = ConfigOptions & {
Expand Down Expand Up @@ -62,10 +61,7 @@ async function initialize(

const karma = await import('karma');

return [
karma,
webpackConfigurationTransformer ? await webpackConfigurationTransformer(config) : config,
];
return [karma, (await webpackConfigurationTransformer?.(config)) ?? config];
}

/**
Expand Down Expand Up @@ -110,45 +106,22 @@ export function execute(
}
}

// prepend special webpack loader that will transform test.ts
if (options.include?.length) {
const projectName = context.target?.project;
if (!projectName) {
throw new Error('The builder requires a target.');
}
const projectName = context.target?.project;
if (!projectName) {
throw new Error('The builder requires a target.');
}

const projectMetadata = await context.getProjectMetadata(projectName);
const sourceRoot = (projectMetadata.sourceRoot ?? projectMetadata.root ?? '') as string;
const projectSourceRoot = path.join(context.workspaceRoot, sourceRoot);
const projectMetadata = await context.getProjectMetadata(projectName);
const sourceRoot = (projectMetadata.sourceRoot ?? projectMetadata.root ?? '') as string;

const files = await findTests(options.include, context.workspaceRoot, projectSourceRoot);
// early exit, no reason to start karma
if (!files.length) {
throw new Error(
`Specified patterns: "${options.include.join(', ')}" did not match any spec files.`,
);
}

// Get the rules and ensure the Webpack configuration is setup properly
const rules = webpackConfig.module?.rules || [];
if (!webpackConfig.module) {
webpackConfig.module = { rules };
} else if (!webpackConfig.module.rules) {
webpackConfig.module.rules = rules;
}

rules.unshift({
test: path.resolve(context.workspaceRoot, options.main),
use: {
// cannot be a simple path as it differs between environments
loader: SingleTestTransformLoader,
options: {
files,
logger: context.logger,
},
},
});
}
webpackConfig.plugins ??= [];
webpackConfig.plugins.push(
new FindTestsPlugin({
include: options.include,
workspaceRoot: context.workspaceRoot,
projectSourceRoot: path.join(context.workspaceRoot, sourceRoot),
}),
);

karmaOptions.buildWebpack = {
options,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@
"items": {
"type": "string"
},
"default": ["**/*.spec.ts"],
"description": "Globs of files to include, relative to workspace or project root. \nThere are 2 special cases:\n - when a path to directory is provided, all spec files ending \".spec.@(ts|tsx)\" will be included\n - when a path to a file is provided, and a matching spec file exists it will be included instead."
},
"sourceMap": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,15 @@ describeBuilder(execute, KARMA_BUILDER_INFO, (harness) => {
include: ['abc.spec.ts', 'def.spec.ts'],
});

const { error } = await harness.executeOnce({
outputLogsOnException: false,
});
expect(error?.message).toBe(
'Specified patterns: "abc.spec.ts, def.spec.ts" did not match any spec files.',
const { result, logs } = await harness.executeOnce();
expect(result?.success).toBeFalse();
expect(logs).toContain(
jasmine.objectContaining({
level: 'error',
message: jasmine.stringContaining(
'Specified patterns: "abc.spec.ts, def.spec.ts" did not match any spec files.',
),
}),
);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,8 @@ export async function getCommonConfig(wco: WebpackConfigOptions): Promise<Config
strictExportPresence: true,
parser: {
javascript: {
// TODO(alanagius): disable the below once we have a migration to remove `require.context` from test.ts file in users projects.
requireContext: true,
// Disable auto URL asset module creation. This doesn't effect `new Worker(new URL(...))`
// https://webpack.js.org/guides/asset-modules/#url-assets
url: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,10 +124,10 @@ const init: any = (config: any, emitter: any) => {

logger.error(statsErrorsToString(statsJson, { colors: true }));

// Notify potential listeners of the compile error.
emitter.emit('compile_error', {
errors: statsJson.errors?.map((e) => e.message),
});
if (config.singleRun) {
// Notify potential listeners of the compile error.
emitter.emit('load_error');
}

// Finish Karma run early in case of compilation error.
emitter.emit('run_complete', [], { exitCode: 1 });
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,8 @@ import {
platformBrowserDynamicTesting,
} from '@angular/platform-browser-dynamic/testing';

declare const require: {
context(path: string, deep?: boolean, filter?: RegExp): {
keys(): string[];
<T>(id: string): T;
};
};

// First, initialize the Angular testing environment.
getTestBed().initTestEnvironment(BrowserDynamicTestingModule, platformBrowserDynamicTesting(), {
errorOnUnknownElements: true,
errorOnUnknownProperties: true
});
// Then we find all the tests.
const context = require.context('./', true, /\.spec\.ts$/);
// And load the modules.
context.keys().forEach(context);
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,8 @@ import {
platformBrowserDynamicTesting
} from '@angular/platform-browser-dynamic/testing';

declare const require: {
context(path: string, deep?: boolean, filter?: RegExp): {
keys(): string[];
<T>(id: string): T;
};
};

// First, initialize the Angular testing environment.
getTestBed().initTestEnvironment(BrowserDynamicTestingModule, platformBrowserDynamicTesting(), {
errorOnUnknownElements: true,
errorOnUnknownProperties: true
});
// Then we find all the tests.
const context = require.context('./', true, /\.spec\.ts$/);
// And load the modules.
context.keys().forEach(context);
12 changes: 0 additions & 12 deletions packages/schematics/angular/application/files/src/test.ts.template
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,8 @@ import {
platformBrowserDynamicTesting
} from '@angular/platform-browser-dynamic/testing';

declare const require: {
context(path: string, deep?: boolean, filter?: RegExp): {
<T>(id: string): T;
keys(): string[];
};
};

// First, initialize the Angular testing environment.
getTestBed().initTestEnvironment(BrowserDynamicTestingModule, platformBrowserDynamicTesting(), {
errorOnUnknownElements: true,
errorOnUnknownProperties: true
});

// Then we find all the tests.
const context = require.context('./', true, /\.spec\.ts$/);
// And load the modules.
context.keys().forEach(context);
12 changes: 0 additions & 12 deletions packages/schematics/angular/library/files/src/test.ts.template
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,8 @@ import {
platformBrowserDynamicTesting
} from '@angular/platform-browser-dynamic/testing';

declare const require: {
context(path: string, deep?: boolean, filter?: RegExp): {
<T>(id: string): T;
keys(): string[];
};
};

// First, initialize the Angular testing environment.
getTestBed().initTestEnvironment(BrowserDynamicTestingModule, platformBrowserDynamicTesting(), {
errorOnUnknownElements: true,
errorOnUnknownProperties: true
});

// Then we find all the tests.
const context = require.context('./', true, /\.spec\.ts$/);
// And load the modules.
context.keys().forEach(context);