fix(core): do not invoke jasmine done callback multiple times with waitForAsync

devversion · devversion · commit 4e77c7fbf38f · 2022-07-18T17:20:20.000Z
Currently tests written using `waitForAsync` would be prone to Jasmine warnings or errors (depending on the version) for tests incorrectly invoking asynchronous jasmine `done` callbacks multiple times. This can happen because the async test zone logic schedules the `done` callback to be called using `setTimeout`, but this could be invoked multiple times, causing multiple `done` invocations to be scheduled. Most of the issues have been resolved with #45025, but it does not solve the case of multiple tasks finished and callbacks being scheduled. Technically, the current logic is built in way that _should_ result in `_finishCallbackIfDone` and eventually the `done` callback to be invoked at maximium once. This is unfortunately not the case in some rather advanced/unexpected scenarios (like our AngularJS upgrade tests) where the scenario is the following (and microtasks from before the actual `waitForAsync` spec are still completing -- which is valid): ``` 1. A test `beforeEach` schedules a microtask in the ProxyZone. 2. An actual empty `it` spec executes in the AsyncTestZone` (using e.g. `waitForAsync`). 3. The `onInvoke` invokes `_finishCallbackIfDone` because the spec runs synchronously. 4. We wait the scheduled timeout (see below) to account for unhandled promises. 5. The microtask from (1) finishes and `onHasTask` is invoked. --> We register a second `_finishCallbackIfDone` even though we have scheduled a timeout. --> we execute the `done` callback twice because the async zone spec state is "stable" ```
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,8 @@ class AsyncTestZoneSpec implements ZoneSpec {
   _pendingMacroTasks: boolean = false;
   _alreadyErrored: boolean = false;
   _isSync: boolean = false;
+  _existingFinishTimer: ReturnType<typeof setTimeout>|null = null;
+
   entryFunction: Function|null = null;
   runZone = Zone.current;
   unresolvedChainedPromiseCount = 0;
@@ -32,11 +34,31 @@ class AsyncTestZoneSpec implements ZoneSpec {
   }
 
   _finishCallbackIfDone() {
+    // NOTE: Technically the `onHasTask` could fire together with the initial synchronous
+    // completion in `onInvoke`. `onHasTask` might call this method when it captured e.g.
+    // microtasks in the proxy zone that now complete as part of this async zone run.
+    // Consider the following scenario:
+    //    1. A test `beforeEach` schedules a microtask in the ProxyZone.
+    //    2. An actual empty `it` spec executes in the AsyncTestZone` (using e.g. `waitForAsync`).
+    //    3. The `onInvoke` invokes `_finishCallbackIfDone` because the spec runs synchronously.
+    //    4. We wait the scheduled timeout (see below) to account for unhandled promises.
+    //    5. The microtask from (1) finishes and `onHasTask` is invoked.
+    //    --> We register a second `_finishCallbackIfDone` even though we have scheduled a timeout.
+
+    // If the finish timeout from below is already scheduled, terminate the existing scheduled
+    // finish invocation, avoiding calling `jasmine` `done` multiple times. *Note* that we would
+    // want to schedule a new finish callback in case the task state changes again.
+    if (this._existingFinishTimer !== null) {
+      clearTimeout(this._existingFinishTimer);
+      this._existingFinishTimer = null;
+    }
+
     if (!(this._pendingMicroTasks || this._pendingMacroTasks ||
           (this.supportWaitUnresolvedChainedPromise && this.isUnresolvedChainedPromisePending()))) {
-      // We do this because we would like to catch unhandled rejected promises.
+      // We wait until the next tick because we would like to catch unhandled promises which could
+      // cause test logic to be executed. In such cases we cannot finish with tasks pending then.
       this.runZone.run(() => {
-        setTimeout(() => {
+        this._existingFinishTimer = setTimeout(() => {
           if (!this._alreadyErrored && !(this._pendingMicroTasks || this._pendingMacroTasks)) {
             this.finishCallback();
           }
@@ -116,7 +138,6 @@ class AsyncTestZoneSpec implements ZoneSpec {
       this._isSync = true;
       return parentZoneDelegate.invoke(targetZone, delegate, applyThis, applyArgs, source);
     } finally {
-      const afterTaskCounts: any = (parentZoneDelegate as any)._taskCounts;
       // We need to check the delegate is the same as entryFunction or not.
       // Consider the following case.
       //
@@ -245,7 +266,7 @@ Zone.__load_patch('asynctest', (global: any, Zone: ZoneType, api: _ZonePrivate)
             // Need to restore the original zone.
             if (proxyZoneSpec.getDelegate() == testZoneSpec) {
               // Only reset the zone spec if it's
-              // sill this one. Otherwise, assume
+              // still this one. Otherwise, assume
               // it's OK.
               proxyZoneSpec.setDelegate(previousDelegate);
             }
diff --git a/packages/zone.js/test/zone-spec/async-test.spec.ts b/packages/zone.js/test/zone-spec/async-test.spec.ts
@@ -186,6 +186,44 @@ describe('AsyncTestZoneSpec', function() {
     }, 50);
   });
 
+  it('should not call done multiple times when proxy zone captures previously ' +
+         'captured microtasks',
+     (done) => {
+       const ProxyZoneSpec = (Zone as any)['ProxyZoneSpec'];
+       const proxyZoneSpec = new ProxyZoneSpec(null) as ProxyZoneSpec;
+       const proxyZone = Zone.current.fork(proxyZoneSpec);
+
+       // This simulates a simple `beforeEach` patched, running in the proxy zone,
+       // but not necessarily waiting for the promise to be resolved. This can
+       // be the case e.g. in the AngularJS upgrade tests where the bootstrap is
+       // performed in the before each, but the waiting is done in the actual `it` specs.
+       proxyZone.run(() => {
+         Promise.resolve().then(() => {});
+       });
+
+       let doneCalledCount = 0;
+       const testFn = () => {
+         // When a test executes with `waitForAsync`, the proxy zone delegates to the async
+         // test zone, potentially also capturing tasks leaking from `beforeEach`.
+         proxyZoneSpec.setDelegate(testZoneSpec);
+       };
+
+       const testZoneSpec = new AsyncTestZoneSpec(() => {
+         // reset the proxy zone delegate after test completion.
+         proxyZoneSpec.setDelegate(null);
+         doneCalledCount++;
+       }, () => done.fail('Error occurred in the async test zone.'), 'name');
+
+       const atz = Zone.current.fork(testZoneSpec);
+       atz.run(testFn);
+
+       setTimeout(() => {
+         expect(doneCalledCount).toBe(1);
+         done();
+       }, 50);
+     });
+
+
   describe('event tasks', ifEnvSupports('document', () => {
              let button: HTMLButtonElement;
              beforeEach(function() {
@@ -359,7 +397,10 @@ describe('AsyncTestZoneSpec', function() {
         },
         (err: Error) => {
           expect(err.message).toEqual('Uncaught (in promise): my reason');
-          done();
+          // Without the `runInTestZone` function, the callback continues to execute
+          // in the async test zone. We don't want to trigger new tasks upon
+          // the failure callback already being invoked (`jasmine.done` schedules tasks)
+          Zone.root.run(() => done());
         },
         'name');
 
