fix(core): errors during ApplicationRef.tick should be rethrown for zoneless tests (#56993)

atscott · dylhunn · commit f7ab04018ea0 · 2024-07-29T13:49:01.000-07:00
The behavior of `ComponentFixture` for zoneless tests was decided somewhat through guesswork, trial, and error. In addition, working on the zoneless fixture revealed oddities in the behavior of the zone-based fixture, or behaviors that we felt were counterintuitive. The most consequential difference is how change detection works: `detectChanges` goes through ApplicationRef.tick in zoneless while it is `changeDetectorRef.detectChanges` in the zone fixture. We felt that running change detection through `ApplicationRef.tick` was important for several reasons: * Aligning application behavior more closely with the test behavior (almost all views are attached to application ref in reality) * Ensuring that afterRender* hooks are executed when calling `fixture.detectChanges` * Ensuring that the change detection runs again if render hooks update state This change, however, has some noticeable consequences that will break some tests, mostly around how errors are handled. `ApplicationRef.tick` catches errors that happen during change detection and reports them to the ErrorHandler from DI. The default error handler only logs the error to the console. This will break tests which have `expect(() => fixture.detectChanges()).toThrow()`. In addition, it allows tests to pass when there are real errors encountered during change detection. This change ensures that errors from `ApplicationRef.tick` are rethrown and will fail the test. We should also do a follow-up investigation to determine whether we can/should also do this for the zone-based `ComponentFixture`. fixes #56977 PR Close #56993
diff --git a/packages/core/src/change_detection/scheduling/ng_zone_scheduling.ts b/packages/core/src/change_detection/scheduling/ng_zone_scheduling.ts
@@ -18,7 +18,6 @@ import {
   makeEnvironmentProviders,
   StaticProvider,
 } from '../../di';
-import {ErrorHandler, INTERNAL_APPLICATION_ERROR_HANDLER} from '../../error_handler';
 import {RuntimeError, RuntimeErrorCode} from '../../errors';
 import {PendingTasks} from '../../pending_tasks';
 import {performanceMarkFeature} from '../../util/performance';
@@ -113,19 +112,12 @@ export function internalProvideZoneChangeDetection({
         };
       },
     },
-    {provide: INTERNAL_APPLICATION_ERROR_HANDLER, useFactory: ngZoneApplicationErrorHandlerFactory},
     // Always disable scheduler whenever explicitly disabled, even if another place called
     // `provideZoneChangeDetection` without the 'ignore' option.
     ignoreChangesOutsideZone === true ? {provide: ZONELESS_SCHEDULER_DISABLED, useValue: true} : [],
   ];
 }
 
