Skip to content
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(core): add task tracking to Testability #16863

Closed
wants to merge 1 commit into from

Conversation

@heathkit
Copy link
Member

commented May 18, 2017

Allow passing an optional timeout to Testability's whenStable(). If
specified, if Angular is not stable before the timeout is hit, the
done callback will be invoked with a list of pending macrotasks.

Also, allows an optional update callback, which will be invoked whenever
the set of pending macrotasks changes. If this callback returns true,
the timeout will be cancelled and the done callback will not be invoked.

Implements #15917

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

Testability.whenStable() will wait an indefinite amount of time for Angular to become stable. Protractor relies on a script timeout to know when waiting has timed out (typically longer than 10 seconds or so).

What is the new behavior?

Users can now pass a timeout to whenStable(). If there are still macrotasks pending when the timeout occurs, the callback will receive a list of pending tasks (provided by the TaskTracking zone spec).

whenStable() also now accepts an optional callback to invoke whenever the state of pending macrotasks changes. If this callback returns true, whenStable() will cancel the pending done callback, allowing users more control over which macrotasks they wait for.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[X] No

@heathkit heathkit force-pushed the heathkit:task-tracking branch from 65c2a6d to a28fdfb May 18, 2017

@googlebot googlebot added the cla: yes label May 18, 2017

@heathkit heathkit changed the title feat(testability): Improvements to the Testability API. feat(core): Improvements to the Testability API. May 18, 2017

@juliemr juliemr requested review from juliemr and vikerman May 18, 2017

@heathkit heathkit force-pushed the heathkit:task-tracking branch 2 times, most recently from 4e0a2c9 to be84dc9 May 18, 2017

@heathkit heathkit changed the title feat(core): Improvements to the Testability API. feat(core): add task tracking to Testability May 18, 2017

scheduleMicroTask(fn);
});
}
import {fakeAsync, tick} from '@angular/core/testing';

This comment has been minimized.

Copy link
@juliemr

juliemr May 18, 2017

Member

Thanks for cleaning up the old AsyncTestCompleter business.

}));

it('should not fire whenstable callbacks synchronously if pending count is 0', () => {
testability.whenStable(execute);
expect(execute).not.toHaveBeenCalled();
});

