ng test exits with success even if test code does not compile

[vigie]

🐞 Bug report

Command (mark with an x)

• new
• build
• serve
• test
• e2e
• generate
• add
• update
• lint
• extract-i18n
• run
• config
• help
• version
• doc

Is this a regression?

Yes, the previous version in which this bug was not present was: 13.2.2

Description

A regression introduced in v13.2.3 of the CLI has been allowing test code that doesn't even compile to appear as a passing job in our CICD system.

The problem can be traced to this change which includes an unconditional promise resolve following compilation, no matter if there are compilation errors. I think I understand the intent here, but I believe the singleRun scenario was overlooked. When running in watch mode, it's true that you always want Karma to start up and the exit code of the command is irrelevant, since the command runs until the user forces it to stop. But within CICD systems singleRun is the norm and the exit code is very important. We do not want karma to start if the compilation fails and the command should exit with a non zero code.

I think all that is required is to add a reject handler and call that one instead it there are errors && singleRun. I have verified this fix locally and will open a PR with precisely this change against this issue.

🔬 Minimal Reproduction

Create an Angular project using ng cli 13.2.3 or above.

Create one unit test. Include a compilation error in the test.

Ensure the karma option failOnEmptyTestSuite is false.

Run ng test --project=<project-name> --watch=false

Observe that karma starts despite the compilation error and the command exits with success (0 code).

This is all captured in the following reproduction repo: https://github.com/vigie/false-positive-repo-app

🔥 Exception or Error

🌍 Your Environment

     _                      _                 ____ _     ___
    / \   _ __   __ _ _   _| | __ _ _ __     / ___| |   |_ _|
   / △ \ | '_ \ / _` | | | | |/ _` | '__|   | |   | |    | |
  / ___ \| | | | (_| | |_| | | (_| | |      | |___| |___ | |
 /_/   \_\_| |_|\__, |\__,_|_|\__,_|_|       \____|_____|___|
                |___/


Angular CLI: 13.3.8
Node: 14.20.0
Package Manager: npm 6.14.17
OS: linux x64

Angular: 13.3.11
... animations, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... platform-server, router

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1303.8
@angular-devkit/build-angular   13.3.8
@angular-devkit/core            13.3.8
@angular-devkit/schematics      13.3.8
@angular/cdk                    13.3.9
@angular/cli                    13.3.8
@schematics/angular             13.3.8
ng-packagr                      13.3.1
rxjs                            6.6.7
typescript                      4.6.3
webpack                         5.73.0

Anything else relevant?

As mentioned, this was introduced at v13.2.3 and as far as I can tell still exists in latest v14. The fix should be backported and released in v13 also.

[alan-agius4]

I looked into this and it does appear that tests exit with a non zero error code. However the following TOTAL: 0 SUCCESS will be displayed in the console which might cause some confusion.

[vigie]

Hi @alan-agius4 , really appreciate you jumping on this.

To reproduce and get the zero exit code, you must add the karma.conf.js property

failOnEmptyTestSuite: false

Again note, prior to 13.2.3, even with this option set to false we would see a non-zero exit code.

I created this reproduction repo using the latest Angular CLI version, just to make this 100% clear

https://github.com/vigie/false-positive-repo-app

[vigie]

I'm happy to fix up my PR if you think I have the correct fix, but I need some help identifying exactly which tests are failing...I can't seem to parse the Circle CI logs to trace back to actual test code.

[alan-agius4]

I did check your fix unfortunately your PR will cause the process not to terminate.

[robertIsaac]

@alan-agius4 why do you mark the frequently of this issue as low, a lot of people depend on the cicd to catch the unit test errors, specially when you have multiple libraries

even if you run it locally we use nx run-many --watch=false because if watch=true it won't run the next one because it waits for it to exit which will never happen with watch

the only way now to catch it is to run the test one by one manually, which is ridiculous in our project because we have more than a dozen libraries

[vigie]

I did check your fix unfortunately your PR will cause the process not to terminate.

Hi @alan-agius4 can you expand a little on what you mean here and how to reproduce? With the fix in my PR I observe the process exiting with a non-zero error code. Are you referring to when it is run in watch mode? In that case the process should not exit, correct?

[alan-agius4]

Hi @vigie,

You can reproduce by running this test using the below command. With the changes suggested the process does not terminate when there is a compilation error in non watch mode.

node tests/legacy-cli/run_e2e.js tests/legacy-cli/e2e/tests/test/test-fail-single-run.ts

The primary reason why this is classified as low, is being by default failOnEmptyTestSuite is by default configured to true,(http://karma-runner.github.io/1.0/config/configuration-file.html#failonemptytestsuite) so unless explicitly setting this to false you should still get a non zero exit code.