From cf7f761953efd2b78d7b8dbbacbfd39aa29258ad Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Tue, 30 Jul 2024 15:16:14 -0700 Subject: [PATCH] fix(core): rethrow errors during ApplicationRef.tick in TestBed Errors during change detection from `ApplicationRef.tick` are only reported to the `ErrorHandler`. By default, this only logs the error to console. As a result, these errors can be missed/ignored and allow tests to pass when they should not. This change ensures that the errors are surfaced. Note that this is already the behavior when zoneless is enabled. BREAKING CHANGE: Errors that are thrown during `ApplicationRef.tick` will now be rethrown when using `TestBed`. These errors should be resolved by ensuring the test environment is set up correctly to complete change detection successfully. There are two alternatives to catch the errors: * Instead of waiting for automatic change detection to happen, trigger it synchronously and expect the error. For example, a jasmine test could write `expect(() => TestBed.inject(ApplicationRef).tick()).toThrow()` * `TestBed` will reject any outstanding `ComponentFixture.whenStable` promises. A jasmine test, for example, could write `expectAsync(fixture.whenStable()).toBeRejected()`. As a last resort, you can configure errors to _not_ be rethrown by setting `rethrowApplicationErrors` to `false` in `TestBed.configureTestingModule`. --- goldens/public-api/core/testing/index.api.md | 1 + packages/core/test/component_fixture_spec.ts | 17 +++----- .../testing/src/application_error_handler.ts | 4 +- packages/core/testing/src/test_bed_common.ts | 17 ++++++-- .../core/testing/src/test_bed_compiler.ts | 42 +++++++++---------- 5 files changed, 42 insertions(+), 39 deletions(-) diff --git a/goldens/public-api/core/testing/index.api.md b/goldens/public-api/core/testing/index.api.md index 24cf38d77489..99aaa06da3c2 100644 --- a/goldens/public-api/core/testing/index.api.md +++ b/goldens/public-api/core/testing/index.api.md @@ -214,6 +214,7 @@ export interface TestModuleMetadata { imports?: any[]; // (undocumented) providers?: any[]; + rethrowApplicationErrors?: boolean; // (undocumented) schemas?: Array; // (undocumented) diff --git a/packages/core/test/component_fixture_spec.ts b/packages/core/test/component_fixture_spec.ts index e4603e80b0b4..d306f7946345 100644 --- a/packages/core/test/component_fixture_spec.ts +++ b/packages/core/test/component_fixture_spec.ts @@ -367,32 +367,25 @@ describe('ComponentFixture', () => { }) class Blank {} - // note: this test only verifies existing behavior was not broken by a change to the zoneless fixture. - // We probably do want the whenStable promise to be rejected. The current zone-based fixture is bad - // and confusing for two reason: - // 1. with autoDetect, errors in the fixture _cannot be handled_ with whenStable because - // they're just thrown inside the rxjs subcription (and then goes to setTimeout(() => throw e)) - // 2. errors from other views attached to ApplicationRef just go to the ErrorHandler, which by default - // only logs to console, allowing the test to pass - it('resolves whenStable promise when errors happen during appRef.tick', async () => { + it('rejects whenStable promise when errors happen during appRef.tick', async () => { const fixture = TestBed.createComponent(Blank); const throwingThing = createComponent(ThrowingThing, { environmentInjector: TestBed.inject(EnvironmentInjector), }); TestBed.inject(ApplicationRef).attachView(throwingThing.hostView); - await expectAsync(fixture.whenStable()).toBeResolved(); + await expectAsync(fixture.whenStable()).toBeRejected(); }); - it('can opt-in to rethrowing application errors and rejecting whenStable promises', async () => { - TestBed.configureTestingModule({_rethrowApplicationTickErrors: true} as any); + it('can opt-out of rethrowing application errors and rejecting whenStable promises', async () => { + TestBed.configureTestingModule({rethrowApplicationErrors: false}); const fixture = TestBed.createComponent(Blank); const throwingThing = createComponent(ThrowingThing, { environmentInjector: TestBed.inject(EnvironmentInjector), }); TestBed.inject(ApplicationRef).attachView(throwingThing.hostView); - await expectAsync(fixture.whenStable()).toBeRejected(); + await expectAsync(fixture.whenStable()).toBeResolved(); }); }); diff --git a/packages/core/testing/src/application_error_handler.ts b/packages/core/testing/src/application_error_handler.ts index c945b09fc1cc..e8051e1cba29 100644 --- a/packages/core/testing/src/application_error_handler.ts +++ b/packages/core/testing/src/application_error_handler.ts @@ -6,9 +6,9 @@ * found in the LICENSE file at https://angular.io/license */ -import {ErrorHandler, inject, NgZone, Injectable, InjectionToken} from '@angular/core'; +import {ErrorHandler, inject, NgZone, Injectable} from '@angular/core'; -export const RETHROW_APPLICATION_ERRORS = new InjectionToken('rethrow application errors'); +export const RETHROW_APPLICATION_ERRORS_DEFAULT = true; @Injectable() export class TestBedApplicationErrorHandler { diff --git a/packages/core/testing/src/test_bed_common.ts b/packages/core/testing/src/test_bed_common.ts index 01eb019f3245..07c300324e2d 100644 --- a/packages/core/testing/src/test_bed_common.ts +++ b/packages/core/testing/src/test_bed_common.ts @@ -68,14 +68,25 @@ export interface TestModuleMetadata { */ errorOnUnknownProperties?: boolean; + /** + * Whether errors that happen during application change detection should be rethrown. + * + * When `true`, errors that are caught during application change detection will + * be reported to the `ErrorHandler` and rethrown to prevent them from going + * unnoticed in tests. + * + * When `false`, errors are only forwarded to the `ErrorHandler`, which by default + * simply logs them to the console. + * + * Defaults to `true`. + */ + rethrowApplicationErrors?: boolean; + /** * Whether defer blocks should behave with manual triggering or play through normally. * Defaults to `manual`. */ deferBlockBehavior?: DeferBlockBehavior; - - /** @internal */ - _rethrowApplicationTickErrors?: boolean; } /** diff --git a/packages/core/testing/src/test_bed_compiler.ts b/packages/core/testing/src/test_bed_compiler.ts index 050357dc5ef5..297465799f2b 100644 --- a/packages/core/testing/src/test_bed_compiler.ts +++ b/packages/core/testing/src/test_bed_compiler.ts @@ -22,7 +22,6 @@ import { LOCALE_ID, ModuleWithComponentFactories, ModuleWithProviders, - ɵZONELESS_ENABLED as ZONELESS_ENABLED, NgModule, NgModuleFactory, Pipe, @@ -52,6 +51,7 @@ import { ɵNG_INJ_DEF as NG_INJ_DEF, ɵNG_MOD_DEF as NG_MOD_DEF, ɵNG_PIPE_DEF as NG_PIPE_DEF, + ɵZONELESS_ENABLED as ZONELESS_ENABLED, ɵNgModuleFactory as R3NgModuleFactory, ɵNgModuleTransitiveScopes as NgModuleTransitiveScopes, ɵNgModuleType as NgModuleType, @@ -80,7 +80,7 @@ import { } from './resolvers'; import {DEFER_BLOCK_DEFAULT_BEHAVIOR, TestModuleMetadata} from './test_bed_common'; import { - RETHROW_APPLICATION_ERRORS, + RETHROW_APPLICATION_ERRORS_DEFAULT, TestBedApplicationErrorHandler, } from './application_error_handler'; @@ -190,6 +190,7 @@ export class TestBedCompiler { private testModuleRef: NgModuleRef | null = null; private deferBlockBehavior = DEFER_BLOCK_DEFAULT_BEHAVIOR; + private rethrowApplicationTickErrors = RETHROW_APPLICATION_ERRORS_DEFAULT; constructor( private platform: PlatformRef, @@ -226,16 +227,14 @@ export class TestBedCompiler { if (moduleDef.providers !== undefined) { this.providers.push(...moduleDef.providers); } - this.providers.push({ - provide: RETHROW_APPLICATION_ERRORS, - useValue: moduleDef._rethrowApplicationTickErrors ?? false, - }); if (moduleDef.schemas !== undefined) { this.schemas.push(...moduleDef.schemas); } this.deferBlockBehavior = moduleDef.deferBlockBehavior ?? DEFER_BLOCK_DEFAULT_BEHAVIOR; + this.rethrowApplicationTickErrors = + moduleDef.rethrowApplicationErrors ?? RETHROW_APPLICATION_ERRORS_DEFAULT; } overrideModule(ngModule: Type, override: MetadataOverride): void { @@ -944,22 +943,6 @@ export class TestBedCompiler { ...this.rootProviderOverrides, internalProvideZoneChangeDetection({}), TestBedApplicationErrorHandler, - { - provide: INTERNAL_APPLICATION_ERROR_HANDLER, - useFactory: () => { - if (inject(ZONELESS_ENABLED) || inject(RETHROW_APPLICATION_ERRORS, {optional: true})) { - const handler = inject(TestBedApplicationErrorHandler); - return (e: unknown) => { - handler.handleError(e); - }; - } else { - const userErrorHandler = inject(ErrorHandler); - const ngZone = inject(NgZone); - return (e: unknown) => - ngZone.runOutsideAngular(() => userErrorHandler.handleError(e)); - } - }, - }, {provide: ChangeDetectionScheduler, useExisting: ChangeDetectionSchedulerImpl}, ], }); @@ -967,6 +950,21 @@ export class TestBedCompiler { const providers = [ {provide: Compiler, useFactory: () => new R3TestCompiler(this)}, {provide: DEFER_BLOCK_CONFIG, useValue: {behavior: this.deferBlockBehavior}}, + { + provide: INTERNAL_APPLICATION_ERROR_HANDLER, + useFactory: () => { + if (this.rethrowApplicationTickErrors) { + const handler = inject(TestBedApplicationErrorHandler); + return (e: unknown) => { + handler.handleError(e); + }; + } else { + const userErrorHandler = inject(ErrorHandler); + const ngZone = inject(NgZone); + return (e: unknown) => ngZone.runOutsideAngular(() => userErrorHandler.handleError(e)); + } + }, + }, ...this.providers, ...this.providerOverrides, ];