From 6aca56b2711d7256702acfa0c306e7ba259068c1 Mon Sep 17 00:00:00 2001 From: Sammy Jelin Date: Tue, 1 Mar 2016 16:12:51 -0800 Subject: [PATCH] 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 https://github.com/angular/protractor/issues/789#issuecomment-190983200 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` --- src/ngMock/angular-mocks.js | 34 +++- src/ngRoute/route.js | 14 +- test/e2e/fixtures/ng-route-promise/index.html | 9 + test/e2e/fixtures/ng-route-promise/script.js | 43 ++++ test/e2e/tests/ng-route-promise.spec.js | 33 ++++ test/ngRoute/routeSpec.js | 184 ++++++++++++++++++ 6 files changed, 308 insertions(+), 9 deletions(-) create mode 100644 test/e2e/fixtures/ng-route-promise/index.html create mode 100644 test/e2e/fixtures/ng-route-promise/script.js create mode 100644 test/e2e/tests/ng-route-promise.spec.js diff --git a/src/ngMock/angular-mocks.js b/src/ngMock/angular-mocks.js index 50fff01fb012..110ef3e2d7f2 100644 --- a/src/ngMock/angular-mocks.js +++ b/src/ngMock/angular-mocks.js @@ -37,10 +37,30 @@ angular.mock.$Browser = function() { self.$$lastUrl = self.$$url; // used by url polling fn self.pollFns = []; - // TODO(vojta): remove this temporary api - self.$$completeOutstandingRequest = angular.noop; - self.$$incOutstandingRequestCount = angular.noop; - + // Testability API + + var outstandingRequestCount = 0; + var outstandingRequestCallbacks = []; + self.$$incOutstandingRequestCount = function() { outstandingRequestCount++; }; + self.$$completeOutstandingRequest = function(fn) { + try { + fn(); + } finally { + outstandingRequestCount--; + if (!outstandingRequestCount) { + while (outstandingRequestCallbacks.length) { + outstandingRequestCallbacks.pop()(); + } + } + } + }; + self.notifyWhenNoOutstandingRequests = function(callback) { + if (outstandingRequestCount) { + outstandingRequestCallbacks.push(callback); + } else { + callback(); + } + }; // register url polling fn @@ -65,6 +85,8 @@ angular.mock.$Browser = function() { self.deferredNextId = 0; self.defer = function(fn, delay) { + // Note that we do not use `$$incOutstandingRequestCount` or `$$completeOutstandingRequest` + // in this mock implementation. delay = delay || 0; self.deferredFns.push({time:(self.defer.now + delay), fn:fn, id: self.deferredNextId}); self.deferredFns.sort(function(a, b) { return a.time - b.time;}); @@ -166,10 +188,6 @@ angular.mock.$Browser.prototype = { state: function() { return this.$$state; - }, - - notifyWhenNoOutstandingRequests: function(fn) { - fn(); } }; diff --git a/src/ngRoute/route.js b/src/ngRoute/route.js index c3b78776e863..5820fd38178f 100644 --- a/src/ngRoute/route.js +++ b/src/ngRoute/route.js @@ -7,6 +7,7 @@ var isArray; var isObject; var isDefined; +var noop; /** * @ngdoc module @@ -54,6 +55,7 @@ function $RouteProvider() { isArray = angular.isArray; isObject = angular.isObject; isDefined = angular.isDefined; + noop = angular.noop; function inherit(parent, extra) { return angular.extend(Object.create(parent), extra); @@ -350,7 +352,8 @@ function $RouteProvider() { '$injector', '$templateRequest', '$sce', - function($rootScope, $location, $routeParams, $q, $injector, $templateRequest, $sce) { + '$browser', + function($rootScope, $location, $routeParams, $q, $injector, $templateRequest, $sce, $browser) { /** * @ngdoc service @@ -680,6 +683,8 @@ function $RouteProvider() { var nextRoutePromise = $q.resolve(nextRoute); + $browser.$$incOutstandingRequestCount(); + nextRoutePromise. then(getRedirectionData). then(handlePossibleRedirection). @@ -700,6 +705,13 @@ function $RouteProvider() { if (nextRoute === $route.current) { $rootScope.$broadcast('$routeChangeError', nextRoute, lastRoute, error); } + }).finally(function() { + // Because `commitRoute()` is called from a `$rootScope.$evalAsync` block (see + // `$locationWatch`), this `$$completeOutstandingRequest()` call will not cause + // `outstandingRequestCount` to hit zero. This is important in case we are redirecting + // to a new route which also requires some asynchronous work. + + $browser.$$completeOutstandingRequest(noop); }); } } diff --git a/test/e2e/fixtures/ng-route-promise/index.html b/test/e2e/fixtures/ng-route-promise/index.html new file mode 100644 index 000000000000..d89b57286b15 --- /dev/null +++ b/test/e2e/fixtures/ng-route-promise/index.html @@ -0,0 +1,9 @@ + + +
+ + + + + + diff --git a/test/e2e/fixtures/ng-route-promise/script.js b/test/e2e/fixtures/ng-route-promise/script.js new file mode 100644 index 000000000000..d9260bdee77d --- /dev/null +++ b/test/e2e/fixtures/ng-route-promise/script.js @@ -0,0 +1,43 @@ +'use strict'; + +angular. + module('lettersApp', ['ngRoute']). + config(function($routeProvider) { + $routeProvider. + otherwise(resolveRedirectTo('/foo1')). + when('/foo1', resolveRedirectTo('/bar1')). + when('/bar1', resolveRedirectTo('/baz1')). + when('/baz1', resolveRedirectTo('/qux1')). + when('/qux1', { + template: '', + resolve: resolveLetters() + }). + when('/foo2', resolveRedirectTo('/bar2')). + when('/bar2', resolveRedirectTo('/baz2')). + when('/baz2', resolveRedirectTo('/qux2')). + when('/qux2', { + template: '{{ $resolve.letters.length }}', + resolve: resolveLetters() + }); + + // Helpers + function resolveLetters() { + return { + letters: function($q) { + return $q(function(resolve) { + window.setTimeout(resolve, 1000, ['a', 'b', 'c', 'd', 'e']); + }); + } + }; + } + + function resolveRedirectTo(path) { + return { + resolveRedirectTo: function($q) { + return $q(function(resolve) { + window.setTimeout(resolve, 250, path); + }); + } + }; + } + }); diff --git a/test/e2e/tests/ng-route-promise.spec.js b/test/e2e/tests/ng-route-promise.spec.js new file mode 100644 index 000000000000..05cd35af1a00 --- /dev/null +++ b/test/e2e/tests/ng-route-promise.spec.js @@ -0,0 +1,33 @@ +'use strict'; + +describe('ngRoute promises', function() { + beforeEach(function() { + loadFixture('ng-route-promise'); + }); + + it('should wait for route promises', function() { + expect(element.all(by.tagName('li')).count()).toBe(5); + }); + + it('should time out if the promise takes long enough', function() { + // Don't try this at home kids, I'm a protractor dev + browser.manage().timeouts().setScriptTimeout(1500); + browser.waitForAngular().then(function() { + fail('waitForAngular() should have timed out, but didn\'t'); + }, function(error) { + expect(error.message).toContain('Timed out waiting for asynchronous Angular tasks to finish'); + }); + }); + + it('should wait for route promises when navigating to another route', function() { + browser.setLocation('/foo2'); + expect(element(by.tagName('body')).getText()).toBe('5'); + }); + + afterEach(function(done) { + // Restore old timeout limit + browser.getProcessedConfig().then(function(config) { + return browser.manage().timeouts().setScriptTimeout(config.allScriptsTimeout); + }).then(done); + }); +}); diff --git a/test/ngRoute/routeSpec.js b/test/ngRoute/routeSpec.js index 9f0544bd4b2a..772bdc7bc226 100644 --- a/test/ngRoute/routeSpec.js +++ b/test/ngRoute/routeSpec.js @@ -2082,4 +2082,188 @@ describe('$route', function() { expect(function() { $route.updateParams(); }).toThrowMinErr('ngRoute', 'norout'); })); }); + + describe('testability', function() { + it('should wait for $resolve promises before calling callbacks', function() { + var deferred; + + module(function($provide, $routeProvider) { + $routeProvider.when('/path', { + template: '', + resolve: { + a: function($q) { + deferred = $q.defer(); + return deferred.promise; + } + } + }); + }); + + inject(function($location, $route, $rootScope, $httpBackend, $$testability) { + $location.path('/path'); + $rootScope.$digest(); + + var callback = jasmine.createSpy('callback'); + $$testability.whenStable(callback); + expect(callback).not.toHaveBeenCalled(); + + deferred.resolve(); + $rootScope.$digest(); + expect(callback).toHaveBeenCalled(); + }); + }); + + it('should call callback after $resolve promises are rejected', function() { + var deferred; + + module(function($provide, $routeProvider) { + $routeProvider.when('/path', { + template: '', + resolve: { + a: function($q) { + deferred = $q.defer(); + return deferred.promise; + } + } + }); + }); + + inject(function($location, $route, $rootScope, $httpBackend, $$testability) { + $location.path('/path'); + $rootScope.$digest(); + + var callback = jasmine.createSpy('callback'); + $$testability.whenStable(callback); + expect(callback).not.toHaveBeenCalled(); + + deferred.reject(); + $rootScope.$digest(); + expect(callback).toHaveBeenCalled(); + }); + }); + + it('should wait for resolveRedirectTo promises before calling callbacks', function() { + var deferred; + + module(function($provide, $routeProvider) { + $routeProvider.when('/path', { + resolveRedirectTo: function($q) { + deferred = $q.defer(); + return deferred.promise; + } + }); + }); + + inject(function($location, $route, $rootScope, $httpBackend, $$testability) { + $location.path('/path'); + $rootScope.$digest(); + + var callback = jasmine.createSpy('callback'); + $$testability.whenStable(callback); + expect(callback).not.toHaveBeenCalled(); + + deferred.resolve(); + $rootScope.$digest(); + expect(callback).toHaveBeenCalled(); + }); + }); + + it('should call callback after resolveRedirectTo promises are rejected', function() { + var deferred; + + module(function($provide, $routeProvider) { + $routeProvider.when('/path', { + resolveRedirectTo: function($q) { + deferred = $q.defer(); + return deferred.promise; + } + }); + }); + + inject(function($location, $route, $rootScope, $httpBackend, $$testability) { + $location.path('/path'); + $rootScope.$digest(); + + var callback = jasmine.createSpy('callback'); + $$testability.whenStable(callback); + expect(callback).not.toHaveBeenCalled(); + + deferred.reject(); + $rootScope.$digest(); + expect(callback).toHaveBeenCalled(); + }); + }); + + it('should wait for all route promises before calling callbacks', function() { + var deferreds = {}; + + module(function($provide, $routeProvider) { + // While normally `$browser.defer()` modifies the `outstandingRequestCount`, the mocked + // version (provided by `ngMock`) does not. This doesn't matter in most tests, but in this + // case we need the `outstandingRequestCount` logic to ensure that we don't call the + // `$$testability.whenStable()` callbacks part way through a `$rootScope.$evalAsync` block. + // See ngRoute's commitRoute()'s finally() block for details. + $provide.decorator('$browser', function($delegate) { + var oldDefer = $delegate.defer; + var newDefer = function(fn, delay) { + var requestCountAwareFn = function() { $delegate.$$completeOutstandingRequest(fn); }; + $delegate.$$incOutstandingRequestCount(); + return oldDefer.call($delegate, requestCountAwareFn, delay); + }; + + $delegate.defer = angular.extend(newDefer, oldDefer); + + return $delegate; + }); + + addRouteWithAsyncRedirect('/foo', '/bar'); + addRouteWithAsyncRedirect('/bar', '/baz'); + addRouteWithAsyncRedirect('/baz', '/qux'); + $routeProvider.when('/qux', { + template: '', + resolve: { + a: function($q) { + var deferred = deferreds['/qux'] = $q.defer(); + return deferred.promise; + } + } + }); + + // Helpers + function addRouteWithAsyncRedirect(fromPath, toPath) { + $routeProvider.when(fromPath, { + resolveRedirectTo: function($q) { + var deferred = deferreds[fromPath] = $q.defer(); + return deferred.promise.then(function() { return toPath; }); + } + }); + } + }); + + inject(function($browser, $location, $rootScope, $route, $$testability) { + $location.path('/foo'); + $rootScope.$digest(); + + var callback = jasmine.createSpy('callback'); + $$testability.whenStable(callback); + expect(callback).not.toHaveBeenCalled(); + + deferreds['/foo'].resolve(); + $browser.defer.flush(); + expect(callback).not.toHaveBeenCalled(); + + deferreds['/bar'].resolve(); + $browser.defer.flush(); + expect(callback).not.toHaveBeenCalled(); + + deferreds['/baz'].resolve(); + $browser.defer.flush(); + expect(callback).not.toHaveBeenCalled(); + + deferreds['/qux'].resolve(); + $browser.defer.flush(); + expect(callback).toHaveBeenCalled(); + }); + }); + }); });