fix(ivy): provider override via TestBed should remove old providers f…

…rom the list (#33706)

While overriding providers in Ivy TestBed (via TestBed.overrideProvider call), the old providers were retained in the list, since the override takes precedence. However, presence of providers in the list might have side-effect: if a provider has the `ngOnDestroy` lifecycle hook, this hook will be registered and invoked later (when component is destroyed). This commit updates TestBed logic to clear provider list by removing the ones which have overrides.

PR Close #33706

packages/core/test/test_bed_spec.ts

@@ -16,6 +16,13 @@ import {onlyInIvy} from '@angular/private/testing';
 
 const NAME = new InjectionToken<string>('name');
 
+@Injectable({providedIn: 'root'})
+class SimpleService {
+  static ngOnDestroyCalls: number = 0;
+  id: number = 1;
+  ngOnDestroy() { SimpleService.ngOnDestroyCalls++; }
+}
+
 // -- module: HWModule
 @Component({
   selector: 'hello-world',
@@ -32,7 +39,22 @@ export class HelloWorld {
 export class GreetingCmp {
   name: string;
 
-  constructor(@Inject(NAME) @Optional() name: string) { this.name = name || 'nobody!'; }
+  constructor(
+      @Inject(NAME) @Optional() name: string,
+      @Inject(SimpleService) @Optional() service: SimpleService) {
+    this.name = name || 'nobody!';
+  }
+}
+
+@Component({
+  selector: 'cmp-with-providers',
+  template: '<hello-world></hello-world>',
+  providers: [
+    SimpleService,  //
+    {provide: NAME, useValue: `from Component`}
+  ]
+})
+class CmpWithProviders {
 }
 
 @NgModule({
@@ -88,7 +110,7 @@ export class ComponentWithInlineTemplate {
 @NgModule({
   declarations: [
     HelloWorld, SimpleCmp, WithRefsCmp, InheritedCmp, SimpleApp, ComponentWithPropBindings,
-    HostBindingDir
+    HostBindingDir, CmpWithProviders
   ],
   imports: [GreetingModule],
   providers: [
@@ -252,6 +274,22 @@ describe('TestBed', () => {
     expect(hello.nativeElement).toHaveText('Hello injected World a second time!');
   });
 
+  it('should not call ngOnDestroy for a service that was overridden', () => {
+    SimpleService.ngOnDestroyCalls = 0;
+
+    TestBed.overrideProvider(SimpleService, {useValue: {id: 2, ngOnDestroy: () => {}}});
+    const fixture = TestBed.createComponent(CmpWithProviders);
+    fixture.detectChanges();
+
+    const service = TestBed.inject(SimpleService);
+    expect(service.id).toBe(2);
+
+    fixture.destroy();
+
+    // verify that original `ngOnDestroy` was not called
+    expect(SimpleService.ngOnDestroyCalls).toBe(0);
+  });
+
   describe('multi providers', () => {
     const multiToken = new InjectionToken<string[]>('multiToken');
     const singleToken = new InjectionToken<string>('singleToken');

packages/core/testing/src/r3_test_bed_compiler.ts

@@ -651,18 +651,20 @@ export class R3TestBedCompiler {
     const overrides = this.getProviderOverrides(flattenedProviders);
     const overriddenProviders = [...flattenedProviders, ...overrides];
     const final: Provider[] = [];
-    const seenMultiProviders = new Set<Provider>();
+    const seenOverriddenProviders = new Set<Provider>();
 
-    // We iterate through the list of providers in reverse order to make sure multi provider
-    // overrides take precedence over the values defined in provider list. We also filter out all
-    // multi providers that have overrides, keeping overridden values only.
+    // We iterate through the list of providers in reverse order to make sure provider overrides
+    // take precedence over the values defined in provider list. We also filter out all providers
+    // that have overrides, keeping overridden values only. This is needed, since presence of a
+    // provider with `ngOnDestroy` hook will cause this hook to be registered and invoked later.
     forEachRight(overriddenProviders, (provider: any) => {
       const token: any = getProviderToken(provider);
-      if (isMultiProvider(provider) && this.providerOverridesByToken.has(token)) {
-        // Don't add overridden multi-providers twice because when you override a multi-provider, we
-        // treat it as `{multi: false}` to avoid providing the same value multiple times.
-        if (!seenMultiProviders.has(token)) {
-          seenMultiProviders.add(token);
+      if (this.providerOverridesByToken.has(token)) {
+        if (!seenOverriddenProviders.has(token)) {
+          seenOverriddenProviders.add(token);
+          // Treat all overridden providers as `{multi: false}` (even if it's a multi-provider) to
+          // make sure that provided override takes highest precedence and is not combined with
+          // other instances of the same multi provider.
           final.unshift({...provider, multi: false});
         }
       } else {
@@ -726,10 +728,6 @@ function getProviderToken(provider: Provider) {
   return getProviderField(provider, 'provide') || provider;
 }
 
-function isMultiProvider(provider: Provider) {
-  return !!getProviderField(provider, 'multi');
-}
-
 function isModuleWithProviders(value: any): value is ModuleWithProviders<any> {
   return value.hasOwnProperty('ngModule');
 }