fix(zone.js): async-test should only call done once (#45025)

JiaLiPassion · alxhub · commit dea7234a76f6 · 2022-02-09T10:18:56.000-08:00
`AsyncTestZoneSpec` triggers jasmine `done()` function multiple times and causes warning ``` An asynchronous function called its 'done' callback more than once. This is a bug in the spec, beforeAll, beforeEach, afterAll, or afterEach function in question. This will be treated as an error in a future version. See<https://jasmine.github.io/tutorials/upgrading_to_Jasmine_4.0#deprecations-due-to-calling-done-multiple-times> for more information ``` The reproduce case will be running some `Zone.run()` inside `waitForAsync()`. ``` it('multiple done', waitForAsync(() => { Zone.current.run(() => {}); Zone.current.run(() => {}); })); ``` The reason the `done()` is called in the `onInvoke()` hook is to handle the case that the testBody is totally sync, but we should only do this check for the entry function not for all `Zone.run()` scenario. Another issue is if we run nested zone inside `waitForAsync()`, the `onHasTask()` hook will be triggered multiple times, and cause `done()` be triggered multiple times, so we need to only trigger the `done()` when the zone is `AsyncTestZone`. PR Close #45025
diff --git a/packages/zone.js/lib/zone-spec/async-test.ts b/packages/zone.js/lib/zone-spec/async-test.ts
@@ -13,6 +13,7 @@ class AsyncTestZoneSpec implements ZoneSpec {
   _pendingMacroTasks: boolean = false;
   _alreadyErrored: boolean = false;
   _isSync: boolean = false;
+  entryFunction: Function|null = null;
   runZone = Zone.current;
   unresolvedChainedPromiseCount = 0;
 
@@ -108,13 +109,25 @@ class AsyncTestZoneSpec implements ZoneSpec {
   onInvoke(
       parentZoneDelegate: ZoneDelegate, currentZone: Zone, targetZone: Zone, delegate: Function,
       applyThis: any, applyArgs?: any[], source?: string): any {
-    let previousTaskCounts: any = null;
+    if (!this.entryFunction) {
+      this.entryFunction = delegate;
+    }
     try {
       this._isSync = true;
       return parentZoneDelegate.invoke(targetZone, delegate, applyThis, applyArgs, source);
     } finally {
       const afterTaskCounts: any = (parentZoneDelegate as any)._taskCounts;
-      if (this._isSync) {
+      // We need to check the delegate is the same as entryFunction or not.
+      // Consider the following case.
+      //
+      // asyncTestZone.run(() => { // Here the delegate will be the entryFunction
+      //   Zone.current.run(() => { // Here the delegate will not be the entryFunction
+      //   });
+      // });
+      //
+      // We only want to check whether there are async tasks scheduled
+      // for the entry function.
+      if (this._isSync && this.entryFunction === delegate) {
         this._finishCallbackIfDone();
       }
     }
@@ -133,6 +146,21 @@ class AsyncTestZoneSpec implements ZoneSpec {
 
   onHasTask(delegate: ZoneDelegate, current: Zone, target: Zone, hasTaskState: HasTaskState) {
     delegate.hasTask(target, hasTaskState);
+    // We should only trigger finishCallback when the target zone is the AsyncTestZone
+    // Consider the following cases.
+    //
+    // const childZone = asyncTestZone.fork({
+    //   name: 'child',
+    //   onHasTask: ...
+    // });
+    //
+    // So we have nested zones declared the onHasTask hook, in this case,
+    // the onHasTask will be triggered twice, and cause the finishCallbackIfDone()
+    // is also be invoked twice. So we need to only trigger the finishCallbackIfDone()
+    // when the current zone is the same as the target zone.
+    if (current !== target) {
+      return;
+    }
     if (hasTaskState.change == 'microTask') {
       this._pendingMicroTasks = hasTaskState.microTask;
       this._finishCallbackIfDone();
diff --git a/packages/zone.js/test/zone-spec/async-test.spec.ts b/packages/zone.js/test/zone-spec/async-test.spec.ts
@@ -132,6 +132,55 @@ describe('AsyncTestZoneSpec', function() {
     });
   });
 
+  it('should not call done multiple times in sync test', (done) => {
+    const testFn = () => {
+      Zone.current.run(() => {});
+      Zone.current.run(() => {});
+    };
+    let doneCalledCount = 0;
+    const testZoneSpec = new AsyncTestZoneSpec(() => {
+      doneCalledCount++;
+    }, () => {}, 'name');
+
+    const atz = Zone.current.fork(testZoneSpec);
+
+    atz.run(testFn);
+    setTimeout(() => {
+      expect(doneCalledCount).toBe(1);
+      done();
+    });
+  });
+
+  it('should not call done multiple times in async test with nested zone', (done) => {
+    const testFn = () => {
+      Promise.resolve(1).then(() => {});
+    };
+    let doneCalledCount = 0;
+    const testZoneSpec = new AsyncTestZoneSpec(() => {
+      doneCalledCount++;
+    }, () => {}, 'name');
+
+    const atz = Zone.current.fork(testZoneSpec);
+    const c1 = atz.fork({
+      name: 'child1',
+      onHasTask: (delegate, current, target, hasTaskState) => {
+        return delegate.hasTask(target, hasTaskState);
+      }
+    });
+    const c2 = c1.fork({
+      name: 'child2',
+      onHasTask: (delegate, current, target, hasTaskState) => {
+        return delegate.hasTask(target, hasTaskState);
+      }
+    });
+
+    c2.run(testFn);
+    setTimeout(() => {
+      expect(doneCalledCount).toBe(1);
+      done();
+    }, 50);
+  });
+
   describe('event tasks', ifEnvSupports('document', () => {
              let button: HTMLButtonElement;
              beforeEach(function() {
