Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Commit c522a43

Browse files
sjelingkalpak
sjelin
authored and
gkalpak
committed
fix($route): make asynchronous tasks count as pending requests
Protractor users were having a problem where if they had asynchonous code in a `route.resolve` or `route.resolveRedirectTo` variable, Protractor was not waiting for that code to complete before continuing. See angular/protractor#789 (comment) for details. This commit fixes it by ensuring that `$browser#outstandingRequestCount` is properly increased/decreased while `$route` (asynchronously) processes a route. Also, enhanced `ngMock` to wait for pending requests, before calling callbacks from `$browser.notifyWhenNoOutstandingRequests()`. Related to angular/protractor#789. Closes #14159
1 parent 9c722cf commit c522a43

File tree

6 files changed

+308
-9
lines changed

6 files changed

+308
-9
lines changed

‎src/ngMock/angular-mocks.js

+26-8
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,30 @@ angular.mock.$Browser = function() {
3737
self.$$lastUrl = self.$$url; // used by url polling fn
3838
self.pollFns = [];
3939

40-
// TODO(vojta): remove this temporary api
41-
self.$$completeOutstandingRequest = angular.noop;
42-
self.$$incOutstandingRequestCount = angular.noop;
43-
40+
// Testability API
41+
42+
var outstandingRequestCount = 0;
43+
var outstandingRequestCallbacks = [];
44+
self.$$incOutstandingRequestCount = function() { outstandingRequestCount++; };
45+
self.$$completeOutstandingRequest = function(fn) {
46+
try {
47+
fn();
48+
} finally {
49+
outstandingRequestCount--;
50+
if (!outstandingRequestCount) {
51+
while (outstandingRequestCallbacks.length) {
52+
outstandingRequestCallbacks.pop()();
53+
}
54+
}
55+
}
56+
};
57+
self.notifyWhenNoOutstandingRequests = function(callback) {
58+
if (outstandingRequestCount) {
59+
outstandingRequestCallbacks.push(callback);
60+
} else {
61+
callback();
62+
}
63+
};
4464

4565
// register url polling fn
4666

@@ -65,6 +85,8 @@ angular.mock.$Browser = function() {
6585
self.deferredNextId = 0;
6686

6787
self.defer = function(fn, delay) {
88+
// Note that we do not use `$$incOutstandingRequestCount` or `$$completeOutstandingRequest`
89+
// in this mock implementation.
6890
delay = delay || 0;
6991
self.deferredFns.push({time:(self.defer.now + delay), fn:fn, id: self.deferredNextId});
7092
self.deferredFns.sort(function(a, b) { return a.time - b.time;});
@@ -166,10 +188,6 @@ angular.mock.$Browser.prototype = {
166188

167189
state: function() {
168190
return this.$$state;
169-
},
170-
171-
notifyWhenNoOutstandingRequests: function(fn) {
172-
fn();
173191
}
174192
};
175193

‎src/ngRoute/route.js

+13-1
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
var isArray;
88
var isObject;
99
var isDefined;
10+
var noop;
1011

1112
/**
1213
* @ngdoc module
@@ -54,6 +55,7 @@ function $RouteProvider() {
5455
isArray = angular.isArray;
5556
isObject = angular.isObject;
5657
isDefined = angular.isDefined;
58+
noop = angular.noop;
5759

5860
function inherit(parent, extra) {
5961
return angular.extend(Object.create(parent), extra);
@@ -350,7 +352,8 @@ function $RouteProvider() {
350352
'$injector',
351353
'$templateRequest',
352354
'$sce',
353-
function($rootScope, $location, $routeParams, $q, $injector, $templateRequest, $sce) {
355+
'$browser',
356+
function($rootScope, $location, $routeParams, $q, $injector, $templateRequest, $sce, $browser) {
354357

355358
/**
356359
* @ngdoc service
@@ -680,6 +683,8 @@ function $RouteProvider() {
680683

681684
var nextRoutePromise = $q.resolve(nextRoute);
682685

686+
$browser.$$incOutstandingRequestCount();
687+
683688
nextRoutePromise.
684689
then(getRedirectionData).
685690
then(handlePossibleRedirection).
@@ -700,6 +705,13 @@ function $RouteProvider() {
700705
if (nextRoute === $route.current) {
701706
$rootScope.$broadcast('$routeChangeError', nextRoute, lastRoute, error);
702707
}
708+
}).finally(function() {
709+
// Because `commitRoute()` is called from a `$rootScope.$evalAsync` block (see
710+
// `$locationWatch`), this `$$completeOutstandingRequest()` call will not cause
711+
// `outstandingRequestCount` to hit zero. This is important in case we are redirecting
712+
// to a new route which also requires some asynchronous work.
713+
714+
$browser.$$completeOutstandingRequest(noop);
703715
});
704716
}
705717
}

‎test/e2e/fixtures/ng-route-promise/index.html

+9
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<html ng-app="lettersApp">
2+
<body>
3+
<div ng-view></div>
4+
5+
<script src="angular.js"></script>
6+
<script src="angular-route.js"></script>
7+
<script src="script.js"></script>
8+
</body>
9+
</html>

‎test/e2e/fixtures/ng-route-promise/script.js

+43
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
'use strict';
2+
3+
angular.
4+
module('lettersApp', ['ngRoute']).
5+
config(function($routeProvider) {
6+
$routeProvider.
7+
otherwise(resolveRedirectTo('/foo1')).
8+
when('/foo1', resolveRedirectTo('/bar1')).
9+
when('/bar1', resolveRedirectTo('/baz1')).
10+
when('/baz1', resolveRedirectTo('/qux1')).
11+
when('/qux1', {
12+
template: '<ul><li ng-repeat="letter in $resolve.letters">{{ letter }}</li></ul>',
13+
resolve: resolveLetters()
14+
}).
15+
when('/foo2', resolveRedirectTo('/bar2')).
16+
when('/bar2', resolveRedirectTo('/baz2')).
17+
when('/baz2', resolveRedirectTo('/qux2')).
18+
when('/qux2', {
19+
template: '{{ $resolve.letters.length }}',
20+
resolve: resolveLetters()
21+
});
22+
23+
// Helpers
24+
function resolveLetters() {
25+
return {
26+
letters: function($q) {
27+
return $q(function(resolve) {
28+
window.setTimeout(resolve, 1000, ['a', 'b', 'c', 'd', 'e']);
29+
});
30+
}
31+
};
32+
}
33+
34+
function resolveRedirectTo(path) {
35+
return {
36+
resolveRedirectTo: function($q) {
37+
return $q(function(resolve) {
38+
window.setTimeout(resolve, 250, path);
39+
});
40+
}
41+
};
42+
}
43+
});

‎test/e2e/tests/ng-route-promise.spec.js

+33
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
'use strict';
2+
3+
describe('ngRoute promises', function() {
4+
beforeEach(function() {
5+
loadFixture('ng-route-promise');
6+
});
7+
8+
it('should wait for route promises', function() {
9+
expect(element.all(by.tagName('li')).count()).toBe(5);
10+
});
11+
12+
it('should time out if the promise takes long enough', function() {
13+
// Don't try this at home kids, I'm a protractor dev
14+
browser.manage().timeouts().setScriptTimeout(1500);
15+
browser.waitForAngular().then(function() {
16+
fail('waitForAngular() should have timed out, but didn\'t');
17+
}, function(error) {
18+
expect(error.message).toContain('Timed out waiting for asynchronous Angular tasks to finish');
19+
});
20+
});
21+
22+
it('should wait for route promises when navigating to another route', function() {
23+
browser.setLocation('/foo2');
24+
expect(element(by.tagName('body')).getText()).toBe('5');
25+
});
26+
27+
afterEach(function(done) {
28+
// Restore old timeout limit
29+
browser.getProcessedConfig().then(function(config) {
30+
return browser.manage().timeouts().setScriptTimeout(config.allScriptsTimeout);
31+
}).then(done);
32+
});
33+
});

‎test/ngRoute/routeSpec.js

+184
Original file line numberDiff line numberDiff line change
@@ -2082,4 +2082,188 @@ describe('$route', function() {
20822082
expect(function() { $route.updateParams(); }).toThrowMinErr('ngRoute', 'norout');
20832083
}));
20842084
});
2085+
2086+
describe('testability', function() {
2087+
it('should wait for $resolve promises before calling callbacks', function() {
2088+
var deferred;
2089+
2090+
module(function($provide, $routeProvider) {
2091+
$routeProvider.when('/path', {
2092+
template: '',
2093+
resolve: {
2094+
a: function($q) {
2095+
deferred = $q.defer();
2096+
return deferred.promise;
2097+
}
2098+
}
2099+
});
2100+
});
2101+
2102+
inject(function($location, $route, $rootScope, $httpBackend, $$testability) {
2103+
$location.path('/path');
2104+
$rootScope.$digest();
2105+
2106+
var callback = jasmine.createSpy('callback');
2107+
$$testability.whenStable(callback);
2108+
expect(callback).not.toHaveBeenCalled();
2109+
2110+
deferred.resolve();
2111+
$rootScope.$digest();
2112+
expect(callback).toHaveBeenCalled();
2113+
});
2114+
});
2115+
2116+
it('should call callback after $resolve promises are rejected', function() {
2117+
var deferred;
2118+
2119+
module(function($provide, $routeProvider) {
2120+
$routeProvider.when('/path', {
2121+
template: '',
2122+
resolve: {
2123+
a: function($q) {
2124+
deferred = $q.defer();
2125+
return deferred.promise;
2126+
}
2127+
}
2128+
});
2129+
});
2130+
2131+
inject(function($location, $route, $rootScope, $httpBackend, $$testability) {
2132+
$location.path('/path');
2133+
$rootScope.$digest();
2134+
2135+
var callback = jasmine.createSpy('callback');
2136+
$$testability.whenStable(callback);
2137+
expect(callback).not.toHaveBeenCalled();
2138+
2139+
deferred.reject();
2140+
$rootScope.$digest();
2141+
expect(callback).toHaveBeenCalled();
2142+
});
2143+
});
2144+
2145+
it('should wait for resolveRedirectTo promises before calling callbacks', function() {
2146+
var deferred;
2147+
2148+
module(function($provide, $routeProvider) {
2149+
$routeProvider.when('/path', {
2150+
resolveRedirectTo: function($q) {
2151+
deferred = $q.defer();
2152+
return deferred.promise;
2153+
}
2154+
});
2155+
});
2156+
2157+
inject(function($location, $route, $rootScope, $httpBackend, $$testability) {
2158+
$location.path('/path');
2159+
$rootScope.$digest();
2160+
2161+
var callback = jasmine.createSpy('callback');
2162+
$$testability.whenStable(callback);
2163+
expect(callback).not.toHaveBeenCalled();
2164+
2165+
deferred.resolve();
2166+
$rootScope.$digest();
2167+
expect(callback).toHaveBeenCalled();
2168+
});
2169+
});
2170+
2171+
it('should call callback after resolveRedirectTo promises are rejected', function() {
2172+
var deferred;
2173+
2174+
module(function($provide, $routeProvider) {
2175+
$routeProvider.when('/path', {
2176+
resolveRedirectTo: function($q) {
2177+
deferred = $q.defer();
2178+
return deferred.promise;
2179+
}
2180+
});
2181+
});
2182+
2183+
inject(function($location, $route, $rootScope, $httpBackend, $$testability) {
2184+
$location.path('/path');
2185+
$rootScope.$digest();
2186+
2187+
var callback = jasmine.createSpy('callback');
2188+
$$testability.whenStable(callback);
2189+
expect(callback).not.toHaveBeenCalled();
2190+
2191+
deferred.reject();
2192+
$rootScope.$digest();
2193+
expect(callback).toHaveBeenCalled();
2194+
});
2195+
});
2196+
2197+
it('should wait for all route promises before calling callbacks', function() {
2198+
var deferreds = {};
2199+
2200+
module(function($provide, $routeProvider) {
2201+
// While normally `$browser.defer()` modifies the `outstandingRequestCount`, the mocked
2202+
// version (provided by `ngMock`) does not. This doesn't matter in most tests, but in this
2203+
// case we need the `outstandingRequestCount` logic to ensure that we don't call the
2204+
// `$$testability.whenStable()` callbacks part way through a `$rootScope.$evalAsync` block.
2205+
// See ngRoute's commitRoute()'s finally() block for details.
2206+
$provide.decorator('$browser', function($delegate) {
2207+
var oldDefer = $delegate.defer;
2208+
var newDefer = function(fn, delay) {
2209+
var requestCountAwareFn = function() { $delegate.$$completeOutstandingRequest(fn); };
2210+
$delegate.$$incOutstandingRequestCount();
2211+
return oldDefer.call($delegate, requestCountAwareFn, delay);
2212+
};
2213+
2214+
$delegate.defer = angular.extend(newDefer, oldDefer);
2215+
2216+
return $delegate;
2217+
});
2218+
2219+
addRouteWithAsyncRedirect('/foo', '/bar');
2220+
addRouteWithAsyncRedirect('/bar', '/baz');
2221+
addRouteWithAsyncRedirect('/baz', '/qux');
2222+
$routeProvider.when('/qux', {
2223+
template: '',
2224+
resolve: {
2225+
a: function($q) {
2226+
var deferred = deferreds['/qux'] = $q.defer();
2227+
return deferred.promise;
2228+
}
2229+
}
2230+
});
2231+
2232+
// Helpers
2233+
function addRouteWithAsyncRedirect(fromPath, toPath) {
2234+
$routeProvider.when(fromPath, {
2235+
resolveRedirectTo: function($q) {
2236+
var deferred = deferreds[fromPath] = $q.defer();
2237+
return deferred.promise.then(function() { return toPath; });
2238+
}
2239+
});
2240+
}
2241+
});
2242+
2243+
inject(function($browser, $location, $rootScope, $route, $$testability) {
2244+
$location.path('/foo');
2245+
$rootScope.$digest();
2246+
2247+
var callback = jasmine.createSpy('callback');
2248+
$$testability.whenStable(callback);
2249+
expect(callback).not.toHaveBeenCalled();
2250+
2251+
deferreds['/foo'].resolve();
2252+
$browser.defer.flush();
2253+
expect(callback).not.toHaveBeenCalled();
2254+
2255+
deferreds['/bar'].resolve();
2256+
$browser.defer.flush();
2257+
expect(callback).not.toHaveBeenCalled();
2258+
2259+
deferreds['/baz'].resolve();
2260+
$browser.defer.flush();
2261+
expect(callback).not.toHaveBeenCalled();
2262+
2263+
deferreds['/qux'].resolve();
2264+
$browser.defer.flush();
2265+
expect(callback).toHaveBeenCalled();
2266+
});
2267+
});
2268+
});
20852269
});

0 commit comments

Comments
 (0)