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

Commit 785c987

Browse files
sjelinsjelin
committed
fix(ngRoute): make route.resolve count as a pending request
Protractor users were having a problem where if they had asynchonous code in a route.resolve variable, Protractor was not waiting for that code to complete before continuing. See angular/protractor#789 (comment) for details Also enhanced ngMock to wait for pending requests before calling callbacks from `$browser.notifyWhenNoOutstandingRequests` Potentially breaking change if someone was relying on route.resolve not blocking `$browser.notifyWhenNoOutstandingRequests`
1 parent 0694af8 commit 785c987

File tree

6 files changed

+241
-9
lines changed

6 files changed

+241
-9
lines changed

‎src/ngMock/angular-mocks.js

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,26 @@ 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() {
46+
outstandingRequestCount--;
47+
if (!outstandingRequestCount) {
48+
while (outstandingRequestCallbacks.length) {
49+
outstandingRequestCallbacks.pop()();
50+
}
51+
}
52+
};
53+
self.notifyWhenNoOutstandingRequests = function(callback) {
54+
if (outstandingRequestCount) {
55+
outstandingRequestCallbacks.push(callback);
56+
} else {
57+
callback();
58+
}
59+
};
4460

4561
// register url polling fn
4662

@@ -166,10 +182,6 @@ angular.mock.$Browser.prototype = {
166182

167183
state: function() {
168184
return this.$$state;
169-
},
170-
171-
notifyWhenNoOutstandingRequests: function(fn) {
172-
fn();
173185
}
174186
};
175187

‎src/ngRoute/route.js

Lines changed: 8 additions & 1 deletion
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,8 @@ function $RouteProvider() {
700705
if (nextRoute === $route.current) {
701706
$rootScope.$broadcast('$routeChangeError', nextRoute, lastRoute, error);
702707
}
708+
}).finally(function() {
709+
$browser.$$completeOutstandingRequest(noop);
703710
});
704711
}
705712
}

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

Lines changed: 9 additions & 0 deletions
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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
'use strict';
2+
3+
angular.
4+
module('lettersApp', ['ngRoute']).
5+
config(function($routeProvider) {
6+
$routeProvider.
7+
when('/foo', {
8+
resolveRedirectTo: function($q) {
9+
return $q(function(resolve) {
10+
window.setTimeout(resolve, 1000, '/bar');
11+
});
12+
}
13+
}).
14+
when('/bar', {
15+
template: '<ul><li ng-repeat="letter in $resolve.letters">{{ letter }}</li></ul>',
16+
resolve: {
17+
letters: function($q) {
18+
return $q(function(resolve) {
19+
window.setTimeout(resolve, 1000, ['a', 'b', 'c', 'd', 'e']);
20+
});
21+
}
22+
}
23+
}).
24+
otherwise('/foo');
25+
});

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
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+
var errorRe = new RegExp(
19+
'Timed out waiting for asynchronous Angular tasks to finish after ' +
20+
'(?:1\\.5 seconds|15\\d\\d ?ms)\\.', '');
21+
expect(error.message).toMatch(errorRe);
22+
});
23+
});
24+
25+
afterEach(function(done) {
26+
// Restore old timeout limit
27+
browser.getProcessedConfig().then(function(config) {
28+
return browser.manage().timeouts().setScriptTimeout(config.allScriptsTimeout);
29+
}).then(function() { done(); });
30+
});
31+
});

