feat(core): add task tracking to Testability #16863

heathkit · 2017-05-18T06:56:15Z

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

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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

juliemr · 2017-05-18T21:51:06Z

packages/core/test/testability/testability_spec.ts

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


Thanks for cleaning up the old AsyncTestCompleter business.

juliemr · 2017-05-18T22:25:41Z

packages/core/test/testability/testability_spec.ts

         }));

      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(() => {


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

juliemr · 2017-05-18T22:32:49Z

packages/core/test/testability/testability_spec.ts

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


expect(tasks.length).toEqual(1)

juliemr · 2017-05-18T23:13:02Z

packages/core/test/testability/testability_spec.ts

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


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

juliemr · 2017-05-18T23:14:25Z

packages/core/src/testability/testability.ts

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


Could it be a number|Timer then?

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 ¯_(ツ)_/¯

juliemr · 2017-05-18T23:17:18Z

packages/core/src/testability/testability.ts

    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);


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

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.

juliemr · 2017-05-18T23:41:57Z

packages/core/src/testability/testability.ts

+   * @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.


... and no further updates will be issued.

juliemr · 2017-05-22T20:03:47Z

LGTM, cc @vikerman for additional approval.

heathkit · 2017-05-22T23:59:18Z

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 · 2017-05-30T21:07:43Z

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

heathkit · 2017-06-02T19:09:03Z

Added an integration test for whenStable() timeout.

juliemr · 2017-06-02T19:09:46Z

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

mhevery · 2017-06-05T18:23:29Z

tools/public_api_guard/core/core.d.ts

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


UpdateCallback does not seem to be exported?

mhevery · 2017-06-05T18:27:04Z

packages/core/src/testability/testability.ts

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


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).

IgorMinar

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?

IgorMinar · 2017-06-15T22:55:11Z

tools/cjs-jasmine/index-tools.ts

@@ -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');


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

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

IgorMinar · 2017-06-15T22:58:29Z

packages/examples/core/testability/ts/whenStable/e2e_test/testability_example_spec.ts

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


it what? '' :)

oops, fixed :)

IgorMinar · 2017-06-15T23:01:00Z

packages/core/src/testability/testability.ts

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


"for application to be stable.."

IgorMinar · 2017-06-15T23:03:59Z

packages/core/src/testability/testability.ts

+   */
+  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!');


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

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.

IgorMinar · 2017-06-16T00:13:49Z

packages/core/src/testability/testability.ts

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

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


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?

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.

IgorMinar · 2017-06-16T00:16:23Z

tools/public_api_guard/core/core.d.ts

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

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


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.

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.

IgorMinar · 2017-06-16T00:17:05Z

tools/public_api_guard/core/core.d.ts

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

+/** @experimental */
+export interface PendingMacrotask {


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

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

IgorMinar · 2017-06-16T00:17:23Z

tools/public_api_guard/core/core.d.ts

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

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


inline (as above)?

heathkit · 2017-06-16T19:19:56Z

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.

ngbot · 2018-03-14T00:23:47Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     missing required label: "PR target: *"
     status "ci/circleci: build" is failing
     status "continuous-integration/travis-ci/pr" is failing
     status "google3" is 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

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

IgorMinar · 2018-03-14T00:26:11Z

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

kara · 2018-03-14T02:52:32Z

presubmit

IgorMinar · 2018-03-14T05:45:44Z

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

IgorMinar · 2018-03-14T06:41:20Z

@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.

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 · 2018-03-14T08:13:09Z

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

IgorMinar · 2018-03-14T15:40:58Z

awesome! thanks @heathkit

IgorMinar · 2018-03-14T15:41:42Z

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

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

angular-automatic-lock-bot · 2019-09-13T10:22:31Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

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

googlebot added the cla: yes label May 18, 2017

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

juliemr requested review from juliemr and vikerman May 18, 2017 18:34

juliemr assigned heathkit May 18, 2017

juliemr added area: testing Issues related to Angular testing features, such as TestBed feature Issue that requests a new feature labels May 18, 2017

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

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

juliemr reviewed May 18, 2017

View reviewed changes

heathkit force-pushed the task-tracking branch 2 times, most recently from a5333f8 to 90629ba Compare May 22, 2017 08:43

juliemr approved these changes May 22, 2017

View reviewed changes

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

vikerman approved these changes May 30, 2017

View reviewed changes

juliemr requested a review from IgorMinar June 2, 2017 19:09

mhevery suggested changes Jun 5, 2017

View reviewed changes

heathkit force-pushed the task-tracking branch from a1bf8ef to 8885443 Compare June 6, 2017 23:56

IgorMinar suggested changes Jun 16, 2017

View reviewed changes

heathkit force-pushed the task-tracking branch from 8885443 to b85914f Compare June 16, 2017 19:19

IgorMinar approved these changes Mar 14, 2018

View reviewed changes

IgorMinar added the target: major This PR is targeted for the next major release label Mar 14, 2018

heathkit force-pushed the task-tracking branch from 6cd35ef to dabf0d9 Compare March 14, 2018 07:36

heathkit force-pushed the task-tracking branch from dabf0d9 to 3993b42 Compare March 14, 2018 07:48

kara closed this in 37fedd0 Mar 14, 2018

heathkit mentioned this pull request Mar 14, 2018

WIP feat(core): Allow configuring enabled Zones via sessionStorage #17390

Closed

3 tasks

robwormald mentioned this pull request Apr 18, 2018

NoopZone throws errors when Zone.js is not included #23428

Closed

heathkit mentioned this pull request Jun 18, 2018

Feature Request: Testability API changes angular/angular.js#15585

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

feat(core): add task tracking to Testability #16863

feat(core): add task tracking to Testability #16863

Conversation

heathkit commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliemr commented May 22, 2017

heathkit commented May 22, 2017 • edited Loading

vikerman commented May 30, 2017

heathkit commented Jun 2, 2017

juliemr commented Jun 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorMinar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heathkit Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heathkit Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heathkit commented Jun 16, 2017

ngbot bot commented Mar 14, 2018

IgorMinar left a comment

Choose a reason for hiding this comment

IgorMinar commented Mar 14, 2018

kara commented Mar 14, 2018

IgorMinar commented Mar 14, 2018

IgorMinar commented Mar 14, 2018

heathkit commented Mar 14, 2018

IgorMinar commented Mar 14, 2018

IgorMinar commented Mar 14, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

heathkit commented May 22, 2017 •

edited

Loading

heathkit Jun 16, 2017 •

edited

Loading

heathkit Jun 16, 2017 •

edited

Loading