iframe-proxy

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

Thanks for cleaning up the old AsyncTestCompleter business.

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

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

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

expect(tasks.length).toEqual(1)

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

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

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

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

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

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

UpdateCallback does not seem to be exported?

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

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

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

it what? '' :)

oops, fixed :)

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

"for application to be stable.."

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

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

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

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

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

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.

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

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 Label used to distinguish feature request from other issues 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

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

heathkit commented May 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliemr commented May 22, 2017

Uh oh!

heathkit commented May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vikerman commented May 30, 2017

Uh oh!

heathkit commented Jun 2, 2017

Uh oh!

juliemr commented Jun 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IgorMinar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heathkit Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heathkit commented May 22, 2017 •

edited

Loading

heathkit Jun 16, 2017 •

edited

Loading

heathkit Jun 16, 2017 •

edited

Loading