‎test/ngRoute/routeSpec.js

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2082,4 +2082,152 @@ 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', { template: '', resolve: {
2092+
a: function($q) {
2093+
deferred = $q.defer();
2094+
return deferred.promise;
2095+
}
2096+
} });
2097+
});
2098+
2099+
inject(function($location, $route, $rootScope, $httpBackend, $$testability) {
2100+
$location.path('/path');
2101+
$rootScope.$digest();
2102+
2103+
var callback = jasmine.createSpy('callback');
2104+
$$testability.whenStable(callback);
2105+
expect(callback).not.toHaveBeenCalled();
2106+
2107+
deferred.resolve();
2108+
$rootScope.$digest();
2109+
expect(callback).toHaveBeenCalled();
2110+
});
2111+
});
2112+
2113+
it('should call callback after $resolve promises are rejected', function() {
2114+
var deferred;
2115+
2116+
module(function($provide, $routeProvider) {
2117+
$routeProvider.when('/path', { template: '', resolve: {
2118+
a: function($q) {
2119+
deferred = $q.defer();
2120+
return deferred.promise;
2121+
}
2122+
} });
2123+
});
2124+
2125+
inject(function($location, $route, $rootScope, $httpBackend, $$testability) {
2126+
$location.path('/path');
2127+
$rootScope.$digest();
2128+
2129+
var callback = jasmine.createSpy('callback');
2130+
$$testability.whenStable(callback);
2131+
expect(callback).not.toHaveBeenCalled();
2132+
2133+
deferred.reject();
2134+
$rootScope.$digest();
2135+
expect(callback).toHaveBeenCalled();
2136+
});
2137+
});
2138+
2139+
it('should wait for resolveRedirectTo promises before calling callbacks', function() {
2140+
var deferred;
2141+
2142+
module(function($provide, $routeProvider) {
2143+
$routeProvider.when('/path', { template: '', resolveRedirectTo: function($q) {
2144+
deferred = $q.defer();
2145+
return deferred.promise;
2146+
}
2147+
});
2148+
});
2149+
2150+
inject(function($location, $route, $rootScope, $httpBackend, $$testability) {
2151+
$location.path('/path');
2152+
$rootScope.$digest();
2153+
2154+
var callback = jasmine.createSpy('callback');
2155+
$$testability.whenStable(callback);
2156+
expect(callback).not.toHaveBeenCalled();
2157+
2158+
deferred.resolve();
2159+
$rootScope.$digest();
2160+
expect(callback).toHaveBeenCalled();
2161+
});
2162+
});
2163+
2164+
it('should call callback after resolveRedirectTo promises are rejected', function() {
2165+
var deferred;
2166+
2167+
module(function($provide, $routeProvider) {
2168+
$routeProvider.when('/path', { template: '', resolveRedirectTo: function($q) {
2169+
deferred = $q.defer();
2170+
return deferred.promise;
2171+
}
2172+
});
2173+
});
2174+
2175+
inject(function($location, $route, $rootScope, $httpBackend, $$testability) {
2176+
$location.path('/path');
2177+
$rootScope.$digest();
2178+
2179+
var callback = jasmine.createSpy('callback');
2180+
$$testability.whenStable(callback);
2181+
expect(callback).not.toHaveBeenCalled();
2182+
2183+
deferred.reject();
2184+
$rootScope.$digest();
2185+
expect(callback).toHaveBeenCalled();
2186+
});
2187+
});
2188+
2189+
it('should wait for all route promises before calling callbacks', function() {
2190+
var redirectDeferred;
2191+
var localsDeferred;
2192+
2193+
module(function($provide, $routeProvider) {
2194+
$routeProvider.
2195+
when('/foo', { template: '', resolveRedirectTo: function($q) {
2196+
redirectDeferred = $q.defer();
2197+
return redirectDeferred.promise;
2198+
} }).
2199+
when('/bar', { template: '', resolve: {
2200+
a: function($q) {
2201+
localsDeferred = $q.defer();
2202+
return localsDeferred.promise;
2203+
}
2204+
} });
2205+
});
2206+
2207+
inject(function($browser, $location, $route, $rootScope, $httpBackend, $$testability) {
2208+
$location.path('/foo');
2209+
$rootScope.$digest();
2210+
2211+
var callback = jasmine.createSpy('callback');
2212+
$$testability.whenStable(callback);
2213+
expect(callback).not.toHaveBeenCalled();
2214+
2215+
// ngRoute code is wrapped in a $browser.defer call (via $rootScope.evalAsync), which in
2216+
// production would automatically call $$incOutstandingRequestCount and
2217+
// $$completeOutstandingRequest before/after execution. However, ngMock does not call these
2218+
// functions in its $browser.defer implementation, as this logic would make many tests
2219+
// difficult to write. In this one case we need that logic, however, so we call the
2220+
// functions manually.
2221+
$browser.$$incOutstandingRequestCount();
2222+
redirectDeferred.resolve('/bar');
2223+
$rootScope.$digest();
2224+
$browser.$$completeOutstandingRequest();
2225+
expect(callback).not.toHaveBeenCalled();
2226+
2227+
localsDeferred.resolve();
2228+
$rootScope.$digest();
2229+
expect(callback).toHaveBeenCalled();
2230+
});
2231+
});
2232+
});
20852233
});

0 commit comments

Comments
 (0)