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

feat(test): can handle non zone aware task in promise within AsyncTestZoneSpec #1014

Merged
merged 1 commit into from
Feb 10, 2018

Conversation

JiaLiPassion
Copy link
Collaborator

@JiaLiPassion JiaLiPassion commented Feb 7, 2018

fix #1013.
currently if there are some async task which is not zone aware, AsyncTestZoneSpec will not handle it. such as,

it('test jsonp', () => {
 let finished = false; 
  const asyncTestZoneSpec = new AsyncTestZoneSpec(() => {
     expect(finished).toBe(true);
   }, () => {}, 'async');
   asyncTestZone.run(() => {
     jsonp(url, () =>{
        // success callback
        finished = true;
     });
   });
}); 
 

this case will fail because jsonp is not a zone aware async operation, so finished will be still be false, when AsyncTestZone think all microTasks and macroTasks have been done (but jsonp is still running).

We can't handle all non zone aware async task, so when such kind of issue occurs, we can help user use Zone.prototype.scheduleMicroTask or Zone.prototype.scheduleMacroTask to handle them.

But some case is much more confusing the user with promise, such as,

it('test jsonp', () => {
 let finished = false; 
  const asyncTestZoneSpec = new AsyncTestZoneSpec(() => {
     expect(finished).toBe(true);
   }, () => {}, 'async');
   asyncTestZone.run(() => {
     new Promise((res, rej) => {
       jsonp(url, () =>{
        // success callback and resolve the promise
        res();
       });
     }).then(() => {
        finished = true;
     });
   });
}); 
 

now even with the promise and promise.then, the case still failed. Because

  • jsonp is not zone aware.
  • so new Promise((res, rej) => jsonp(...)) will not trigger any async operation (promise itself it not async).
  • promise.then is also not async until the promise is resolved.

But the asyncTestZone will think all async finished (in this case, no async happened) before promise.then is invoked and schedule a microTask. So the test case will still fail.

We can still handle this issue by make those non zone aware task to zone aware, but it will cost time, and in most case, user call promise.then, they believe promise will be resolved or rejected, so I think zone.js may handle it more friendly.

in this PR, if there is such case.

  • promise is chained (means promise.then is called)
  • parent promise is unresolved.

I will increase a chainedPromise number in AsyncTestZoneSpec. And when the chainedPromise's parent resolved and schedule a microTask, I will decrease the number, and when AsyncTestZoneSpec check whether should call finish or not not only check hasPendingMicroTask/MacoTask but also check is there any promise.then waiting for non zone aware promise.

it('test jsonp', () => {
 let finished = false; 
  const asyncTestZoneSpec = new AsyncTestZoneSpec(() => {
     expect(finished).toBe(true);
   }, () => {}, 'async');
   asyncTestZone.run(() => {
     new Promise((res, rej) => {
       jsonp(url, () =>{
        // success callback and resolve the promise
        res();  // 2. here, when parent promise resolved, and a microTask is scheduled, decrease the chainedPromise number.
       });
     }).then(() => {  // 1. here, when promise.then is called, increase the chainedPromise number
        finished = true; 
     });
   });
}); 

@mhevery , could you take a look about this idea is ok or not? thank you!

@JiaLiPassion JiaLiPassion changed the title feat(test): can handle non zone aware task in promise within AsyncTestZoneSpec WIP(test): can handle non zone aware task in promise within AsyncTestZoneSpec Feb 7, 2018
mhevery
mhevery previously approved these changes Feb 8, 2018
@mhevery
Copy link
Contributor

mhevery commented Feb 8, 2018

LGTM, what is going on with the Travis?

@JiaLiPassion
Copy link
Collaborator Author

@mhevery, thank you for review, travis failed for almost all pr because current test case or jasmine patch not compatible with newest version of jasmine.

I have fixed jasmine to 2.4.1 in this pr #994, Travis should pass.

I will continue to find out how to compatible with jasmine new version.

@JiaLiPassion JiaLiPassion force-pushed the promise-for-test branch 2 times, most recently from 581146b to c65d81a Compare February 8, 2018 06:18
@JiaLiPassion JiaLiPassion changed the title WIP(test): can handle non zone aware task in promise within AsyncTestZoneSpec feat(test): can handle non zone aware task in promise within AsyncTestZoneSpec Feb 8, 2018
@JiaLiPassion JiaLiPassion force-pushed the promise-for-test branch 2 times, most recently from 7e2eb4a to aa4c16c Compare February 9, 2018 04:57
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.

HasTaskState says there are no micro and macro tasks but I think there are outstanding tasks. Who is right?
3 participants