Skip to content
This repository was archived by the owner on Feb 26, 2024. It is now read-only.

fix(proxy): proxyZone should call onHasTask when change delegate, async should call finish only when sync#1030

Merged
mhevery merged 1 commit intoangular:masterfrom
JiaLiPassion:proxy-async
Feb 27, 2018
Merged

fix(proxy): proxyZone should call onHasTask when change delegate, async should call finish only when sync#1030
mhevery merged 1 commit intoangular:masterfrom
JiaLiPassion:proxy-async

Conversation

@JiaLiPassion
Copy link
Copy Markdown
Collaborator

@JiaLiPassion JiaLiPassion commented Feb 26, 2018

fix 4 issues.

  1. proxyZone should call onHasTask when change delegate
  2. AsyncTestZoneSpec should only call finishCallback when the it is pure async.
  3. jasmine internal setTimeout should not in zone.
  4. AsyncTestZoneSpec doesn't handle eventTask correctly.

fix angular/angular#22448, which can be described as the following code. (this issue will also bring some other issues), I have created several cases here, https://github.com/angular/angular/pull/22454/files.

I also created test cases in zone.js to simulate @angular/core/async.

beforeEach(async(() => {
  TestBed.configureTestingModule({
      declarations: [
        AppComponent
      ],
    }).compileComponents();
}));
it('simple timeout', async(() => {
    setTimeout(() => {
      expect(true).toBe(true);
    }, 200);
  }));

the case above will report Error: 'expect' was used when there was no current spec.
Because simple timeout test will not wait for setTimeout finished.

There are several reasons.

  1. ProxyZone should call onHasTask when change delegate, otherwise the new delegateZoneSpec may never get onHasTask triggered.

For example:

  • we have a proxyZone with delegate zoneSpec1.
  • we setTimeout in zoneSpec1
  • and we change the delegate to zoneSpec2.
  • we setTimeout again

The onHasTask will not triggered in zoneSpec2 when call setTimeout because the taskCount is already be 1 which was increased by zoneSpec1.

  1. when we fix the first issue, the case will still fail, because jasmine will call setTimeout to monitor timeout, and this setTimeout will also run in Proxy(AsyncTest)Zone, so there will always a pendingMacroTask to make the test case timeout forever.

  2. When we fix the above 2 issues, there will come out a new problem, https://github.com/angular/zone.js/blob/master/lib/zone-spec/async-test.ts#L84, the finishCallback will triggered twice (one by onHasTask, one by onInvoke). We should only trigger the onInvoke one when the callback is pure sync.

  3. When we fix all those 3 problems, there still one left in angular side, https://github.com/angular/angular/blob/master/packages/core/testing/src/async.ts#L91, async should reset the delegateSpec outside of currentZone.run.

I have created several test cases with async of @angular/core, and I will also create test cases in zone.js side.

@mhevery , please review this one, it seems will bring some impact of AsyncTestZone and async in angular, thank you!

@JiaLiPassion JiaLiPassion reopened this Feb 26, 2018
@JiaLiPassion JiaLiPassion changed the title fix(proxy): proxyZone should call onHasTask when change delegate WIP(proxy): proxyZone should call onHasTask when change delegate, async should call finish only when sync Feb 26, 2018
@JiaLiPassion JiaLiPassion force-pushed the proxy-async branch 3 times, most recently from f80ee44 to 64b523e Compare February 27, 2018 13:19
@JiaLiPassion JiaLiPassion changed the title WIP(proxy): proxyZone should call onHasTask when change delegate, async should call finish only when sync fix(proxy): proxyZone should call onHasTask when change delegate, async should call finish only when sync Feb 27, 2018
@mhevery mhevery merged commit 40b110d into angular:master Feb 27, 2018
@JiaLiPassion JiaLiPassion deleted the proxy-async branch March 15, 2018 06:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]async test will report error when call setTimeout with async beforeEach

3 participants