feat(*): implement more granular pending task tracking #16603

gkalpak · 2018-06-16T08:11:57Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Refactoring/Internal feature.

What is the current behavior? (You can also link to an open issue here)
Previously, all pending async tasks (tracked via $browser) are treated the same. I.e. things like $$testability.whenStable() and ngMock#$timeout.verifyNoPendingTasks() take all tasks into account.

Yet, in some cases we might be interested in specific tasks only. For example, if one wants to verify there are no pending $timeouts, they don't care if there are other pending tasks, such as $http requests. Similarly, one might want to get notified when all $http requests have completed and does not care about pending promises.

What is the new behavior (if this is a feature change)?
This PR adds support for more granular task tracking, by enabling callers to specify the type of task that is being added/removed from the queue and enabling listeners to be triggered when specific types of tasks are completed (even if there are more pending tasks of different types).

The change is backwards compatible. I.e. calling the affected methods with no explicit task-type, behaves the same as before.

UPDATE: This PR now implements the API suggested in #16603 (comment).

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:
Closes #14336.

gkalpak · 2018-06-16T08:27:52Z

Some work is still needed, but I would like confirm we are itnerested in this change, before investing more time.

TODO:

Add tests for changes in ngMock.
Optional Share code between $interval and ngMock.$interval (e.g. by extracting the relevant bits into a reusable factory). Right now the functionality is replicated in two places with minimal differences (e.g. setTimeout, clearTimeout).
Optional Share task-tracking code between $browser and ngMock.$browser (e.g. by extracting the relevant bits into a reusable factory). Right now, the functionality is replicated in two places with no difference.
~~Investigate flushing $http requests from ngMock/$flushPendingTasks() (even if requests aren't scheduled via $browser.defer()).~~
- We decided not to do this for now.
Consider logging debug messages from ngMock/$timeout.flush() and ngMock/$timeout.verifyNoPendingTasks(), recommending ngMock/$flushPendingTasks() and ngMock/$verifyNoPendingTasks() instead.
- We decided there is little point in doing so if the methods work. What we'll do instead is mention ngMock/$verifyNoPendingTasks() in the error message thrown by ngMock/$timeout.verifyNoPendingTasks() if the pending tasks are not $timeout-related.
Add tests for ngMock/$flushPendingTasks() and ngMock/$verifyNoPendingTasks().
Document that $interval uses a different queue (and its own flushing method - $interval.flush())
and is not taken into account by ngMock/$flushPendingTasks() and ngMock/$verifyNoPendingTasks().
Deprecate ngMock/$timeout.flush() and ngMock/$timeout.verifyNoPendingTasks().
Bonus task: Mark ngMock/$timeout.flush() and ngMock/$timeout.verifyNoPendingTasks() as deprecated in TS typings (on DefinitelyTyped).

In its current state, the PR does not introduce any change in external behavior, but adds internal features that could be built on top of. Here are my suggested public changes (in an attempts to address #14336 in a backwards compatible way):

New API: ngMock/$flushPendingTasks()
- Alias for ngMock/$timeout.flush() and thus ngMock/$browser.defer.flush().
New API: ngMock/$verifyNoPendingTasks([taskType])
- Verifies that there are no pending tasks at all or no pending tasks of the specified type.
- Alias for ngMock/$timeout.verifyNoPendingTasks() (for historical reasons) when called with
  no argument.
- Additionally supports more granular checks with taskType.
New API: Match Angular's task tracking APIs.
- ~~See Feature Request: Testability API changes #15585 (comment) for details.~~
- We decided not try to implement Angular’s whenStable() API, since it would be too complicated to provide equivalent info (and wouldn’t be 100% equivalent anyway).
Existing API: ngMock/$timeout.flush()
- Leave unchanged (for backwards compatibility).
- Document that all types of tasks (not just timeouts) are flushed.
- Recommend using ngMock/$flushPendingTasks() instead. (It does the same, but is named more intuitively.)
Existing API: ngMock/$timeout.verifyNoPendingTasks()
- Leave unchanged (for backwards compatibility).
- Document that all types of tasks (not just timeouts) are verified.
- Recommend using ngMock/$verifyNoPendingTasks('$timeout') instead.

Narretz · 2018-06-18T09:59:43Z

This sound like a good idea.
It could also help with e2e tests, provided protractor includes the changes to $$testability in the public API for waitForAngularEnabled() (see #14118). This is also where I see a problem: the proposed API allows whenStable to be called with a single argument, but I think for the most-wanted use case (ignoring timeouts that are used as intervals), you'd want to exclude a taskType rather than specifying one explicitly. This is especially important as many users may not be aware of stuff that uses applyAsync or evalAsync. I guess protractor could wrap multiple whenStable() executions, but I think we can have a better API here. This might also be an opportunity to make whenStable public.

thorn0 · 2018-06-18T17:02:24Z

BTW, there remains one more unsolved problem with those tasks, namely timeouts and intervals have separate queues that have to be flushed separately. That's why it's difficult to unit-test logic that mixes timeouts and intervals. Another mention of this issue: #10525 (comment)

gkalpak · 2018-06-18T19:02:39Z

Tackling that would be a much bigger change (and one we probably won't get to 😁).
I am not even sure how I feel about unifying the queues for $timeout and $interval, since they use different methods to schedule tasks (setTimeout vs setInterval) and you can't really guarrantee their relative order afaict (so why would tests make such guarrantees).

jbedard · 2018-06-18T19:08:38Z

src/ng/browser.js

+  function decOutstandingRequestCount(taskType) {
+    taskType = taskType || DEFAULT_TASK_TYPE;
+    if (outstandingRequestCounts[taskType]) outstandingRequestCounts[taskType]--;
+    if (outstandingRequestCounts[ALL_TASKS_TYPE]) outstandingRequestCounts[ALL_TASKS_TYPE]--;


I feel like this should go into the if (outstandingRequestCounts[taskType]) and not be it's own if. That way if taskType is invalid or not set it won't decrement outstandingRequestCounts[ALL_TASKS_TYPE]...?

Narretz

That's a lot of code change ...
I can't say I reviewed everything - mostly focussed on the docs.

So the second parameter from whenStable was removed intentionally because the API would have been too complex, right? I guess Protractor could still use the 2nd argument to browser.notifyWhenNoOutstandingRequests() to ignore some task types ($timeout, cough) on startup. Both browser and whenStable are private APIs anyway.
I guess at this point it doesn't make much sense to make them public.

Narretz · 2018-07-04T08:52:24Z

src/ngMock/angular-mocks.js

+ *   These include tasks scheduled via `$evalAsync()` indirectly (such as {@link $q} promises).
+ *
+ * <div class="alert alert-info">
+ *   Periodic tasks scheduled via {@link $interval} use a different queue and are flushed by


are flushed -> are not flushed

Narretz · 2018-07-04T08:53:45Z

src/ngMock/angular-mocks.js

+ *   `$flushPendingTasks()`. Use {@link ngMock.$interval#flush $interval.flush([millis])} instead.
+ * </div>
+ *
+ * @param {number=} delay - The number of milliseconds to flush.


Should it be noted that this only applies to the $timeout?

What do you mean? It doesn't apply to $timeout only (I have listed the types of tasks flushed above).

I mean "the number milliseconds to flush" only applies to the $timeout queue. This might have been obvious when flush() was on $timeout, but now the flush() also flushes applyAsync and evalAsync and they don't have a "timeline" like timeout does. At least I would expect if I put in a delay of 0 or no delay that applyAsync and evalAsync would still be flushed, but timeouts wouldn't

Good point. I'll mention that. I will ensure there are tests that verify the expected behavior (no delay flushes everything, 0 delay flushes stuff with 0 delay only, etc.).

Narretz · 2018-07-04T09:03:38Z

src/ngMock/angular-mocks.js

+ *   account by `$verifyNoPendingTasks()`.
+ * </div>
+ *
+ * @param {string=} taskType - The type of tasks to check for.


@returns is missing. Should also include that it throws when tasks are pending.

It doesn't really return anything atm 😁
+1 for documenting the thrown error.

Narretz · 2018-07-04T09:11:07Z

src/ngMock/angular-mocks.js

+ *   These include tasks scheduled via `$evalAsync()` indirectly (such as {@link $q} promises).
+ *
+ * <div class="alert alert-info">
+ *   Periodic tasks scheduled via {@link $interval} use a different queue and are not taken into


Should we mention that it's impossible to verify that no intervals are running? "uses a different queue" makes it sound a bit like the possibility exists.

Narretz · 2018-07-04T09:19:27Z

src/ngMock/angular-mocks.js

+ * The types of tasks that are flushed include:
+ *
+ * - Pending timeouts (via {@link $timeout}).
+ * - Pending tasks scheduled via {@link ng.$rootScope.Scope#$applyAsync}.


This must be {@link ng.$rootScope.Scope#$applyAsync $applyAsync} or similar, otherwise it'll show $rootScope.Scope as the name.

Same in the line below.

Narretz · 2018-07-04T09:22:09Z

src/ng/interval.js

@@ -4,10 +4,18 @@ var $intervalMinErr = minErr('$interval');

 /** @this */
 function $IntervalProvider() {
-  this.$get = ['$rootScope', '$window', '$q', '$$q', '$browser',
-       function($rootScope,   $window,   $q,   $$q,   $browser) {
+  this.$get = ['$$intervalFactory', '$window',


I assume we don't care that this is "breaking" the decoration of $interval by including a private service?

Why does it break it?

Now that you ask ... doesn't look like it breaks anything (and it relied on a private service before)

Narretz · 2018-07-04T09:26:38Z

src/ngMock/angular-mocks.js

+          'If you only want to verify pending timeouts, use ' +
+          '`$verifyNoPendingTasks(\'$timeout\')` instead.';
+
+      throw new Error('Deferred tasks to flush (' + pendingTasks.length + '):\n  ' +


I think we should document that this fn will throw this error. I know that it's not new, but since we're at it ...

Narretz · 2018-07-04T09:29:17Z

src/ngMock/angular-mocks.js

+   * Verifies that there are no pending tasks that need to be flushed.
+   * You can check for a specific type of tasks only, by specifying a `taskType`.
+   *
+   * See {@link $verifyNoPendingsTasks} for more info.


verifyNoPendingsTasks -> verifyNoPendingTasks

~~why does this not fail in the docs processor?~~ because it's not public

gkalpak · 2018-07-04T10:28:10Z

So the second parameter from whenStable was removed intentionally because the API would have been too complex, right?

The reason we dropped the whenStable() changes (and kept it private 😁) is that Angular has since made different changes to their whenStable() API (e.g. passing a timeout as a second argument), so it doesn't make sense to make incompatible changes. (Matching their API would be too complicated (and wouldn’t be 100% equivalent anyway), because they are relying on Zone.js.)

gkalpak · 2018-07-04T11:27:33Z

@Narretz, addressed feedback. Thx for taking a look ❤️

Narretz · 2018-07-04T12:57:20Z

The reason we dropped the whenStable() changes (and kept it private 😁) is that Angular has since made different changes to their whenStable() API

Ok, but if you only think about AngularJS then e.g. Protractor could provide a different API for AngularJS apps. In fact, in the docs I can't really see anything related to timeouts passed into https://www.protractortest.org/#/api?view=ProtractorBrowser.prototype.waitForAngularEnabled

However, they can still do it without the API being public.

gkalpak · 2018-07-04T13:03:37Z

Yeah, protractor is still not using Angular's new whenStable API (but theoretically it will in the future).
And yes, protractor could probably use the private $browser.notifyWhenNoPendingTasks() API for AngularJS apps, if they want to, but it might be less straight forward than using $$testability, because we expose $$testability in an easier way iirc (whereas for $browser, they would have to get hold of the $injector first). (I could be wrong though.)

Narretz · 2018-07-04T13:13:31Z

Protractor uses testability or browser via the injector: https://github.com/angular/protractor/blob/ed955e56a839d7f69da43acb6755763220d3681d/lib/clientsidescripts.js#L78-L87

Now if they wanted to use granular tracking they'd have to add some more conditions because granular tracking is only available via browser.

I think we should simply give whenStable the second argument back to make this easier.

gkalpak · 2018-07-07T14:48:56Z

We discussed this "offline" and decided that changing the testability API does not have to be part of this PR. Let's get this merged and discuss this with the Protractor team.

petebacondarwin · 2018-07-10T10:37:05Z

src/ng/browser.js

-      outstandingRequestCallbacks.push(callback);
-    }
-  };
+  // TODO(vojta): prefix this method with $$ ?


It's probably a bit late for this TODO now :-)

Hope never dies 😃

petebacondarwin · 2018-07-10T11:25:22Z

test/ngMock/angular-mocksSpec.js

+        callback = jasmine.createSpy('callback');
+      });
+
+      it('should immediatelly run the callback if no pending tasks', function() {


typo => immediately

and below...

petebacondarwin

Generally LGTM - a few typos to fix and a couple of questions to answer.

Previously, all pending async tasks (tracked via `$browser`) are treated the same. I.e. things like `$$testability.whenStable()` and `ngMock#$timeout.verifyNoPendingTasks()` take all tasks into account. Yet, in some cases we might be interested in specific tasks only. For example, if one wants to verify there are no pending `$timeout`s, they don't care if there are other pending tasks, such as `$http` requests. Similarly, one might want to get notified when all `$http` requests have completed and does not care about pending promises. This commit adds support for more granular task tracking, by enabling callers to specify the type of task that is being added/removed from the queue and enabling listeners to be triggered when specific types of tasks are completed (even if there are more pending tasks of different types). The change is backwards compatible. I.e. calling the affected methods with no explicit task-type, behaves the same as before. Related to angular#14336.

…internal implementation

…rval` This avoids code/logic duplication and helps the implementations stay in-sync.

…ngMock/$browser` This avoids code/logic duplication and helps the implementations stay in-sync.

`$flushPendingTasks([delay])` allows flushing all pending tasks (or up to a specific delay). This includes `$timeout`s, `$q` promises and tasks scheduled via `$rootScope.$applyAsync()` and `$rootScope.$evalAsync()`. (ATM, it only flushes tasks scheduled via `$browser.defer()`, which does not include `$http` requests and `$route` transitions.) `$verifyNoPendingTasks([taskType])` allows verifying that there are no pending tasks (in general or of a specific type). This includes tasks flushed by `$flushPendingTasks()` as well as pending `$http` requests and in-progress `$route` transitions. Background: `ngMock/$timeout` has `flush()` and `verifyNoPendingTasks()` methods, but they take all kinds of tasks into account which is confusing. For example, `$timeout.verifyNoPendingTasks()` can fail (even if there are no pending timeouts) because of an unrelated pending `$http` request. This behavior is retained for backwards compatibility, but the new methods are more generic (and thus less confusing) and also allow more fine-grained control (when appropriate). Closes angular#14336

…Tasks()`

…priate For historical reasons, `$timeout.verifyNoPendingTasks()` throws if there is any type of task pending (even if it is not a timeout). When calling `$timeout.verifyNoPendingTasks()`, the user is most likely interested in verifying pending timeouts only, which is now possible with `$verifyNoPendingTasks('$timeout')`. To raise awareness of `$verifyNoPendingTasks()`, it is mentioned in the error message thrown by `$timeoutverifyNoPendingTasks()` if none of the pending tasks is a timeout.

…Tasks()`

…` and `ngMock/$browser`

gkalpak · 2018-07-10T16:05:53Z

Thx for the review, @petebacondarwin. I think I've addressed all feedback (either by making the changes or trying to be entertaining 😛).

petebacondarwin · 2018-07-10T21:16:51Z

You were just about entertaining enough to distract me from requesting you make any more changes. Which I think means I am happy to let you merge this.

Narretz

🚀

Closes #16603

Related to angular/angular.js#16603 and angular/angular.js#16640.

* angular-route: Update types and docs for `IRoute` * angular-mocks: Update types and docs for 1.7 Related to angular/angular.js#16603 and angular/angular.js#16640.

…telyTyped#27473) * angular-route: Update types and docs for `IRoute` * angular-mocks: Update types and docs for 1.7 Related to angular/angular.js#16603 and angular/angular.js#16640.

googlebot added cla: yes labels Jun 16, 2018

gkalpak added type: feature type: refactor component: misc core component: ngMock needs: test labels Jun 16, 2018

gkalpak added this to the 1.7.x milestone Jun 16, 2018

gkalpak self-assigned this Jun 16, 2018

gkalpak added the needs: review label Jun 16, 2018

Narretz mentioned this pull request Jun 18, 2018

Feature Request: Testability API changes #15585

Closed

jbedard reviewed Jun 18, 2018

View reviewed changes

gkalpak force-pushed the feat-granular-pending-task-tracking branch 5 times, most recently from 637f13b to 672e65b Compare July 2, 2018 13:53

Narretz suggested changes Jul 4, 2018

View reviewed changes

gkalpak force-pushed the feat-granular-pending-task-tracking branch from 3c7e596 to f14938b Compare July 7, 2018 14:46

petebacondarwin reviewed Jul 10, 2018

View reviewed changes

petebacondarwin approved these changes Jul 10, 2018

View reviewed changes

petebacondarwin added needs: work and removed needs: review needs: test labels Jul 10, 2018

gkalpak added 13 commits July 10, 2018 18:43

fixup! feat(*): implement more granular pending task tracking

5d68b93

fixup! feat(*): implement more granular pending task tracking

fbbaf89

refactor(ngMock/$interval): more closely follow actual $interval's …

d39460f

…internal implementation

refactor($interval): share code between $interval and `ngMock/$inte…

a446203

…rval` This avoids code/logic duplication and helps the implementations stay in-sync.

refactor($browser): share task-tracking code between $browser and `…

fd4f707

…ngMock/$browser` This avoids code/logic duplication and helps the implementations stay in-sync.

fixup! feat(ngMock): add $flushPendingTasks() and `$verifyNoPending…

2c78686

…Tasks()`

docs(ngMock/$timeout): deprecate flush() and verifyNoPendingTasks()

5527335

fixup! feat(ngMock): add $flushPendingTasks() and `$verifyNoPending…

8dfd9d9

…Tasks()`

fixup! feat(*): implement more granular pending task tracking

e93405d

fixup! refactor($browser): share task-tracking code between `$browser…

86b0c03

…` and `ngMock/$browser`

gkalpak force-pushed the feat-granular-pending-task-tracking branch from f14938b to 86b0c03 Compare July 10, 2018 16:11

Narretz approved these changes Jul 13, 2018

View reviewed changes

gkalpak closed this in 6706353 Jul 13, 2018

gkalpak added a commit that referenced this pull request Jul 13, 2018

docs(ngMock/$timeout): deprecate flush() and verifyNoPendingTasks()

5cb9465

Closes #16603

gkalpak deleted the feat-granular-pending-task-tracking branch July 13, 2018 10:39

gkalpak added a commit to gkalpak/DefinitelyTyped that referenced this pull request Jul 21, 2018

angular-mocks: Update types and docs for 1.7

61ddd37

Related to angular/angular.js#16603 and angular/angular.js#16640.

gkalpak added a commit to gkalpak/DefinitelyTyped that referenced this pull request Jul 22, 2018

angular-mocks: Update types and docs for 1.7

c2ef446

Related to angular/angular.js#16603 and angular/angular.js#16640.

feat(*): implement more granular pending task tracking #16603

feat(*): implement more granular pending task tracking #16603

Conversation

gkalpak commented Jun 16, 2018 • edited Loading

gkalpak commented Jun 16, 2018 • edited Loading

Narretz commented Jun 18, 2018

thorn0 commented Jun 18, 2018

gkalpak commented Jun 18, 2018

Choose a reason for hiding this comment

Narretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Jul 4, 2018 • 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

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

Narretz Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

gkalpak commented Jul 4, 2018 • edited Loading

gkalpak commented Jul 4, 2018

Narretz commented Jul 4, 2018 • edited Loading

gkalpak commented Jul 4, 2018

Narretz commented Jul 4, 2018

gkalpak commented Jul 7, 2018

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

petebacondarwin left a comment

Choose a reason for hiding this comment

gkalpak commented Jul 10, 2018

petebacondarwin commented Jul 10, 2018

Narretz left a comment

Choose a reason for hiding this comment

gkalpak commented Jun 16, 2018 •

edited

Loading

gkalpak commented Jun 16, 2018 •

edited

Loading

gkalpak Jul 4, 2018 •

edited

Loading

Narretz Jul 4, 2018 •

edited

Loading

gkalpak commented Jul 4, 2018 •

edited

Loading

Narretz commented Jul 4, 2018 •

edited

Loading