-export function ngZoneApplicationErrorHandlerFactory() {
-  const zone = inject(NgZone);
-  const userErrorHandler = inject(ErrorHandler);
-  return (e: unknown) => zone.runOutsideAngular(() => userErrorHandler.handleError(e));
-}
-
 /**
  * Provides `NgZone`-based change detection for the application bootstrapped using
  * `bootstrapApplication`.
diff --git a/packages/core/src/core_private_export.ts b/packages/core/src/core_private_export.ts
@@ -11,6 +11,7 @@ export {
   detectChangesInViewIfRequired as ɵdetectChangesInViewIfRequired,
   whenStable as ɵwhenStable,
 } from './application/application_ref';
+export {INTERNAL_APPLICATION_ERROR_HANDLER as ɵINTERNAL_APPLICATION_ERROR_HANDLER} from './error_handler';
 export {
   IMAGE_CONFIG as ɵIMAGE_CONFIG,
   IMAGE_CONFIG_DEFAULTS as ɵIMAGE_CONFIG_DEFAULTS,
diff --git a/packages/core/src/error_handler.ts b/packages/core/src/error_handler.ts
@@ -8,6 +8,7 @@
 
 import {inject, InjectionToken} from './di';
 import {getOriginalError} from './util/errors';
+import {NgZone} from './zone';
 
 /**
  * Provides a hook for centralized exception handling.
@@ -71,8 +72,9 @@ export const INTERNAL_APPLICATION_ERROR_HANDLER = new InjectionToken<(e: any) =>
   {
     providedIn: 'root',
     factory: () => {
+      const zone = inject(NgZone);
       const userErrorHandler = inject(ErrorHandler);
-      return userErrorHandler.handleError.bind(this);
+      return (e: unknown) => zone.runOutsideAngular(() => userErrorHandler.handleError(e));
     },
   },
 );
diff --git a/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json b/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json
@@ -1256,9 +1256,6 @@
   {
     "name": "ngOnChangesSetInput"
   },
-  {
-    "name": "ngZoneApplicationErrorHandlerFactory"
-  },
   {
     "name": "nonNull"
   },
diff --git a/packages/core/test/bundling/animations/bundle.golden_symbols.json b/packages/core/test/bundling/animations/bundle.golden_symbols.json
@@ -1319,9 +1319,6 @@
   {
     "name": "ngOnChangesSetInput"
   },
-  {
-    "name": "ngZoneApplicationErrorHandlerFactory"
-  },
   {
     "name": "noSideEffects"
   },
diff --git a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json
@@ -1061,9 +1061,6 @@
   {
     "name": "ngOnChangesSetInput"
   },
-  {
-    "name": "ngZoneApplicationErrorHandlerFactory"
-  },
   {
     "name": "noSideEffects"
   },
diff --git a/packages/core/test/bundling/defer/bundle.golden_symbols.json b/packages/core/test/bundling/defer/bundle.golden_symbols.json
@@ -2285,9 +2285,6 @@
   {
     "name": "ngOnChangesSetInput"
   },
-  {
-    "name": "ngZoneApplicationErrorHandlerFactory"
-  },
   {
     "name": "nonNull"
   },
diff --git a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json
@@ -1583,9 +1583,6 @@
   {
     "name": "ngOnChangesSetInput"
   },
-  {
-    "name": "ngZoneApplicationErrorHandlerFactory"
-  },
   {
     "name": "noSideEffects"
   },
diff --git a/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json b/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json
@@ -1550,9 +1550,6 @@
   {
     "name": "ngOnChangesSetInput"
   },
-  {
-    "name": "ngZoneApplicationErrorHandlerFactory"
-  },
   {
     "name": "noSideEffects"
   },
diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json
@@ -836,9 +836,6 @@
   {
     "name": "ngOnChangesSetInput"
   },
-  {
-    "name": "ngZoneApplicationErrorHandlerFactory"
-  },
   {
     "name": "noop"
   },
diff --git a/packages/core/test/bundling/hydration/bundle.golden_symbols.json b/packages/core/test/bundling/hydration/bundle.golden_symbols.json
@@ -1178,9 +1178,6 @@
   {
     "name": "ngOnChangesSetInput"
   },
-  {
-    "name": "ngZoneApplicationErrorHandlerFactory"
-  },
   {
     "name": "nonNull"
   },
diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json
@@ -1778,9 +1778,6 @@
   {
     "name": "ngOnChangesSetInput"
   },
-  {
-    "name": "ngZoneApplicationErrorHandlerFactory"
-  },
   {
     "name": "noMatch"
   },
diff --git a/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json b/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json
@@ -941,9 +941,6 @@
   {
     "name": "ngOnChangesSetInput"
   },
-  {
-    "name": "ngZoneApplicationErrorHandlerFactory"
-  },
   {
     "name": "nonNull"
   },
diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json
@@ -1277,9 +1277,6 @@
   {
     "name": "ngOnChangesSetInput"
   },
-  {
-    "name": "ngZoneApplicationErrorHandlerFactory"
-  },
   {
     "name": "noSideEffects"
   },
diff --git a/packages/core/test/component_fixture_spec.ts b/packages/core/test/component_fixture_spec.ts
@@ -7,10 +7,14 @@
  */
 
 import {
+  ApplicationRef,
   Component,
+  EnvironmentInjector,
   ErrorHandler,
   Injectable,
   Input,
+  NgZone,
+  createComponent,
   provideExperimentalZonelessChangeDetection,
   signal,
 } from '@angular/core';
@@ -333,6 +337,53 @@ describe('ComponentFixture', () => {
     });
   }));
 
