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

fix(ngMock/$httpBackend): correctly ignore query params in {expect,when}Route #16589

Closed
wants to merge 1 commit into from
Closed

fix(ngMock/$httpBackend): correctly ignore query params in {expect,when}Route #16589

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions angularFiles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ var angularFiles = {
],
'ngRoute': [
'src/shallowCopy.js',
'src/routeToRegExp.js',
'src/ngRoute/route.js',
'src/ngRoute/routeParams.js',
'src/ngRoute/directive/ngView.js'
Expand All @@ -139,6 +140,7 @@ var angularFiles = {
'src/ngSanitize/filter/linky.js'
],
'ngMock': [
'src/routeToRegExp.js',
'src/ngMock/angular-mocks.js',
'src/ngMock/browserTrigger.js'
],
Expand Down
56 changes: 15 additions & 41 deletions src/ngMock/angular-mocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
'use strict';

/* global routeToRegExp: false */

/**
* @ngdoc object
* @name angular.mock
Expand Down Expand Up @@ -1282,7 +1284,7 @@ angular.mock.dump = function(object) {
* ## Matching route requests
*
* For extra convenience, `whenRoute` and `expectRoute` shortcuts are available. These methods offer colon
* delimited matching of the url path, ignoring the query string. This allows declarations
* delimited matching of the url path, ignoring the query string and trailing slashes. This allows declarations
* similar to how application routes are configured with `$routeProvider`. Because these methods convert
* the definition url to regex, declaration order is important. Combined with query parameter parsing,
* the following is possible:
Expand Down Expand Up @@ -1481,8 +1483,9 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
* ```
* – The respond method takes a set of static data to be returned or a function that can
* return an array containing response status (number), response data (Array|Object|string),
* response headers (Object), and the text for the status (string). The respond method returns
* the `requestHandler` object for possible overrides.
* response headers (Object), HTTP status text (string), and XMLHttpRequest status (string:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it get XHR status? AFAICT it doesn't (because it is auto-generated).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't get it. It returns it. See http://jsbin.com/vumefiz/edit?html,js,output

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I misread that 😳

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, our createResponse() method isn't consistently treating XHR status. It does not accept one when passing the data directly to .respond() (and it automatically uses completeas the XHR status), but it does accept one when a function is passed to createResponse().

I think this is a bug (or at least unintentional). createResponse() should look something like:

 function createResponse(status, data, headers, statusText) {
-  if (angular.isFunction(status)) return status;
+  if (angular.isFunction(status)) {
+    return function(method, url, data, headers, params) {
+      var response = status(method, url, data, headers, params);
+      response[4] = 'complete';
+      return response;
+    };
+  }

   return function() {
     return angular.isNumber(status)
         ? [status, data, headers, statusText, 'complete']
         : [200, status, data, headers, 'complete'];
   };
 }

But this is unrelated to this PR and would probably be a breaking change now 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you think so? What if we want to test the behavior of our app with XHR statuses other than complete (e.g. error)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But (like I said), this is not something to worry about in this PR. It is not related (and that ship has probably sailed 😁).

Copy link
Contributor Author

@thorn0 thorn0 Jun 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't have error with a "normal" response (it is something that will not come up in the actual app

I believe it can. It happens in case of network error, doesn't it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's not a "normal" response. I.e. an XHR status other than complete can only happen if the XHR's .onabort/.onerror/.ontimeout is called. In that case, you would have a status of 0 and empty values for other fields (statusText, headers, etc).

But the mock $httpBackend is not really designed to let you test these scenarios. Sure you can emulate it by returning the correct values from your response function, you need to know $httpBackend internals. E.g. returning [200, 'OK', ..., 'aborted'] is a scenario that will never happen in the actual app and will lead to unexpected results (because aborted will have been called with a "success" status).

So, ideally, we should not let users specify the XHR status, but instead infer it from other params.
Even more ideally, $httpBackend could support testing scenarions when an XHR errored, or timed out or was aborted (and again automatically setting the correct XHR status; not letting the user specify it).

But as it is now, $httpBackend is better used with "normal" responses (unless you really know what you are doing) 😛

Copy link
Contributor Author

@thorn0 thorn0 Jun 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a separate PR, we can add simple checks and throw in cases like [200, 'OK', ..., 'abort']. What are your thoughts on that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't be opposed to that 😃

* `complete`, `error`, `timeout` or `abort`). The respond method returns the `requestHandler`
* object for possible overrides.
*/
$httpBackend.when = function(method, url, data, headers, keys) {

Expand Down Expand Up @@ -1663,40 +1666,10 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
* See {@link ngMock.$httpBackend#when `when`} for more info.
*/
$httpBackend.whenRoute = function(method, url) {
var pathObj = parseRoute(url);
var pathObj = routeToRegExp(url, {caseInsensitiveMatch: true, ignoreTrailingSlashes: true});
return $httpBackend.when(method, pathObj.regexp, undefined, undefined, pathObj.keys);
};

function parseRoute(url) {
var ret = {
regexp: url
},
keys = ret.keys = [];

if (!url || !angular.isString(url)) return ret;

url = url
.replace(/([().])/g, '\\$1')
.replace(/(\/)?:(\w+)([?*])?/g, function(_, slash, key, option) {
var optional = option === '?' ? option : null;
var star = option === '*' ? option : null;
keys.push({ name: key, optional: !!optional });
slash = slash || '';
return ''
+ (optional ? '' : slash)
+ '(?:'
+ (optional ? slash : '')
+ (star && '(.+?)' || '([^/]+)')
+ (optional || '')
+ ')'
+ (optional || '');
})
.replace(/([/$*])/g, '\\$1');

ret.regexp = new RegExp('^' + url, 'i');
return ret;
}

/**
* @ngdoc method
* @name $httpBackend#expect
Expand All @@ -1717,14 +1690,15 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
* order to change how a matched request is handled.
*
* - respond –
* ```
* { function([status,] data[, headers, statusText])
* | function(function(method, url, data, headers, params)}
* ```
* ```js
* {function([status,] data[, headers, statusText])
* | function(function(method, url, data, headers, params)}
* ```
* – The respond method takes a set of static data to be returned or a function that can
* return an array containing response status (number), response data (Array|Object|string),
* response headers (Object), and the text for the status (string). The respond method returns
* the `requestHandler` object for possible overrides.
* response headers (Object), HTTP status text (string), and XMLHttpRequest status (string:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I don't think it gets the XHR status.

* `complete`, `error`, `timeout` or `abort`). The respond method returns the `requestHandler`
* object for possible overrides.
*/
$httpBackend.expect = function(method, url, data, headers, keys) {

Expand Down Expand Up @@ -1876,7 +1850,7 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
* See {@link ngMock.$httpBackend#expect `expect`} for more info.
*/
$httpBackend.expectRoute = function(method, url) {
var pathObj = parseRoute(url);
var pathObj = routeToRegExp(url, {caseInsensitiveMatch: true, ignoreTrailingSlashes: true});
return $httpBackend.expect(method, pathObj.regexp, undefined, undefined, pathObj.keys);
};

Expand Down
46 changes: 3 additions & 43 deletions src/ngRoute/route.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';

/* global routeToRegExp: false */
/* global shallowCopy: false */

// `isArray` and `isObject` are necessary for `shallowCopy()` (included via `src/shallowCopy.js`).
Expand Down Expand Up @@ -210,7 +211,7 @@ function $RouteProvider() {
}
routes[path] = angular.extend(
routeCopy,
path && pathRegExp(path, routeCopy)
path && routeToRegExp(path, routeCopy)
);

// create redirection for trailing slashes
Expand All @@ -221,7 +222,7 @@ function $RouteProvider() {

routes[redirectPath] = angular.extend(
{redirectTo: path},
pathRegExp(redirectPath, routeCopy)
routeToRegExp(redirectPath, routeCopy)
);
}

Expand All @@ -239,47 +240,6 @@ function $RouteProvider() {
*/
this.caseInsensitiveMatch = false;

/**
* @param path {string} path
* @param opts {Object} options
* @return {?Object}
*
* @description
* Normalizes the given path, returning a regular expression
* and the original path.
*
* Inspired by pathRexp in visionmedia/express/lib/utils.js.
*/
function pathRegExp(path, opts) {
var insensitive = opts.caseInsensitiveMatch,
ret = {
originalPath: path,
regexp: path
},
keys = ret.keys = [];

path = path
.replace(/([().])/g, '\\$1')
.replace(/(\/)?:(\w+)(\*\?|[?*])?/g, function(_, slash, key, option) {
var optional = (option === '?' || option === '*?') ? '?' : null;
var star = (option === '*' || option === '*?') ? '*' : null;
keys.push({ name: key, optional: !!optional });
slash = slash || '';
return ''
+ (optional ? '' : slash)
+ '(?:'
+ (optional ? slash : '')
+ (star && '(.+?)' || '([^/]+)')
+ (optional || '')
+ ')'
+ (optional || '');
})
.replace(/([/$*])/g, '\\$1');

ret.regexp = new RegExp('^' + path + '$', insensitive ? 'i' : '');
return ret;
}

/**
* @ngdoc method
* @name $routeProvider#otherwise
Expand Down
46 changes: 46 additions & 0 deletions src/routeToRegExp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
'use strict';

/* global routeToRegExp: true */

/**
* @param path {string} path
* @param opts {Object} options
* @return {?Object}
*
* @description
* Normalizes the given path, returning a regular expression
* and the original path.
*
* Inspired by pathRexp in visionmedia/express/lib/utils.js.
*/
function routeToRegExp(path, opts) {
var keys = [];

var pattern = path
.replace(/([().])/g, '\\$1')
.replace(/(\/)?:(\w+)(\*\?|[?*])?/g, function(_, slash, key, option) {
var optional = option === '?' || option === '*?';
var star = option === '*' || option === '*?';
keys.push({ name: key, optional: optional });
slash = slash || '';
return (
(optional ? '(?:' + slash : slash + '(?:') +
(star ? '([^?#]+?)' : '([^/?#]+)') +
(optional ? '?)?' : ')')
);
})
.replace(/([/$*])/g, '\\$1');

if (opts.ignoreTrailingSlashes) {
pattern = pattern.replace(/\/+$/, '') + '/*';
}

return {
originalPath: path,
keys: keys,
regexp: new RegExp(
'^' + pattern + '(?:[?#]|$)',
opts.caseInsensitiveMatch ? 'i' : ''
)
};
}
34 changes: 29 additions & 5 deletions test/ngMock/angular-mocksSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1941,12 +1941,36 @@ describe('ngMock', function() {
expect(callback).toHaveBeenCalledOnceWith(200, 'path', '', '', 'complete');
}
);
they('should ignore query param when matching in ' + routeShortcut + ' $prop method', methods,
they('should ignore query params when matching in ' + routeShortcut + ' $prop method', methods,
function() {
hb[routeShortcut](this, '/route/:id').respond('path');
hb(this, '/route/123?q=str&foo=bar', undefined, callback);
hb.flush();
expect(callback).toHaveBeenCalledOnceWith(200, 'path', '', '', 'complete');
angular.forEach([
{route: '/route1/:id', url: '/route1/Alpha', expectedParams: {id: 'Alpha'}},
{route: '/route2/:id', url: '/route2/Bravo/?', expectedParams: {id: 'Bravo'}},
{route: '/route3/:id', url: '/route3/Charlie?q=str&foo=bar', expectedParams: {id: 'Charlie', q: 'str', foo: 'bar'}},
{route: '/:x/route4', url: '/Delta/route4?q=str&foo=bar', expectedParams: {x: 'Delta', q: 'str', foo: 'bar'}},
{route: '/route5/:id*', url: '/route5/Echo/456?q=str&foo=bar', expectedParams: {id: 'Echo/456', q: 'str', foo: 'bar'}},
{route: '/route6/:id*', url: '/route6/Foxtrot/456/?q=str&foo=bar', expectedParams: {id: 'Foxtrot/456', q: 'str', foo: 'bar'}},
{route: '/route7/:id*', url: '/route7/Golf/456//?q=str&foo=bar', expectedParams: {id: 'Golf/456', q: 'str', foo: 'bar'}},
{route: '/:x*/route8', url: '/Hotel/123/456/route8/?q=str&foo=bar', expectedParams: {x: 'Hotel/123/456', q: 'str', foo: 'bar'}},
{route: '/:x*/route9/:id', url: '/India/456/route9/0?q=str&foo=bar', expectedParams: {x: 'India/456', id: '0', q: 'str', foo: 'bar'}},
{route: '/route10', url: '/route10?q=Juliet&foo=bar', expectedParams: {q: 'Juliet', foo: 'bar'}},
{route: '/route11', url: '/route11///?q=Kilo', expectedParams: {q: 'Kilo'}},
{route: '/route12', url: '/route12///', expectedParams: {}}
], function(testDataEntry) {
callback.calls.reset();
var paramsSpy = jasmine.createSpy('params');
hb[routeShortcut](this, testDataEntry.route).respond(
function(method, url, data, headers, params) {
paramsSpy(params);
// status, response, headers, statusText, xhrStatus
return [200, 'path', { 'x-header': 'foo' }, 'OK', 'complete'];
}
);
hb(this, testDataEntry.url, undefined, callback);
hb.flush();
expect(callback).toHaveBeenCalledOnceWith(200, 'path', 'x-header: foo', 'OK', 'complete');
expect(paramsSpy).toHaveBeenCalledOnceWith(testDataEntry.expectedParams);
});
}
);
});
Expand Down