feat(@angular-devkit/build-angular): option to build and test only specified spec files

[FDIM]

A new --include option is added. It is a glob/pattern that specifies which specs should be included in the build.

Command:
ng test --include=app/services/auth*

Few rules are applied to the pattern to make certain things possible:

• .spec.ts is added automatically if missing (less typing)
• if it ends with *.ts and doesn't have .spec.ts, extension will be replaced with .spec.ts
• if path starts at root, source root directory will be trimmed. So that you could use relative file path from editor to make custom tasks to run the command

Behind the scenes test.ts is transformed which instead of using require.context imports all matching files. This gives quite a big boost in the project at work (at least 10s on each iterative build!)

Fixes #3603

Motivation: It is a pain in a big application to wait until entire app is rebuilt just to run single fit block. Takes almost a minute with 4000+ tests and barely works when code coverage is enabled.

Missing: docs(?)

Example tasks for vscode:

{
  // See https://go.microsoft.com/fwlink/?LinkId=733558
  // for the documentation about the tasks.json format
  "version": "2.0.0",
  "tasks": [
    {
      "type": "shell",
      "label": "test selected spec",
      "command": "node_modules/.bin/ng",
      "args": [
        "test",
        "--include",
        "${relativeFile}"
      ],
      "problemMatcher": []
    }
  ]
}

PS. The setup for writing tests is quite nice!

[FDIM]