+  it('throws errors that happen during detectChanges', () => {
+    @Component({
+      template: '',
+      standalone: true,
+    })
+    class App {
+      ngOnInit() {
+        throw new Error();
+      }
+    }
+
+    const fixture = TestBed.createComponent(App);
+    expect(() => fixture.detectChanges()).toThrow();
+  });
+
+  // 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 () => {
+    @Component({
+      template: '',
+      standalone: true,
+    })
+    class ThrowingThing {
+      ngOnInit() {
+        throw new Error();
+      }
+    }
+    @Component({
+      template: '',
+      standalone: true,
+    })
+    class Blank {}
+
+    const fixture = TestBed.createComponent(Blank);
+    const throwingThing = createComponent(ThrowingThing, {
+      environmentInjector: TestBed.inject(EnvironmentInjector),
+    });
+
+    TestBed.inject(ApplicationRef).attachView(throwingThing.hostView);
+    await expectAsync(fixture.whenStable()).toBeResolved();
+  });
+
   describe('defer', () => {
     it('should return all defer blocks in the component', async () => {
       @Component({
@@ -388,17 +439,35 @@ describe('ComponentFixture', () => {
       componentFixture.detectChanges();
       expect(componentFixture.nativeElement).toHaveText('MyIf(More)');
     }));
+
+    it('throws errors that happen during detectChanges', () => {
+      @Component({
+        template: '',
+        standalone: true,
+      })
+      class App {
+        ngOnInit() {
+          throw new Error();
+        }
+      }
+
+      const fixture = TestBed.createComponent(App);
+      expect(() => fixture.detectChanges()).toThrow();
+    });
   });
 });
 
 describe('ComponentFixture with zoneless', () => {
-  it('will not refresh CheckAlways views when detectChanges is called if not marked dirty', () => {
+  beforeEach(() => {
     TestBed.configureTestingModule({
       providers: [
         provideExperimentalZonelessChangeDetection(),
         {provide: ErrorHandler, useValue: {handleError: () => {}}},
       ],
     });
+  });
+
+  it('will not refresh CheckAlways views when detectChanges is called if not marked dirty', () => {
     @Component({standalone: true, template: '{{signalThing()}}|{{regularThing}}'})
     class CheckAlwaysCmp {
       regularThing = 'initial';
@@ -410,10 +479,41 @@ describe('ComponentFixture with zoneless', () => {
     fixture.detectChanges();
     expect(fixture.nativeElement.innerText).toEqual('initial|initial');
     fixture.componentInstance.regularThing = 'new';
-    fixture.detectChanges();
+    // Expression changed after checked
+    expect(() => fixture.detectChanges()).toThrow();
     expect(fixture.nativeElement.innerText).toEqual('initial|initial');
     fixture.componentInstance.signalThing.set('new');
     fixture.detectChanges();
     expect(fixture.nativeElement.innerText).toEqual('new|new');
   });
+
+  it('throws errors that happen during detectChanges', () => {
+    @Component({
+      template: '',
+      standalone: true,
+    })
+    class App {
+      ngOnInit() {
+        throw new Error();
+      }
+    }
+
+    const fixture = TestBed.createComponent(App);
+    expect(() => fixture.detectChanges()).toThrow();
+  });
+
+  it('rejects whenStable promise when errors happen during detectChanges', async () => {
+    @Component({
+      template: '',
+      standalone: true,
+    })
+    class App {
+      ngOnInit() {
+        throw new Error();
+      }
+    }
+
+    const fixture = TestBed.createComponent(App);
+    await expectAsync(fixture.whenStable()).toBeRejected();
+  });
 });
diff --git a/packages/core/testing/src/application_error_handler.ts b/packages/core/testing/src/application_error_handler.ts
@@ -0,0 +1,52 @@
+/**
+ * @license
+ * Copyright Google LLC All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {
+  ErrorHandler,
+  inject,
+  NgZone,
+  Injectable,
+  ɵZONELESS_ENABLED as ZONELESS_ENABLED,
+} from '@angular/core';
+
+@Injectable()
+export class TestBedApplicationErrorHandler {
+  private readonly zone = inject(NgZone);
+  private readonly userErrorHandler = inject(ErrorHandler);
+  private readonly zoneless = inject(ZONELESS_ENABLED);
+  readonly whenStableRejectFunctions: Set<(e: unknown) => void> = new Set();
+
+  handleError(e: unknown) {
+    // TODO(atscott): Investigate if we can align the behaviors of zone and zoneless
+    if (this.zoneless) {
+      this.zonelessHandleError(e);
+    } else {
+      this.zone.runOutsideAngular(() => this.userErrorHandler.handleError(e));
+    }
+  }
+
+  private zonelessHandleError(e: unknown) {
+    try {
+      this.zone.runOutsideAngular(() => this.userErrorHandler.handleError(e));
+    } catch (userError: unknown) {
+      e = userError;
+    }
+
+    // Instead of throwing the error when there are outstanding `fixture.whenStable` promises,
+    // reject those promises with the error. This allows developers to write
+    // expectAsync(fix.whenStable()).toBeRejected();
+    if (this.whenStableRejectFunctions.size > 0) {
+      for (const fn of this.whenStableRejectFunctions.values()) {
+        fn(e);
+      }
+      this.whenStableRejectFunctions.clear();
+    } else {
+      throw e;
+    }
+  }
+}
diff --git a/packages/core/testing/src/component_fixture.ts b/packages/core/testing/src/component_fixture.ts
diff --git a/packages/core/testing/src/test_bed_compiler.ts b/packages/core/testing/src/test_bed_compiler.ts

Original file line number	Diff line number	Diff line change
`@@ -1256,9 +1256,6 @@`
`1256`	`1256`	`{`
`1257`	`1257`	`"name": "ngOnChangesSetInput"`
`1258`	`1258`	`},`
`1259`		`- {`
`1260`		`- "name": "ngZoneApplicationErrorHandlerFactory"`
`1261`		`- },`
`1262`	`1259`	`{`
`1263`	`1260`	`"name": "nonNull"`
`1264`	`1261`	`},`
Original file line number	Diff line number	Diff line change
`@@ -1319,9 +1319,6 @@`
`1319`	`1319`	`{`
`1320`	`1320`	`"name": "ngOnChangesSetInput"`
`1321`	`1321`	`},`
`1322`		`- {`
`1323`		`- "name": "ngZoneApplicationErrorHandlerFactory"`
`1324`		`- },`
`1325`	`1322`	`{`
`1326`	`1323`	`"name": "noSideEffects"`
`1327`	`1324`	`},`
Original file line number	Diff line number	Diff line change
`@@ -1061,9 +1061,6 @@`
`1061`	`1061`	`{`
`1062`	`1062`	`"name": "ngOnChangesSetInput"`
`1063`	`1063`	`},`
`1064`		`- {`
`1065`		`- "name": "ngZoneApplicationErrorHandlerFactory"`
`1066`		`- },`
`1067`	`1064`	`{`
`1068`	`1065`	`"name": "noSideEffects"`
`1069`	`1066`	`},`
Original file line number	Diff line number	Diff line change
`@@ -2285,9 +2285,6 @@`
`2285`	`2285`	`{`
`2286`	`2286`	`"name": "ngOnChangesSetInput"`
`2287`	`2287`	`},`
`2288`		`- {`
`2289`		`- "name": "ngZoneApplicationErrorHandlerFactory"`
`2290`		`- },`
`2291`	`2288`	`{`
`2292`	`2289`	`"name": "nonNull"`
`2293`	`2290`	`},`
Original file line number	Diff line number	Diff line change
`@@ -1583,9 +1583,6 @@`
`1583`	`1583`	`{`
`1584`	`1584`	`"name": "ngOnChangesSetInput"`
`1585`	`1585`	`},`
`1586`		`- {`
`1587`		`- "name": "ngZoneApplicationErrorHandlerFactory"`
`1588`		`- },`
`1589`	`1586`	`{`
`1590`	`1587`	`"name": "noSideEffects"`
`1591`	`1588`	`},`
Original file line number	Diff line number	Diff line change
`@@ -1550,9 +1550,6 @@`
`1550`	`1550`	`{`
`1551`	`1551`	`"name": "ngOnChangesSetInput"`
`1552`	`1552`	`},`
`1553`		`- {`
`1554`		`- "name": "ngZoneApplicationErrorHandlerFactory"`
`1555`		`- },`
`1556`	`1553`	`{`
`1557`	`1554`	`"name": "noSideEffects"`
`1558`	`1555`	`},`
Original file line number	Diff line number	Diff line change
`@@ -836,9 +836,6 @@`
`836`	`836`	`{`
`837`	`837`	`"name": "ngOnChangesSetInput"`
`838`	`838`	`},`
`839`		`- {`
`840`		`- "name": "ngZoneApplicationErrorHandlerFactory"`
`841`		`- },`
`842`	`839`	`{`
`843`	`840`	`"name": "noop"`
`844`	`841`	`},`