refactor(core): ComponentFixture autodetect should detect changes wit…

…hin ApplicationRef.tick (angular#54354)

The current behavior of `autoDetect` in `ComponentFixture` does not
match production very well. It has several flaws that make it an
insufficient change detection mechanism:

* It runs change detection for the component under test _after_ views
  attached to the `ApplicationRef`. This can cause real behavior
  differences that break in production, because tests can observe view
  refreshes in the incorrect order (for example, a dialog refreshing
  before the component which opened it).
* Because of the above ordering, render hooks registered during change
  detection of the fixture views _will not execute at all_ because
  `ApplicationRef.tick` already happen.
* It does not rerun change detection on the view tree if there are more
  dirty views to refresh after the render hooks complete.
* It effectively hides/swallows errors from change detection inside the
  `onMicrotaskEmpty` subscription by not reporting them to the error
  handler. Instead, this error ends up being unhandled in the
  subscription and rxjs throws these in a `setTimeout`.

All of the above are problematic but this commit _does not_ fix the
final point. Ideally, we can land this in a future change but this
requires additional internal fixes. In the meantime, we have to juggle
special-case handling of the component fixture views within
`ApplicationRef.tick` using some special events to retain current
behavior and avoid errors from the fixture propagating to the `ErrorHandler`.

breaking note for future when isG3 flag condition is removed for v18:
The `ComponentFixture.autoDetect` feature now executes
change detection for the fixture within `ApplicationRef.tick`. This more
closely matches the behavior of how a component would refresh in
production. The order of component refresh in tests may be slightly
affected as a result, especially when dealing with additional components
attached to the application, such as dialogs. Tests sensitive to this
type of change (such as screenshot tests) may need to be updated.

PR Close angular#54354

packages/core/src/application/application_ref.ts

@@ -9,7 +9,7 @@
 import '../util/ng_jit_mode';
 
 import {setActiveConsumer, setThrowInvalidWriteToSignalError} from '@angular/core/primitives/signals';
-import {Observable} from 'rxjs';
+import {Observable, Subject} from 'rxjs';
 import {first, map} from 'rxjs/operators';
 
 import {Console} from '../console';
@@ -276,6 +276,12 @@ export class ApplicationRef {
   private readonly internalErrorHandler = inject(INTERNAL_APPLICATION_ERROR_HANDLER);
   private readonly afterRenderEffectManager = inject(AfterRenderEventManager);
 
+  // Needed for ComponentFixture temporarily during migration of autoDetect behavior
+  // Eventually the hostView of the fixture should just attach to ApplicationRef.
+  private externalTestViews: Set<InternalViewRef<unknown>> = new Set();
+  private beforeRender = new Subject<boolean>();
+  private afterTick = new Subject<void>();
+
   /**
    * Indicates whether this instance was destroyed.
    */
@@ -508,6 +514,7 @@ export class ApplicationRef {
       // Attention: Don't rethrow as it could cancel subscriptions to Observables!
       this.internalErrorHandler(e);
     } finally {
+      this.afterTick.next();
       this._runningTick = false;
       setActiveConsumer(prevConsumer);
     }
@@ -526,53 +533,30 @@ export class ApplicationRef {
       }
 
       const isFirstPass = runs === 0;
+      this.beforeRender.next(isFirstPass);
       for (let {_lView, notifyErrorHandler} of this._views) {
-        // When re-checking, only check views which actually need it.
-        if (!isFirstPass && !shouldRecheckView(_lView)) {
-          continue;
-        }
-        this.detectChangesInView(_lView, notifyErrorHandler, isFirstPass);
+        detectChangesInViewIfRequired(_lView, isFirstPass, notifyErrorHandler);
       }
       runs++;
 
       afterRenderEffectManager.executeInternalCallbacks();
       // If we have a newly dirty view after running internal callbacks, recheck the views again
       // before running user-provided callbacks
-      if (this._views.some(({_lView}) => shouldRecheckView(_lView))) {
+      if ([...this.externalTestViews.keys(), ...this._views].some(
+              ({_lView}) => shouldRecheckView(_lView))) {
         continue;
       }
 
       afterRenderEffectManager.execute();
       // If after running all afterRender callbacks we have no more views that need to be refreshed,
       // we can break out of the loop
-      if (!this._views.some(({_lView}) => shouldRecheckView(_lView))) {
+      if (![...this.externalTestViews.keys(), ...this._views].some(
+              ({_lView}) => shouldRecheckView(_lView))) {
         break;
       }
     }
   }
 
-  private detectChangesInView(lView: LView, notifyErrorHandler: boolean, isFirstPass: boolean) {
-    let mode: ChangeDetectionMode;
-    if (isFirstPass) {
-      // The first pass is always in Global mode, which includes `CheckAlways` views.
-      mode = ChangeDetectionMode.Global;
-      // Add `RefreshView` flag to ensure this view is refreshed if not already dirty.
-      // `RefreshView` flag is used intentionally over `Dirty` because it gets cleared before
-      // executing any of the actual refresh code while the `Dirty` flag doesn't get cleared
-      // until the end of the refresh. Using `RefreshView` prevents creating a potential
-      // difference in the state of the LViewFlags during template execution.
-      lView[FLAGS] |= LViewFlags.RefreshView;
-    } else if (lView[FLAGS] & LViewFlags.Dirty) {
-      // The root view has been explicitly marked for check, so check it in Global mode.
-      mode = ChangeDetectionMode.Global;
-    } else {
-      // The view has not been marked for check, but contains a view marked for refresh
-      // (likely via a signal). Start this change detection in Targeted mode to skip the root
-      // view and check just the view(s) that need refreshed.
-      mode = ChangeDetectionMode.Targeted;
-    }
-    detectChangesInternal(lView, notifyErrorHandler, mode);
-  }
 
   /**
    * Attaches a view so that it will be dirty checked.
@@ -718,8 +702,40 @@ export function whenStable(applicationRef: ApplicationRef): Promise<void> {
 }
 
 
+export function detectChangesInViewIfRequired(
+    lView: LView, isFirstPass: boolean, notifyErrorHandler: boolean) {
+  // When re-checking, only check views which actually need it.
+  if (!isFirstPass && !shouldRecheckView(lView)) {
+    return;
+  }
+  detectChangesInView(lView, notifyErrorHandler, isFirstPass);
+}
+
 function shouldRecheckView(view: LView): boolean {
   return requiresRefreshOrTraversal(view) ||
       // TODO(atscott): Remove isG3 check and make this a breaking change for v18
       (isG3 && !!(view[FLAGS] & LViewFlags.Dirty));
 }
+
+function detectChangesInView(lView: LView, notifyErrorHandler: boolean, isFirstPass: boolean) {
+  let mode: ChangeDetectionMode;
+  if (isFirstPass) {
+    // The first pass is always in Global mode, which includes `CheckAlways` views.
+    mode = ChangeDetectionMode.Global;
+    // Add `RefreshView` flag to ensure this view is refreshed if not already dirty.
+    // `RefreshView` flag is used intentionally over `Dirty` because it gets cleared before
+    // executing any of the actual refresh code while the `Dirty` flag doesn't get cleared
+    // until the end of the refresh. Using `RefreshView` prevents creating a potential
+    // difference in the state of the LViewFlags during template execution.
+    lView[FLAGS] |= LViewFlags.RefreshView;
+  } else if (lView[FLAGS] & LViewFlags.Dirty) {
+    // The root view has been explicitly marked for check, so check it in Global mode.
+    mode = ChangeDetectionMode.Global;
+  } else {
+    // The view has not been marked for check, but contains a view marked for refresh
+    // (likely via a signal). Start this change detection in Targeted mode to skip the root
+    // view and check just the view(s) that need refreshed.
+    mode = ChangeDetectionMode.Targeted;
+  }
+  detectChangesInternal(lView, notifyErrorHandler, mode);
+}

packages/core/src/core_private_export.ts

@@ -7,9 +7,8 @@
  */
 
 export {setAlternateWeakRefImpl as ɵsetAlternateWeakRefImpl} from '../primitives/signals';
-export {whenStable as ɵwhenStable} from './application/application_ref';
+export {detectChangesInViewIfRequired as ɵdetectChangesInViewIfRequired, whenStable as ɵwhenStable} from './application/application_ref';
 export {IMAGE_CONFIG as ɵIMAGE_CONFIG, IMAGE_CONFIG_DEFAULTS as ɵIMAGE_CONFIG_DEFAULTS, ImageConfig as ɵImageConfig} from './application/application_tokens';
-export {queueStateUpdate as ɵqueueStateUpdate} from './render3/queue_state_update';
 export {internalCreateApplication as ɵinternalCreateApplication} from './application/create_application';
 export {defaultIterableDiffers as ɵdefaultIterableDiffers, defaultKeyValueDiffers as ɵdefaultKeyValueDiffers} from './change_detection/change_detection';
 export {getEnsureDirtyViewsAreAlwaysReachable as ɵgetEnsureDirtyViewsAreAlwaysReachable, setEnsureDirtyViewsAreAlwaysReachable as ɵsetEnsureDirtyViewsAreAlwaysReachable} from './change_detection/flags';
@@ -32,13 +31,15 @@ export {HydratedNode as ɵHydratedNode, HydrationInfo as ɵHydrationInfo, readHy
 export {CurrencyIndex as ɵCurrencyIndex, ExtraLocaleDataIndex as ɵExtraLocaleDataIndex, findLocaleData as ɵfindLocaleData, getLocaleCurrencyCode as ɵgetLocaleCurrencyCode, getLocalePluralCase as ɵgetLocalePluralCase, LocaleDataIndex as ɵLocaleDataIndex, registerLocaleData as ɵregisterLocaleData, unregisterAllLocaleData as ɵunregisterLocaleData} from './i18n/locale_data_api';
 export {DEFAULT_LOCALE_ID as ɵDEFAULT_LOCALE_ID} from './i18n/localization';
 export {Writable as ɵWritable} from './interface/type';
+export {isG3 as ɵisG3} from './is_internal';
 export {ComponentFactory as ɵComponentFactory} from './linker/component_factory';
 export {clearResolutionOfComponentResourcesQueue as ɵclearResolutionOfComponentResourcesQueue, isComponentDefPendingResolution as ɵisComponentDefPendingResolution, resolveComponentResources as ɵresolveComponentResources, restoreComponentResolutionQueue as ɵrestoreComponentResolutionQueue} from './metadata/resource_loading';
 export {PendingTasks as ɵPendingTasks} from './pending_tasks';
 export {ALLOW_MULTIPLE_PLATFORMS as ɵALLOW_MULTIPLE_PLATFORMS} from './platform/platform';
 export {ReflectionCapabilities as ɵReflectionCapabilities} from './reflection/reflection_capabilities';
 export {AnimationRendererType as ɵAnimationRendererType} from './render/api';
 export {InjectorProfilerContext as ɵInjectorProfilerContext, ProviderRecord as ɵProviderRecord, setInjectorProfilerContext as ɵsetInjectorProfilerContext} from './render3/debug/injector_profiler';
+export {queueStateUpdate as ɵqueueStateUpdate} from './render3/queue_state_update';
 export {allowSanitizationBypassAndThrow as ɵallowSanitizationBypassAndThrow, BypassType as ɵBypassType, getSanitizationBypassType as ɵgetSanitizationBypassType, SafeHtml as ɵSafeHtml, SafeResourceUrl as ɵSafeResourceUrl, SafeScript as ɵSafeScript, SafeStyle as ɵSafeStyle, SafeUrl as ɵSafeUrl, SafeValue as ɵSafeValue, unwrapSafeValue as ɵunwrapSafeValue} from './sanitization/bypass';
 export {_sanitizeHtml as ɵ_sanitizeHtml} from './sanitization/html_sanitizer';
 export {_sanitizeUrl as ɵ_sanitizeUrl} from './sanitization/url_sanitizer';

packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json

@@ -278,6 +278,9 @@
   {
     "name": "LifecycleHooksFeature"
   },
+  {
+    "name": "MAXIMUM_REFRESH_RERUNS"
+  },
   {
     "name": "MODIFIER_KEYS"
   },
@@ -797,6 +800,9 @@
   {
     "name": "detectChangesInViewIfAttached"
   },
+  {
+    "name": "detectChangesInViewIfRequired"
+  },
   {
     "name": "detectChangesInternal"
   },

packages/core/test/bundling/animations/bundle.golden_symbols.json

@@ -302,6 +302,9 @@
   {
     "name": "LifecycleHooksFeature"
   },
+  {
+    "name": "MAXIMUM_REFRESH_RERUNS"
+  },
   {
     "name": "MODIFIER_KEYS"
   },
@@ -863,6 +866,9 @@
   {
     "name": "detectChangesInViewIfAttached"
   },
+  {
+    "name": "detectChangesInViewIfRequired"
+  },
   {
     "name": "detectChangesInternal"
   },

packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json

@@ -203,6 +203,9 @@
   {
     "name": "LifecycleHooksFeature"
   },
+  {
+    "name": "MAXIMUM_REFRESH_RERUNS"
+  },
   {
     "name": "MODIFIER_KEYS"
   },
@@ -647,6 +650,9 @@
   {
     "name": "detectChangesInViewIfAttached"
   },
+  {
+    "name": "detectChangesInViewIfRequired"
+  },
   {
     "name": "detectChangesInternal"
   },

packages/core/test/bundling/defer/bundle.golden_symbols.json

@@ -746,6 +746,9 @@
   {
     "name": "detectChangesInViewIfAttached"
   },
+  {
+    "name": "detectChangesInViewIfRequired"
+  },
   {
     "name": "detectChangesInternal"
   },

packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json

@@ -287,6 +287,9 @@
   {
     "name": "LifecycleHooksFeature"
   },
+  {
+    "name": "MAXIMUM_REFRESH_RERUNS"
+  },
   {
     "name": "MODIFIER_KEYS"
   },
@@ -905,6 +908,9 @@
   {
     "name": "detectChangesInViewIfAttached"
   },
+  {
+    "name": "detectChangesInViewIfRequired"
+  },
   {
     "name": "detectChangesInternal"
   },

packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json

@@ -272,6 +272,9 @@
   {
     "name": "LifecycleHooksFeature"
   },
+  {
+    "name": "MAXIMUM_REFRESH_RERUNS"
+  },
   {
     "name": "MODIFIER_KEYS"
   },
@@ -875,6 +878,9 @@
   {
     "name": "detectChangesInViewIfAttached"
   },
+  {
+    "name": "detectChangesInViewIfRequired"
+  },
   {
     "name": "detectChangesInternal"
   },

packages/core/test/bundling/hello_world/bundle.golden_symbols.json

@@ -143,6 +143,9 @@
   {
     "name": "LifecycleHooksFeature"
   },
+  {
+    "name": "MAXIMUM_REFRESH_RERUNS"
+  },
   {
     "name": "MONKEY_PATCH_KEY_NAME"
   },
@@ -506,6 +509,9 @@
   {
     "name": "detectChangesInViewIfAttached"
   },
+  {
+    "name": "detectChangesInViewIfRequired"
+  },
   {
     "name": "detectChangesInternal"
   },

packages/core/test/bundling/hydration/bundle.golden_symbols.json

@@ -233,6 +233,9 @@
   {
     "name": "LifecycleHooksFeature"
   },
+  {
+    "name": "MAXIMUM_REFRESH_RERUNS"
+  },
   {
     "name": "MODIFIER_KEYS"
   },
@@ -707,6 +710,9 @@
   {
     "name": "detectChangesInViewIfAttached"
   },
+  {
+    "name": "detectChangesInViewIfRequired"
+  },
   {
     "name": "detectChangesInternal"
   },

packages/core/test/bundling/router/bundle.golden_symbols.json

@@ -314,6 +314,9 @@
   {
     "name": "MATRIX_PARAM_SEGMENT_RE"
   },
+  {
+    "name": "MAXIMUM_REFRESH_RERUNS"
+  },
   {
     "name": "MODIFIER_KEYS"
   },
@@ -1091,6 +1094,9 @@
   {
     "name": "detectChangesInViewIfAttached"
   },
+  {
+    "name": "detectChangesInViewIfRequired"
+  },
   {
     "name": "detectChangesInternal"
   },

packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json

@@ -185,6 +185,9 @@
   {
     "name": "LifecycleHooksFeature"
   },
+  {
+    "name": "MAXIMUM_REFRESH_RERUNS"
+  },
   {
     "name": "MODIFIER_KEYS"
   },
@@ -575,6 +578,9 @@
   {
     "name": "detectChangesInViewIfAttached"
   },
+  {
+    "name": "detectChangesInViewIfRequired"
+  },
   {
     "name": "detectChangesInternal"
   },

packages/core/test/bundling/todo/bundle.golden_symbols.json

@@ -212,6 +212,9 @@
   {
     "name": "LifecycleHooksFeature"
   },
+  {
+    "name": "MAXIMUM_REFRESH_RERUNS"
+  },
   {
     "name": "MODIFIER_KEYS"
   },
@@ -779,6 +782,9 @@
   {
     "name": "detectChangesInViewIfAttached"
   },
+  {
+    "name": "detectChangesInViewIfRequired"
+  },
   {
     "name": "detectChangesInternal"
   },