feat(tests): manage asynchronous tests using zones#7735
feat(tests): manage asynchronous tests using zones#7735juliemr wants to merge 1 commit intoangular:masterfrom
Conversation
|
This is blocked until zone.js treats XHRs as a macrotask. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 is now using the latest zones release, and no longer blocked! |
|
cc @mhevery @IgorMinar if you want to review! |
There was a problem hiding this comment.
Does this need a return type?
|
LGTM |
1ee351a to
c515ec3
Compare
|
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... |
There was a problem hiding this comment.
timeouts -> tasks (timeout is just one kind of task, but it's well representative of the whole class)
|
LGTM once the test failure is resolved. Good job Julie! |
|
LGTM |
There was a problem hiding this comment.
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.
|
one more thing, do we really have only this few async injected tests? I'd expect hundreds of them. Isn't that surprising? |
|
Most of our tests are still using testing_internal, which uses the old |
|
@juliemr I see. that explains it. thanks! |
|
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. |
|
Done. |
|
Hi @juliemr I have removed the |
|
We are blocking this because zones doesn't handle this properly on node.js yes, so some tests run with Pending a fix in zone.js... |
|
With the new zone release, this is unblocked! Working on rebasing now. |
|
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 |
There was a problem hiding this comment.
Can we add a doc string for this?
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); });
}));
```
|
@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? |
|
@IgorMinar I would like to, but we need to figure out what the ts2dart plan is. |
|
@juliemr I wanted to make a quick note as it is not mentioned within the changelog.md |
|
@schippie see #8253 |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Instead of returning a promise, use the
asyncfunction to wraptests. This will run the test inside a zone which does not complete
the test until all asynchronous tasks have been completed.
asyncmay be used with theinjectfunction, or separately.BREAKING CHANGE:
injectAsyncis now deprecated. Instead, use theasyncfunctionto wrap any asynchronous tests.
Before:
After: