New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(tests): manage asynchronous tests using zones #7735

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@juliemr
Member

juliemr commented Mar 23, 2016

Instead of returning a promise, use the async function to wrap
tests. This will run the test inside a zone which does not complete
the test until all asynchronous tasks have been completed.

async may be used with the inject function, or separately.

BREAKING CHANGE:

injectAsync is now deprecated. Instead, use the async function
to wrap any asynchronous tests.

Before:

it('should wait for returned promises', injectAsync([FancyService], (service) => {
  return service.getAsyncValue().then((value) => { expect(value).toEqual('async value'); });
}));

it('should wait for returned promises', injectAsync([], () => {
  return somePromise.then(() => { expect(true).toEqual(true); });
}));

After:

it('should wait for returned promises', async(inject([FancyService], (service) => {
  service.getAsyncValue().then((value) => { expect(value).toEqual('async value'); });
})));

// Note that if there is no injection, we no longer need `inject` OR `injectAsync`.
it('should wait for returned promises', async(() => {
  somePromise.then() => { expect(true).toEqual(true); });
}));
@juliemr

This comment has been minimized.

Member

juliemr commented Mar 23, 2016

This is blocked until zone.js treats XHRs as a macrotask.

@@ -130,6 +130,7 @@ export class InjectSetupWrapper {
return new FunctionWithParamTokens(tokens, fn, false, this._providers);

This comment has been minimized.

@vikerman

vikerman Mar 24, 2016

Contributor

Can we use this opportunity to remove FunctionWithParamTokens from regular paths(and handle it temporarily in till injectAsync is completely removed) since we no longer have a sync and async variations of it.

So the new inject() would just be something like

function inject(tokens, fn): Function {
  return () => {
     var params = tokens.map(t => injector.get(t));
     return FunctionWrapper.apply(fn, params);  
  } 
}

This way inject can be a generic mechanism to execute a function with injected parameters and not return the special FunctionWithParams type.

This comment has been minimized.

@juliemr

juliemr Apr 4, 2016

Member

I don't think that we necessarily need to get rid of FunctionWithParamTokens - it's useful to store information about what we want to test and it's not tied to Jasmine.

In particular, we need to store information about additional providers somewhere so that we can do:

it('only uses these providers once', withProviders([MyService]).inject([MyService], (service) => {
 ...
});

We could pass the providers around everywhere, but I think it's clearer to store them on an object.

This comment has been minimized.

@vikerman

vikerman Apr 5, 2016

Contributor

Understood. Agreed.

@@ -161,6 +164,15 @@ export function injectAsync(tokens: any[], fn: Function): FunctionWithParamToken
return new FunctionWithParamTokens(tokens, fn, true);
}
export function async(fn: Function | FunctionWithParamTokens) {
if (fn instanceof FunctionWithParamTokens) {

This comment has been minimized.

@vikerman

vikerman Mar 24, 2016

Contributor

Can we just have it return a function that just creates the async zone right here and run "fn" as part of it? That way we don't have to have the special FunctionWithParamTokens handling later. This same async method could probably be used outside our specialized jasmine wrapper also in another test framework too.

This comment has been minimized.

@vikerman

vikerman Mar 24, 2016

Contributor

Hmm - Maybe it should return a function that take in the 'done' parameter that can directly be run as a Jasmine async test?

async(fn: Function): Function {
return (done) => {
// Create async test zone and run fn.
}
}

This comment has been minimized.

@juliemr

juliemr Apr 4, 2016

Member

After thinking about it a bit - I think that having a special FunctionWithParamTokens object actually makes it easier to reuse this outside of Jasmine. Otherwise, we're baking in assumptions about what the done and done.fail functions look like and how the test framework handles asynchronous code. FunctionWithParamTokens lets the framework adapter do whatever it needs to, and it has all the information (tokens, additional parameters, whether the thing is asynchronous or not) nicely stored.

This comment has been minimized.

@juliemr

juliemr Apr 4, 2016

Member

We don't want to create the async zone right away, we want to create it when this it block is run. So for the it('does stuff', async(fn)) call to work, we need to to just store the info that it will be async for later.

This comment has been minimized.

@vikerman

vikerman Apr 5, 2016

Contributor

This won't create a zone immediately but returns a function which will do it when invoked later. I understand the use of FunctionWithParamTokens better now and yes tying with the done is not a good idea.

@juliemr

This comment has been minimized.

Member

juliemr commented Apr 5, 2016

This is now using the latest zones release, and no longer blocked!

@juliemr

This comment has been minimized.

Member

juliemr commented Apr 5, 2016

cc @mhevery @IgorMinar if you want to review!

@@ -161,6 +164,15 @@ export function injectAsync(tokens: any[], fn: Function): FunctionWithParamToken
return new FunctionWithParamTokens(tokens, fn, true);
}
export function async(fn: Function | FunctionWithParamTokens) {

This comment has been minimized.

@vikerman

vikerman Apr 5, 2016

Contributor

Does this need a return type?

This comment has been minimized.

@juliemr

juliemr Apr 5, 2016

Member

done.

@@ -10,7 +10,7 @@ import {
tick,

This comment has been minimized.

@vikerman

vikerman Apr 5, 2016

Contributor

Do we have a test where there are async tasks scheduled in the test that are resolved successfully? I know we rely on Zone tests but should we add separate tests for different kind of async tasks and async tasks within tasks?

This comment has been minimized.

@juliemr

juliemr Apr 5, 2016

Member

All the TestComponentBuilder tests are all using async calls - but I think it's reasonable to double-check some of the logic from the AsyncZoneSpec here even though it's already tested in the zone.js repo - I've added a section to group the together.

@vikerman

This comment has been minimized.

Contributor

vikerman commented Apr 5, 2016

LGTM

@juliemr

This comment has been minimized.

Member

juliemr commented Apr 6, 2016

Looks like new zones may be causing an issue with the http code - I'm seeing errors in http://localhost:8001/playground/src/http/. Investigating...

done();
}, 0);
});
it('should run async tests with timeouts',

This comment has been minimized.

@IgorMinar

IgorMinar Apr 6, 2016

Member

timeouts -> tasks (timeout is just one kind of task, but it's well representative of the whole class)

This comment has been minimized.

@juliemr

juliemr Apr 7, 2016

Member

done.

@IgorMinar

This comment has been minimized.

Member

IgorMinar commented Apr 6, 2016

LGTM once the test failure is resolved. Good job Julie!

@mhevery

This comment has been minimized.

Member

mhevery commented Apr 6, 2016

LGTM

@@ -326,16 +290,16 @@ export function main() {
it('should fail when a returned promise is rejected', (done) => {
var itPromise = patchJasmineIt();
it('should fail with an error from a promise', injectAsync([], () => {
it('should fail with an error from a promise', async(inject([], () => {
var deferred = PromiseWrapper.completer();
var p = deferred.promise.then(() => { expect(1).toEqual(2); });
deferred.reject('baz');
return p;

This comment has been minimized.

@IgorMinar

IgorMinar Apr 6, 2016

Member

should we clean up all the return values?

they are harmless but people will keep on copy&pasting existing tests and it will take forever to remove them. We could have a team-wide fixit after tobias' change lands.

This comment has been minimized.

@juliemr

juliemr Apr 7, 2016

Member

done from this file.

@IgorMinar

This comment has been minimized.

Member

IgorMinar commented Apr 6, 2016

one more thing, do we really have only this few async injected tests? I'd expect hundreds of them. Isn't that surprising?

@juliemr

This comment has been minimized.

Member

juliemr commented Apr 6, 2016

Most of our tests are still using testing_internal, which uses the old AsyncTestCompleter. I have an issue to remove that when possible: #5443

@IgorMinar

This comment has been minimized.

Member

IgorMinar commented Apr 6, 2016

@juliemr I see. that explains it. thanks!

@juliemr

This comment has been minimized.

Member

juliemr commented Apr 7, 2016

The Travis error is a real problem with my changes in zone.js. I'm going to do a new zone release, then clean this up.

@juliemr

This comment has been minimized.

Member

juliemr commented Apr 7, 2016

Ok, new zones is out, travis is happy! Merge-master, please add to your queue :)

@IgorMinar

This comment has been minimized.

Member

IgorMinar commented Apr 7, 2016

@juliemr there is merge conflict. can you please rebase?

@juliemr

This comment has been minimized.

Member

juliemr commented Apr 8, 2016

Done.

@mhevery

This comment has been minimized.

Member

mhevery commented Apr 8, 2016

Hi @juliemr I have removed the merge label, because it seems to be passing tests which should be failing. can we discuss?

mhevery added a commit to mhevery/angular that referenced this pull request Apr 8, 2016

feat(tests): manage asynchronous tests using zones
Instead of using injectAsync and returning a promise, use the `async` function
to wrap tests. This will run the test inside a zone which does not complete
the test until all asynchronous tasks have been completed.

`async` may be used with the `inject` function, or separately.

BREAKING CHANGE:

`injectAsync` is now deprecated. Instead, use the `async` function
to wrap any asynchronous tests.

Before:
```
it('should wait for returned promises', injectAsync([FancyService], (service) => {
  return service.getAsyncValue().then((value) => { expect(value).toEqual('async value'); });
}));

it('should wait for returned promises', injectAsync([], () => {
  return somePromise.then(() => { expect(true).toEqual(true); });
}));
```

After:
```
it('should wait for returned promises', async(inject([FancyService], (service) => {
  service.getAsyncValue().then((value) => { expect(value).toEqual('async value'); });
})));

// Note that if there is no injection, we no longer need `inject` OR `injectAsync`.
it('should wait for returned promises', async(() => {
  somePromise.then() => { expect(true).toEqual(true); });
}));
```

Closes angular#7735
@juliemr

This comment has been minimized.

Member

juliemr commented Apr 8, 2016

We are blocking this because zones doesn't handle this properly on node.js yes, so some tests run with gulp test.unit.cjs can't fail!

Pending a fix in zone.js...

@juliemr

This comment has been minimized.

Member

juliemr commented Apr 14, 2016

With the new zone release, this is unblocked! Working on rebasing now.

@juliemr

This comment has been minimized.

Member

juliemr commented Apr 14, 2016

OK! This is rebased and ready to go again. @mhevery or @IgorMinar can I get another ok?

Note that the example from https://github.com/mhevery/angular/tree/async-not-working still won't work without modification because it is importing from angular2/testing_internal instead of angular2/testing.

@juliemr juliemr removed their assignment Apr 14, 2016

@juliemr juliemr referenced this pull request Apr 14, 2016

Closed

WIP - Zone Async Spec #7604

@@ -161,6 +164,17 @@ export function injectAsync(tokens: any[], fn: Function): FunctionWithParamToken
return new FunctionWithParamTokens(tokens, fn, true);
}

This comment has been minimized.

@vikerman

vikerman Apr 14, 2016

Contributor

Can we add a doc string for this?

This comment has been minimized.

@juliemr

juliemr Apr 14, 2016

Member

done.

feat(tests): manage asynchronous tests using zones
Instead of using injectAsync and returning a promise, use the `async` function
to wrap tests. This will run the test inside a zone which does not complete
the test until all asynchronous tasks have been completed.

`async` may be used with the `inject` function, or separately.

BREAKING CHANGE:

`injectAsync` is now deprecated. Instead, use the `async` function
to wrap any asynchronous tests.

Before:
```
it('should wait for returned promises', injectAsync([FancyService], (service) => {
  return service.getAsyncValue().then((value) => { expect(value).toEqual('async value'); });
}));

it('should wait for returned promises', injectAsync([], () => {
  return somePromise.then(() => { expect(true).toEqual(true); });
}));
```

After:
```
it('should wait for returned promises', async(inject([FancyService], (service) => {
  service.getAsyncValue().then((value) => { expect(value).toEqual('async value'); });
})));

// Note that if there is no injection, we no longer need `inject` OR `injectAsync`.
it('should wait for returned promises', async(() => {
  somePromise.then() => { expect(true).toEqual(true); });
}));
```
@IgorMinar

This comment has been minimized.

Member

IgorMinar commented Apr 15, 2016

@juliemr should we deprecate testing_internal? it seems that it's dangerous to use it now if you don't return the promise or call the async completer. no?

@juliemr

This comment has been minimized.

Member

juliemr commented Apr 18, 2016

@IgorMinar I would like to, but we need to figure out what the ts2dart plan is.

@schippie

This comment has been minimized.

schippie commented Apr 26, 2016

@juliemr I wanted to make a quick note as it is not mentioned within the changelog.md
That its important to include the 'node_modules/zone.js/dist/async-test.js', within your karma config / tests. Else you will experience a lot of red errors in your tests like i had.

8490921#diff-c4667ceccd7e91092048e752850fab9cR23

@juliemr

This comment has been minimized.

Member

juliemr commented Apr 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment