From eece138fa78ca35ae33d76f091a0a99efbf44b10 Mon Sep 17 00:00:00 2001
From: Andrew Kushnir <akushnir@google.com>
Date: Fri, 8 Nov 2019 16:27:53 -0800
Subject: [PATCH] fix(ivy): provider override via TestBed should remove old
 providers from 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           | 42 ++++++++++++++++++-
 .../core/testing/src/r3_test_bed_compiler.ts  | 24 +++++------
 2 files changed, 51 insertions(+), 15 deletions(-)

diff --git a/packages/core/test/test_bed_spec.ts b/packages/core/test/test_bed_spec.ts
index 3254db34d6..0696e50dc2 100644
--- a/packages/core/test/test_bed_spec.ts
+++ b/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');
diff --git a/packages/core/testing/src/r3_test_bed_compiler.ts b/packages/core/testing/src/r3_test_bed_compiler.ts
index 551b038d7e..ff5402140a 100644
--- a/packages/core/testing/src/r3_test_bed_compiler.ts
+++ b/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');
 }