it('should not call whenstable callbacks when there are pending counts',
inject([AsyncTestCompleter], (async: AsyncTestCompleter) => {
it('should not call whenstable callbacks when there are pending counts', fakeAsync(() => {

This comment has been minimized.

Copy link
@juliemr

juliemr May 18, 2017

Member

As discussed offline, let's make these twoasync tests to increase confidence

This comment has been minimized.

Copy link
@heathkit

heathkit May 20, 2017

Author Member

Done

tick(200);
expect(execute).toHaveBeenCalled();
const tasks = execute.calls.mostRecent().args[1] as PendingMacrotask[];

This comment has been minimized.

Copy link
@juliemr

juliemr May 18, 2017

Member

expect(tasks.length).toEqual(1)

This comment has been minimized.

Copy link
@heathkit

heathkit May 20, 2017

Author Member

Done

let timeout1Done = false;
let timeout2Done = false;
ngZone.run(() => setTimeout(() => timeout1Done = true, 500));
testability.whenStable(execute, 1000, execute2);

This comment has been minimized.

Copy link
@juliemr

juliemr May 18, 2017

Member

Maybe instead of execute2 make a new spy and call it something like updateCallback?

This comment has been minimized.

Copy link
@heathkit

heathkit May 20, 2017

Author Member

Done


interface WaitCallback {
// Needs to be 'any' - setTimeout returns a number according to ES6, but
// on NodeJS it returns a Timer.

This comment has been minimized.

Copy link
@juliemr

juliemr May 18, 2017

Member

Could it be a number|Timer then?

This comment has been minimized.

Copy link
@heathkit

heathkit May 20, 2017

Author Member

The problem is Timer is only defined in the NodeJS typings, which is only brought in for the platform-server tsconfig. I can't figure out a type that builds for both browser and server platforms, since in the browser setTimeout returns a number.

Of course, with Zone.js, setTimeout returns the same thing on both platforms - a Zone task. So the typings are wrong, anyway ¯_(ツ)_/¯

if (this.isStable()) {
// Schedules the call backs in a new frame so that it is always async.
scheduleMicroTask(() => {
while (this._callbacks.length !== 0) {
(this._callbacks.pop() !)(this._didWork);
let cb = (this._callbacks.pop() as WaitCallback);

This comment has been minimized.

Copy link
@juliemr

juliemr May 18, 2017

Member

Why is casting here necessary? Shouldn't TS know it's an array of WaitCallbacks?

This comment has been minimized.

Copy link
@heathkit

heathkit May 20, 2017

Author Member

I accidentally dropped the not null assertion !, so I needed a cast. I put it back, but I do find the! a little easy to miss.

* @param timeout Optional. The maximum time to wait for Angular to become stable. If not
* specified, whenStable() will wait forever.
* @param updateCb Optional. If specified, this callback will be invoked whenever the set of
* pending macrotasks changes. If this callback returns true doneCb will not be invoked.

This comment has been minimized.

Copy link
@juliemr

juliemr May 18, 2017

Member

... and no further updates will be issued.

This comment has been minimized.

Copy link
@heathkit

heathkit May 20, 2017

Author Member

Done

@heathkit heathkit force-pushed the heathkit:task-tracking branch 2 times, most recently from a5333f8 to 90629ba May 21, 2017

@juliemr

This comment has been minimized.

Copy link
Member

commented May 22, 2017

LGTM, cc @vikerman for additional approval.

@heathkit heathkit force-pushed the heathkit:task-tracking branch 3 times, most recently from fee9b00 to cf3fc55 May 22, 2017

@heathkit

This comment has been minimized.

Copy link
Member Author

commented May 22, 2017

PTAL - I had to make some slight changes to get CI to pass - updated the public API. Also changed it so you only get an error when the task tracking zone isn't available if you pass a timeout or update callback to whenStable()

@vikerman

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

LGTM - can we add an end to end test with Protractor for this?

@heathkit

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2017

Added an integration test for whenStable() timeout.

@juliemr

This comment has been minimized.

Copy link
Member

commented Jun 2, 2017

I believe Vikram said that Igor should take a look at this - ping @IgorMinar

@juliemr juliemr requested a review from IgorMinar Jun 2, 2017

isStable(): boolean;
whenStable(callback: Function): void;
whenStable(doneCb: DoneCallback, timeout?: number, updateCb?: UpdateCallback): void;

This comment has been minimized.

Copy link
@mhevery

mhevery Jun 5, 2017

Member

UpdateCallback does not seem to be exported?

This comment has been minimized.

Copy link
@heathkit

heathkit Jun 7, 2017

Author Member

Fixed.

}

export type DoneCallback = (didWork: boolean, tasks?: PendingMacrotask[]) => void;
export type UpdateCallback = (tasks: PendingMacrotask[]) => boolean;

This comment has been minimized.

Copy link
@mhevery

mhevery Jun 5, 2017

Member

UpdateCallback and DoneCallback should show up in the API guard. The fact that they don't implies that they are not visible to the developer (ie they are not exported at top level).

This comment has been minimized.

Copy link
@heathkit

heathkit Jun 7, 2017

Author Member

Fixed.

@heathkit heathkit force-pushed the heathkit:task-tracking branch from a1bf8ef to 8885443 Jun 6, 2017

@IgorMinar
Copy link
Member

left a comment

How do we expect developers to receive this update?

I suspect that once they update to karma that depends on this API they will get the missing zone spec error, not understanding what that means or what to do about it.

If they read up on the change in some changelog they'll figure out that they need to add the new zone spec.

If they do add these zone spec, would they know how to remove them in production?

And if they are not removed from production builds, do we understand the performance impact of having task tracking enabled all the time?

It seems to me that the PR as is will create a lot of pain for developers not suspecting or understanding this change. Am I missing something?

@@ -15,6 +15,7 @@ require('zone.js/dist/zone-node.js');
var JasmineRunner = require('jasmine');
var path = require('path');
require('zone.js/dist/long-stack-trace-zone.js');
require('zone.js/dist/task-tracking.js');

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jun 16, 2017

Member

doesn't this mean that task tracking must be enabled in all apps for testability to work?

This comment has been minimized.

Copy link
@heathkit

heathkit Jun 16, 2017

Author Member

No, the timeout and update callback are optional. If those aren't passed, testability should continue to work as before.

describe('times out with list of tasks', () => {
const URL = '/core/testability/ts/whenStable/';

it('', (done) => {

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jun 16, 2017

Member

it what? '' :)

This comment has been minimized.

Copy link
@heathkit

heathkit Jun 16, 2017

Author Member

oops, fixed :)

}

/**
* Wait for angular to be stable with a timeout. If the timeout is hit before Angular becomes

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jun 16, 2017

Member

"for application to be stable.."

This comment has been minimized.

Copy link
@heathkit

heathkit Jun 16, 2017

Author Member

Done

*/
whenStable(doneCb: DoneCallback, timeout?: number, updateCb?: UpdateCallback): void {
if ((timeout || updateCb) && !this.taskTrackingZone) {
throw new Error('Task tracking zone required when using whenStable() with a timeout!');

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jun 16, 2017

Member

If I were a regular developer, I'd have no idea what to do about this error. :-(

This comment has been minimized.

Copy link
@heathkit

heathkit Jun 16, 2017

Author Member

I tried to make it more clear. Also, this API isn't really intended for end users as much as it is for tool developers, like Protractor. On the Protractor side, we'll try to turn this into a more actionable, understandable error.

@@ -67,12 +92,14 @@ export class Testability implements PublicTestability {
});
}

/** @deprecated pending requests are now tracked with zones */

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jun 16, 2017

Member

does this mean that the old way of using this api still works? what happens if you have task tracking enabled and you use older protractor that doesn't have the task tracking support yet? Will it just work?

This comment has been minimized.

Copy link
@heathkit

heathkit Jun 16, 2017

Author Member

Yes, the old way of using the API should still work without errors. If task tracking is enabled but Protractor isn't passing an update callback, it will just work with the current behavior.

@@ -375,6 +375,9 @@ export interface DoCheck {
ngDoCheck(): void;
}

/** @experimental */
export declare type DoneCallback = (didWork: boolean, tasks?: PendingMacrotask[]) => void;

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jun 16, 2017

Member

Do we have to export these types? Can't we just inline them?

It's weird that someone can do import {DoneCallback} from '@angular/core'; - not only that the name is too ambiguous, I'm not sure if we want to be on the hook to support such api if someone decides to create their implementation.

This comment has been minimized.

Copy link
@heathkit

heathkit Jun 16, 2017

Author Member

Yeah. I kind of like having the types for the callbacks available so I know what parameters they need to accept, but it's probably not a good thing to have this in the public API. The Protractor side of this is written in ES5, anyway, so it's not like I'd actually be able to use these types yet.

@@ -698,6 +701,15 @@ export declare const Output: OutputDecorator;
/** @experimental */
export declare const PACKAGE_ROOT_URL: InjectionToken<string>;

/** @experimental */
export interface PendingMacrotask {

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jun 16, 2017

Member

same as above. we should expose as little as possible. Do we need to provide all of this type info?

This comment has been minimized.

Copy link
@heathkit

heathkit Jun 16, 2017

Author Member

Yup, I'll just inline this or make it Function.

@@ -1034,6 +1046,9 @@ export interface TypeDecorator {
export interface TypeProvider extends Type<any> {
}

/** @experimental */
export declare type UpdateCallback = (tasks: PendingMacrotask[]) => boolean;

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jun 16, 2017

Member

inline (as above)?

This comment has been minimized.

Copy link
@heathkit

heathkit Jun 16, 2017

Author Member

see above

@heathkit

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2017

How do we expect developers to receive this update?

I suspect that once they update to karma that depends on this API they will get the missing zone spec error, not understanding what that means or what to do about it.

That should not happen - at least it's not the intention. The error for the missing zone spec is only thrown when someone tries to call whenStable() with an update callback. Protractor's current behavior should not be affected.

The current behavior for Protractor is to pass a done callback and wait forever. The timeout is actually handled by WebDriver's script execution timeout. Moving the timeout and task tracking logic to Testability will allow us to greatly simplify Protractor's browser.waitForAngular

If they read up on the change in some changelog they'll figure out that they need to add the new zone spec.

If they do add these zone spec, would they know how to remove them in production?

That's a good point, which Misko brought up when we talked this idea over with him. The goal is to fix that with #17390, which adds a way to explicitly set which zone specs get loaded by setting flags in session state. The idea is that if NG_DEFER_BOOTSTRAP is in window.name (which is how Protractor signals to AngularJS that it's under test and needs to load mock modules and such), then the task tracking zone is enabled. So users that want to use Protractor's task tracking would need to always include the zone spec (similar to how they include long-stack-trace), but it will only be forked during a Protractor test.

It seems to me that the PR as is will create a lot of pain for developers not suspecting or understanding this change. Am I missing something?

The goal of this PR is to preserve current behavior, but allow us to add task tracking in future Protractor releases. This will provide better debugging information when Protractor times out waiting for stability, and will also enable us to build a more high-level API for waiting on macrotasks. The change should be transparent. If Protractor attempts to use task tracking but the zone isn't present, it can log a warning (or error or whatever we decide is appropriate).

Once #17390 is in we'll be able to continue this work on the Protractor side. We're also planning on doing the same changes to whenStable for AngularJS.

@heathkit heathkit force-pushed the heathkit:task-tracking branch from 8885443 to b85914f Jun 16, 2017

* found in the LICENSE file at https://angular.io/license
*/

// #docregion Component

This comment has been minimized.

Copy link
@gkalpak

gkalpak Mar 7, 2018

Member

Nit: Unused docregion.

This comment has been minimized.

Copy link
@heathkit

heathkit Mar 9, 2018

Author Member

Done

done();
});

expect(element(by.css('.status')).getText()).not.toContain('done');

This comment has been minimized.

Copy link
@gkalpak

gkalpak Mar 7, 2018

Member

Wouldn't it make more sense for this to be moved inside the then() callback above (in order to ensure that whenStable() was called before the timeout was triggered)?

This comment has been minimized.

Copy link
@heathkit

heathkit Mar 9, 2018

Author Member

Good catch, fixed.

@ngbot

This comment has been minimized.

Copy link

commented Mar 14, 2018

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure missing required label: "PR target: *"
    failure status "ci/circleci: build" is failing
    failure status "continuous-integration/travis-ci/pr" is failing
    pending status "google3" is pending
    pending 1 pending code review

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@IgorMinar
Copy link
Member

left a comment

I discussed this with @heathkit and we resolved all the remaining issues. lgtm.

@IgorMinar

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

caretaker note: can you please run a g3 presubmit on this one before merging? thanks!

@kara

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2018

@IgorMinar

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

great. the presubmit passed. @kara can you please merge this? thanks

@IgorMinar

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

@heathkit can you please check why the circleci tests are failing? they are the same unit tests that execute on travis, but they run under bazel which also need the new zone spec configuration. you should be able to reproduce this failure locally.

feat(core): add task tracking to Testability
Allow passing an optional timeout to Testability's whenStable(). If
specified, if Angular is not stable before the timeout is hit, the
done callback will be invoked with a list of pending macrotasks.

Also, allows an optional update callback, which will be invoked whenever
the set of pending macrotasks changes. If this callback returns true,
the timeout will be cancelled and the done callback will not be invoked.

If the optional parameters are not passed, whenStable() will work
as it did before, whether or not the task tracking zone spec is
available.

This change also migrates the Testability unit tests off the deprecated
AsyncTestCompleter.

@heathkit heathkit force-pushed the heathkit:task-tracking branch from dabf0d9 to 3993b42 Mar 14, 2018

@heathkit

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2018

@IgorMinar Thanks, that was it. Updated BUILD.bazel and init_node_spec.ts, circle is passing now.

@IgorMinar

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

awesome! thanks @heathkit

@IgorMinar

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

@kara the previous presubmit is still valid since the fixes that Michael made do not affect g3.

@kara kara closed this in 37fedd0 Mar 14, 2018

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

feat(core): add task tracking to Testability (angular#16863)
Allow passing an optional timeout to Testability's whenStable(). If
specified, if Angular is not stable before the timeout is hit, the
done callback will be invoked with a list of pending macrotasks.

Also, allows an optional update callback, which will be invoked whenever
the set of pending macrotasks changes. If this callback returns true,
the timeout will be cancelled and the done callback will not be invoked.

If the optional parameters are not passed, whenStable() will work
as it did before, whether or not the task tracking zone spec is
available.

This change also migrates the Testability unit tests off the deprecated
AsyncTestCompleter.

PR Close angular#16863
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.