Original PR (#12955) was closed because of inactivity - 1 day too soon. This one is as ready as I can make it without input

[ngbot]

Hi @FDIM! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

[FDIM]

@filipesilva let me know if you need any help with pushing this futher/making solution better

[filipesilva]

Heya @FDIM, thank you for tackling this feature! I want to give you an update on the status of this PR.

We're looking at the feature as you have designed it so far and thinking about the pros and cons. We're also considering what extra work needs to be done to update projects to use it (e.g. ignoring the generated file). I should have some news soon as I drive it forward within the team!

[filipesilva]

@FDIM heya, we got together and discussed this PR yesterday. This PR adds two new pieces of functionality: running targeted specs, and changing the targeted spec of a running process. We feel there's a lot of value in the former, but comparatively less so in the latter since it's a subset of the usescases of the first.

The main gripe the team has with it is the temporary file. In this PR the temporary file is used to transform the original test code via regexes running it, and communicate between processes when updating the targeted spec

Creating temporary files on disk has a few problems though:

• requires a migration of existing projects (via ng update) to ignore it on git
• it's surprising for users that files are created in their sources
• it can remain on disk despite best efforts to remove it (e.g. process crash)
• multiple targeted spec runs will interfere with each other because they use the same temp file

So for now we'd like to not add the temporary file at all. There are some ways of limiting the specs without it so we can still have targeted specs. But changing targeted specs mid-run without it would not be easy. So for now we propose to have targeted specs but not changing them on the fly, and later we could add a way to do that as well.

If we ignore the cross process communication we could easily avoid the temporary file by either

• editing the tsconfig include for that run, causing only some specs to be compiled, or
• creating a custom context for the require in test.ts, similar to what we do for lazy routes

I favor the first option myself because it also makes for a smaller, TS compilation, which should make both initial build and rebuilds faster.

I understand it's disappointing to be asked to reduce the scope of something you've designed and that covers your usecase very well. But for us as maintainers these questions of migration and usability are always key concerns.

For instance, anything that requires a migration can only be on a major since our policy is to only require migrations for majors. On the other hand something that requires no interaction with the user source is much easier to land. Things that have any chance of leaving artifacts in the user sources are also scrutinized because they are hard to clean after.

Please let me know what you think.

[FDIM]

@filipesilva I agree 100% that generated/temp file is not ideal, but I do not know how else scope could be limited effectively without a way to feed webpack/typescript a stream/string instead of an entry point file.

I'll test if your theory holds by changing tsconfig include option and get back to you. Do you have any pointers for custom context? I'd like to see how that works as well. Anyway, I am fairly certain that require.context would scan full project structure on every build though.

On a flip side, I do not really agree with other team concerns:

• requires a migration of existing projects (via ng update) to ignore it on git
• it's surprising for users that files are created in their sources
• it can remain on disk despite best efforts to remove it (e.g. process crash)

These steps can be fairly easily handled manually and documented as a quirk of --spec option for performance reasons - this is my number 1 reason for spending time on this! Not sure if you realized, but if you do not use the option - file will not be generated. I'd rather spend 5 minutes configuring my project manually and having fastest rebuild time than making this automatic process, but potentially slower (currently I have no idea if tsconfig include option would be slower/faster and if it would be a working solution) and having to restart the process to switch to a different test.
In other words, I am in favor of development experience/quick iteration when doing unit tests over configuration. Unfortunately some things do not have simple solutions.

• multiple targeted spec runs will interfere with each other because they use the same temp file

I'd say this is more like a limitation that you can have only single targeted test run at the same time per project. Isn't it safe to assume that one would use this option when working on a specific feature/task?

Thanks for looking into this!

[filipesilva]

This is where we manipulate the custom context for lazy routes:

angular-cli/packages/ngtools/webpack/src/angular_compiler_plugin.ts

Lines 697 to 753 in 6fc6e84

	compiler.hooks.contextModuleFactory.tap('angular-compiler', cmf => {
	const angularCorePackagePath = require.resolve('@angular/core/package.json');
	// APFv6 does not have single FESM anymore. Instead of verifying if we're pointing to
	// FESMs, we resolve the `@angular/core` path and verify that the path for the
	// module starts with it.
	// This may be slower but it will be compatible with both APF5, 6 and potential future
	// versions (until the dynamic import appears outside of core I suppose).
	// We resolve any symbolic links in order to get the real path that would be used in webpack.
	const angularCoreResourceRoot = fs.realpathSync(path.dirname(angularCorePackagePath));
	cmf.hooks.afterResolve.tapPromise('angular-compiler', async result => {
	// Alter only existing request from Angular or one of the additional lazy module resources.
	const isLazyModuleResource = (resource: string) =>
	resource.startsWith(angularCoreResourceRoot) ||
	( this.options.additionalLazyModuleResources &&
	this.options.additionalLazyModuleResources.includes(resource));
	if (!result || !this.done || !isLazyModuleResource(result.resource)) {
	return result;
	}
	return this.done.then(
	() => {
	// This folder does not exist, but we need to give webpack a resource.
	// TODO: check if we can't just leave it as is (angularCoreModuleDir).
	result.resource = path.join(this._basePath, '$$_lazy_route_resource');
	// tslint:disable-next-line:no-any
	result.dependencies.forEach((d: any) => d.critical = false);
	// tslint:disable-next-line:no-any
	result.resolveDependencies = (_fs: any, options: any, callback: Callback) => {
	const dependencies = Object.keys(this._lazyRoutes)
	.map((key) => {
	const modulePath = this._lazyRoutes[key];
	if (modulePath !== null) {
	const name = key.split('#')[0];
	return new this._contextElementDependencyConstructor(modulePath, name);
	} else {
	return null;
	}
	})
	.filter(x => !!x);
	if (this._options.nameLazyFiles) {
	options.chunkName = '[request]';
	}
	callback(null, dependencies);
	};
	return result;
	},
	() => undefined,
	);
	});
	});

These steps can be fairly easily handled manually and documented as a quirk of --spec option for performance reasons

Generally speaking, anything that requires a user to go figure out the quirks of the tool isn't great. And anything that requires a user to manually change their project isn't going to happen for the vast majority of projects.

Yes, the performance would be better. And I also understand that it would be much preferable for your usecase to have the "hot switch" feature. But I think that for most users, the base "targetted spec" feature gets them most of the way to faster test turnaround time without any quirks, pitfalls, needed upgrades, etc. Then the feature can be enhanced over time.

One thing I ask you to keep in mind is that we have a two release cycle deprecation policy. Since we release roughly every six months, that means that a new feature will need to be around and maintained for at least one year. I hope this puts into perspective why we favor less user-facing complexity and things that don't require upgrade paths.

[FDIM]

You have very valid points from maintenance point of view and I agree, but this does not help in landing such feature to the 'core'. I am also a bit puzzled why there is no demand for such option (maybe not many enterprise apps have tests... I hope not!) or what people do after they pass certain number of tests (gonna have to check jest).

Anyway, I've tried tinkering and customizing tsconfig include option and from my understanding solution would not be simpler. Basically I'd have to recursively find all related files to targeted specs and pass them via files option or include option. I don't think there is a way to pass a mask to typescript and let it handle this automatically.

To make matters worse, consider this tsconfig:

{
    "extends": "./tsconfig.json",
    "exclude": [
        "./node_modules"
    ],
    "include": [
        "source/polyfills.ts",
        "source/test.ts",
        "source/common/components/input/**/*.ts"
    ]
}

One could expect that only files related to the listed ones would be included, but no - typescript also tries to compile files that are not referenced directly or indirectly (such as source/common/services/* or source/mw/*). Only changing require.context('./', true, /\.spec\..s$/); to require.context('./', true, /common\/components\/input\/.*\.spec\..s$/); in test.ts fixes the issue.

Could you share the solution that worked for you when you discussed include option as a potential alternative?

In the meantime I'll investigate what custom context does and how it works - thanks for that!

[filipesilva]

The tsconfig you showed is quite different from the ones we generate for specs, mind you. We explicitly set both files and include. If you don't set files, everything is included by default.

Tried my hand at manipulating the tsconfig manually:

• made a new app with ng new targeted-specs-tsconfig
• added a failing spec src/app/failing.spec.ts: it('should fail', () => expect(false).toBeTruthy());
• npm run test -- --watch=false --browsers=ChromeHeadless fails
• edit tsconfig to have this include property:

  "include": [
    "**/app.component.spec.ts",
    "**/*.d.ts"
  ]

• npm run test -- --watch=false --browsers=ChromeHeadless fails with a badly formatted message that says

Uncaught Error: Module build failed (from ./node_modules/@ngtools/webpack/src/index.js):
Error: D:\\sandbox\\targeted-specs-tsconfig\\src\\app\\failing.spec.ts is missing from the TypeScript compilation. Please make sure it is in your tsconfig via the 'files' or 'include' property.

I expected that to work. But it doesn't because our webpack plugin errors out when requested TS files that aren't in the compilation.

We added this error specifically to avoid implicitly importing TS files. Maybe this is a restriction that we can relax on this particular case, and serve an empty file instead. But either way this approach got more complicated.

[filipesilva]

If you want to check how ignoring that error looks like, open node_modules/@ngtools/webpack/src/angular_compiler_plugin.js and comment out this block:

angular-cli/packages/ngtools/webpack/src/angular_compiler_plugin.ts

Lines 1016 to 1030 in 6fc6e84

	if (((fileName.endsWith('.ts') || fileName.endsWith('.tsx'))
	&& !this._compilerHost.fileExists(fileName))
	|| !this._compilerHost.fileExists(outputFile, false)) {
	let msg = `${fileName} is missing from the TypeScript compilation. `
	+ `Please make sure it is in your tsconfig via the 'files' or 'include' property.`;
	if (/(\\|\/)node_modules(\\|\/)/.test(fileName)) {
	msg += '\nThe missing file seems to be part of a third party library. '
	+ 'TS files in published libraries are often a sign of a badly packaged library. '
	+ 'Please open an issue in the library repository to alert its author and ask them '
	+ 'to package the library using the Angular Package Format (https://goo.gl/jB3GVv).';
	}
	throw new Error(msg);
	}

Doing that will work with my previous example and only run src/app/app.component.spec.ts.

I'm unsure of what the best API within the plugin for this would be, but probably an option that alters the include array, and ignores the error above if the file matches the alteration.

[FDIM]

@filipesilva I think I found a perfect solution!

I'll make a webpack loader (see last commit) that will be registered in karma builder for test.ts files which will transform it in the same way as in original suggestion.

I tested the idea in angular 8 project by modifying karma js file manually in node_modules and it seemed to work.

Now I have to figure out again how to work on cli locally, finalize formatter and update previous test. Hopefully I'll wrap this up this week.

EDIT: I also looked into your previous suggestion with tsconfig and files list, sadly this approach would not prevent entire project from being build as require.context() would still include all the spec files and potentially make performance gains negligible.

[FDIM]

I think I have it working! Only missing 2 things now:

• support directory when one is passed as spec
• figure out sourceRoot from target (so it would also work in multi-project setup)

[FDIM]

PR is ready for review. @filipesilva would you mind having a look at current implementation and provide some feedback? Thanks!

Aside of that some help regarding failing circleci test would be great. Is it supposed to start actual chrome and run dummy tests? The same tests are passing locally and the feature is working as intended when build_angular package is installed in another project.

EDIT: nevermind about the failing test, I've changed suffix to spec_large.ts. Hopefully it will be green now

[FDIM]

PR should be ready now. I've rebased from master and updated karma/schema.json with more details about spec option.

[FDIM]

I just had to adjust the text in schema.json, apparently having **/* in description results in broken schema.ts file. As it ends multi-line comment.

[filipesilva]

I think it looks great! LGTM from my side, just waiting for @clydin to also take a look atm.

[clydin]

Why is sourceRoot used here?
The current behavior with the require.context call is that the files are relative to the test.ts (mainFilePath in this case).
However, I think it would be more intuitive if the option's base directory was the workspace root as this would be consist with all other options. A project might also not have the concept of a source root (its currently really an extension option for the browser builder).
This would also eliminate the need for all the extra code to find the source root.

[FDIM]

Well, option's base directory is workspace root - path relative to source root is an alternative that is exposed, but mainly needed internally. sourceRoot is simply subtracted from what is passed as spec, so that typescript would be able to find these files in test.ts (main) as path relative to it is needed (see findSpecs).

Any suggestions on how to refactor this to not rely on sourceRoot?

[clydin]

The glob.sync call's cwd option can be the workspace root and the absolute option would also be needed. From there, get the absolute path of the directory name of the main file; and then loop and convert the test file list from the glob call to relative paths from the main file.

[FDIM]

I've ended up with a bit different solution and sourceRoot is no longer used. I also reverted some of the other changes. I hope it's OK now.

[FDIM]

@filipesilva I see you've added 8.1 milestone, this is great news!
I am just a bit confused on what should I change to finalize this? Would it be enough to rename option from spec to include (or files)?

[FDIM]

I've rebased from master and I think this is ready to go. include option can now be passed multiple times and appropriate specs are executed in test project.
It's just a bit strange that I see a warning that the option is passed multiple times. Can one of you point out how one can pass an array to cli? e.g. codeCoverageExclude, there is no example in docs :(

[filipesilva]

@FDIM I updated your original PR message to list the new --include name, and remove references to --spec-update. I was pointing people here and they were confused that the spec option wasn't listed in docs.

[krokofant]

Angular CLI: 8.1.2
Node: 10.16.0
OS: win32 x64
Angular: 8.1.2
... animations, cli, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router, upgrade

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.800.6
@angular-devkit/build-angular     0.800.6
@angular-devkit/build-optimizer   0.800.6
@angular-devkit/build-webpack     0.800.6
@angular-devkit/core              7.3.4
@angular-devkit/schematics        8.1.2
@angular/cdk                      8.1.1
@ngtools/webpack                  8.0.6
@schematics/angular               8.1.2
@schematics/update                0.801.2
rxjs                              6.5.2
typescript                        3.4.5
webpack                           4.30.0

npx ng test myProject --include something
Unknown option: '--include'
Unknown option: 'something'

Shouldn't this work?

Omitting the project name produces double error messages:

Unknown option: '--include'
Unknown option: 'something'
Unknown option: '--include'
Unknown option: 'something'

Edit: Started to work after i upgraded the @Angular-devkit packages

[filipesilva]

@krokofant yeah you need to update @angular-devkit/build-angular to include these changes.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}