Angular.js: fix($timeout): `verifyNoPendingTasks()` throws even for non-timeout pending tasks

[gkalpak]

Do you want to request a _feature_ or report a _bug_?

Bug

What is the current behavior?

ngMock's $timeout.verifyNoPendingTasks() throws if there are deferred tasks that are not related to $timeout.
Reproduction

What is the expected behavior?

ngMock's $timeout.verifyNoPendingTasks() should not throw if there are deferred tasks that are not related to $timeout.

What is the motivation / use case for changing the behavior?

It makes it difficult/unintuitive to test code that uses $timeout wrt pending tasks.

Which versions of Angular, and which browser / OS are affected by this issue? Did this work in previous versions of Angular? Please also test with the latest stable and snapshot (https://code.angularjs.org/snapshot/) versions.

Angular versions: 1.4 and 1.5 (those I tested with, but should affect oldrer versions too)
Browsers: All

Other information (e.g. stacktraces, related issues, suggestions how to fix)

When this method was introduced (back in f0c6ebc), $timeout was probably the only service that used $browser.defer(), so all $browser.deferredFns came from $timeout (thus an non-empty array, meant pending timeouts).
This is no longer the case. E.g. $browser.defer() is used in $evalAsync() and $applyAsync(), which means it transitively used throughout Angular (in watchers, directives etc).

[gkalpak]

This has been broken for so long, that fixing it should probably be considered a breaking change :disappointed:

[mbarakaja]

Hello @gkalpak

I made a demo to reproduce the error.

So, if I flush a queued function with $timeout and then that queued function makes an HTTP request, which I have to flush with $httpBackend as well, to satisfy the HTTP request expectation in my test, $timeout.verifyNoPendingTasks() fails. If I don't flush the HTTP request, it passes.

I don't know how ngMock works internally, but is seen that a call to $httpBackend.flush(); adds an extra task that $timeout.verifyNoPendingTasks() thinks is related to a $timeout queued function.

[gkalpak]

@mbarakaja, thx for the demo. So, it is the same issue after all (an unexpected, non-timeout related deferred task, created after $httpBackend.flush()).

[petebacondarwin]

The problem in the reproduction by @mbarakaja is that the call to $http.post creates a $q deferred object that in turn adds a new item to the $browser pending tasks queue. It seems wrong to me that the call to $http.flush() does not remove this pending task from the $browser queue.

I think we need to find a way to segment calls to $browser.defer in ngMock so that we can then verify only particular kinds of pending tasks.

[rbevers]

That sounds good, @petebacondarwin . I understand it has been this way a long time, @gkalpak, but it still eats dev time when tests involve a combination of $http and $timeout.

I was dealing with an asynch select directive the other day that required both of these mocked in a test and banged my head testing all the cases. I was glad to see this reported already.

[eraye1]

Running into this on 1.5.9. :(

[jackyliang]

I am curious if you ever fixed this issue? I am running into (what I think) is a similar issue. If I use applyAsync instead of timeout, call verifyNoPendingTask(), then flush, the test would pass as if applyAsync is the same as timeout. Not quite sure how to get around this issue either.

[scottohara]

This issue started causing failures in my test suite when upgrading between 1.6.1 and 1.6.2.

I suspect that it may be due to https://github.com/angular/angular.js/pull/14159, which now adds a pending task for any pending route resolves.

Although I don't have a repro to prove this yet, looking at the set of items listed in the 1.6.2 changelog, I'm basing my suspicion on the fact that $route: make asynchronous tasks count as pending requests change _sounds_ like it could be related, given the similar comments above about $http.post creating pending task.

What I can confirm is that if I _don't_ load the ui.router module in my test suite, the test that checks $timeout.verifyNoPendingTasks() starts passing again. Curiously, even without any $stateProvider states configured (i.e. no routes, and nothing to resolve), the error still occurs if ui.router is loaded.

If, as discussed above, $timeout.verifyNoPendingTasks() is unable to distinguish between these pending resolve tasks and $timeout tasks, then this issue will potentially start to impact more users as they upgrade to 1.6.2 and beyond.

For now, I'm working around the issue by performing an extra preventative $timeout.flush() before my failing test runs.

[gkalpak]

The $route: make asynchronous tasks count as pending requests part does not affect ui.router, only ngRoute. So, it must be something else that is causing the new behavior.

[scottohara]

The $route: make asynchronous tasks count as pending requests part does not affect ui.router, only ngRoute

@gkalpak That is true.

However looking at the commit, there were also some changes made to ngMock/angular-mocks.js; specifically to:

• $browser.$$incOutstandingRequestCount() (previously a noop)
• $browser.$$completeOutstandingRequest() (previously a noop)
• $browser.notifyWhenNoOutstandingRequests()

So it is not only ngRoute that is affected by the change, but also ngMock.

Perhaps ui.router was already doing something similar to what ngRoute now does (i.e. calling $browser.$$incOutstandingRequestCount()), and since the function is no longer a noop in 1.6.2, the problem arises?

I'll keep looking to see if I can pinpoint what is creating the pending tasks.

[gkalpak]

These are private methods and external libraries should not be using or relying on them. If ui.router does, then it might indeed cause your tests to break. If it doesn't, then it must be something else.

[scottohara]

OK, thanks @gkalpak. I think I've tracked down where the pending task is being created.

The issue seems to be that ui.router.state.$state.$get() _immediately_ creates a rejected promise (which it will later return if it determines the state being transitioned to is no longer current):

function $get($rootScope, $q, $view, $injector, $resolve, $stateParams, $urlRouter, $location, $urlMatcherFactory) {
    var TransitionSupersededError = new Error('transition superseded');
    var TransitionSuperseded = silenceUncaughtInPromise($q.reject(TransitionSupersededError));
    ...
}   

It is the call to $q.reject that creates a pending task, i.e.

qFactory.reject() -> qFactory.rejectPromise() -> qFactory.$$reject() -> scheduleProcessQueue(promise.$$state)

scheduleProcessQueue(state) checks if state.status === 2, and if so calls nextTick(processChecks) -> $rootScope.$evalAsync() -> $browser.defer() -> which pushes an item onto $browser.deferredFns making it’s length > 0...hence a pending task.

I'll refer this issue to the ui.router project, and check if they are aware that they're inadvertently creating a pending task by setting up a rejected promise in their provider's $get() function.