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

thorn0 · 2018-06-03T22:45:53Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

fix

What is the current behavior? (You can also link to an open issue here)

#14173

Does this PR introduce a breaking change?

no

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

gkalpak

Actually, I think it would be better to re-use the method used in ngRoute (see #14173 (comment)).
It would be similar to how we re-use shallowCopy() (configured in angularFiles.js).

gkalpak · 2018-06-04T06:01:36Z

src/ngMock/angular-mocks.js

@@ -1680,14 +1681,14 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
          + (optional ? '' : slash)
          + '(?:'
          + (optional ? slash : '')
-          + (star && '(.+?)' || '([^/]+)')
+          + (star ? '(.+?)' : '([^/?]+)')


Maybe also add # (just in case).

thorn0 · 2018-06-04T18:25:37Z

@gkalpak Where is it better to place the file with this function? src? src/ngRoute?

gkalpak · 2018-06-04T18:34:11Z

I would put it in src/ (since it will be used in different modules).

thorn0 · 2018-06-07T21:17:33Z

@gkalpak PTAL

gkalpak

Looks great 👍
Just a couple of minor things.

gkalpak · 2018-06-07T21:45:23Z

src/ngMock/angular-mocks.js

@@ -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:


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

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

Sorry, I misread that 😳

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 😞

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)?

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

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?

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) 😛

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

I wouldn't be opposed to that 😃

gkalpak · 2018-06-07T21:47:40Z

src/ngMock/angular-mocks.js

   *    – 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:


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

gkalpak · 2018-06-07T21:56:27Z

src/routeToRegExp.js

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


!!optional --> optional (now that optional is boolean)

gkalpak · 2018-06-07T22:22:52Z

src/routeToRegExp.js

+ * Inspired by pathRexp in visionmedia/express/lib/utils.js.
+ */
+function routeToRegExp(path, opts) {
+  if (!angular.isString(path)) path = '';


AFAICT, this function would never be called with something that is not a string (if people followed the docs 😁).
I suggest we remove this line (and fail hard if path is not a string, so that we can detect issues quicker), instead of covering it up with a route that does not correspond to the passed in path.

gkalpak · 2018-06-07T22:23:51Z

test/ngMock/angular-mocksSpec.js

+            hb[routeShortcut](this, '/route/:id').respond(function(method, url, data, headers, params) {
+              paramsSpy(params);
+              // status, response, headers, statusText, xhrStatus
+              return [200, 'path', { 'x-header': 'foo' }, 'OK', 'complete'];


I don't think complete is used (here and below).

I'm verifying that params are correct. Without the fix, paramSpy is called with {id: '123?q=str&foo=bar', q: 'str', foo: 'bar'}. So I'm testing exactly what is said in the name of the test: "should ignore query params when matching".

gkalpak · 2018-06-07T22:26:11Z

test/ngMock/angular-mocksSpec.js

@@ -1943,10 +1943,30 @@ describe('ngMock', function() {
        );
        they('should ignore query param when matching in ' + routeShortcut + ' $prop method', methods,
          function() {
-            hb[routeShortcut](this, '/route/:id').respond('path');


Why the changes in this test? What are you trying to verify?

gkalpak · 2018-06-07T22:27:56Z

test/ngMock/angular-mocksSpec.js

+        they('should ignore query param when matching eager parameters in ' + routeShortcut + ' $prop method', methods,
+          function() {
+            var paramsSpy = jasmine.createSpy('paramsSpy');
+            hb[routeShortcut](this, '/route/:id*').respond(function(method, url, data, headers, params) {


How about adding a test without the * (for good measure)?

I misread this question before. There already is a test without *. I added a test with *. I thought you were asking about a test without any routing parameters at all.

thorn0 · 2018-06-08T17:44:30Z

@gkalpak Should the URL /route/123/ match the route /route/:id?

gkalpak · 2018-06-08T18:25:54Z

I don't think it matters much (for tests). I don't have a strong opinion (unless changing the current behavior is a breaking change).
(Let's keep the current behavior in this PR though.)

thorn0 · 2018-06-08T18:52:18Z

The current behavior is a bit unexpected:

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

Which means even /route/123/bla/bla/bla matches the route /route/:id. Do we really want to keep it?

gkalpak · 2018-06-08T19:03:45Z

No, I meant the behavior wrt trailing /. Matching URLs both with and without a trailing / is reasonable (and is also how $route works by default - which is unrelated, but still 😁).

Matching the bla/bla/bla part is a bug imo. If one wants to do that, they can use a :rest*? segment at the end.

So, I woud vote for matching with and without trailing /, but not match with trailing segments (e.g. bla/bla/bla). WDYT?

thorn0 · 2018-06-08T20:05:03Z

Okay. Matching the bla/bla/bla part isn't documented, so we can consider it a bug.

thorn0 · 2018-06-09T10:10:37Z

@gkalpak PTAL

…en}Route Fixes angular#14173

gkalpak · 2018-06-18T16:50:44Z

Thx, @thorn0 ❤️
(Added some more info in the commit message and merged.)

…en}Route Previously, a route definition such as `$httpBackend.whenRoute('GET', '/route/:id')` matched against a URL with query params, for example `/route/1?q=foo`, would incorrectly include the query params in `id`: `{id: '1?q=foo', q: 'foo'}`. This commit fixes it, so that the extracted `params` will now be: `{id: '1', q: 'foo'}`. Fixes #14173 Closes #16589

googlebot added the cla: yes label Jun 3, 2018

thorn0 force-pushed the whenRoute branch from c451a45 to a445526 Compare June 4, 2018 08:26

gkalpak reviewed Jun 4, 2018

View reviewed changes

thorn0 force-pushed the whenRoute branch 9 times, most recently from f984b29 to 04024ed Compare June 6, 2018 08:55

gkalpak suggested changes Jun 7, 2018

View reviewed changes

thorn0 force-pushed the whenRoute branch from 04024ed to 7812cfb Compare June 8, 2018 18:00

thorn0 force-pushed the whenRoute branch 3 times, most recently from ad838e1 to 82785e6 Compare June 9, 2018 09:33

fix(ngMock/$httpBackend): correctly ignore query params in {expect,wh…

6f6c180

…en}Route Fixes angular#14173

thorn0 force-pushed the whenRoute branch from 82785e6 to 6f6c180 Compare June 10, 2018 15:34

Narretz added this to the 1.7.x milestone Jun 18, 2018

gkalpak approved these changes Jun 18, 2018

View reviewed changes

gkalpak closed this in 840b5f0 Jun 18, 2018

dependencies bot mentioned this pull request Aug 9, 2018

Update 6 dependencies from npm JetBrains/ring-ui#349

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

thorn0 commented Jun 3, 2018

gkalpak left a comment

gkalpak Jun 4, 2018

thorn0 commented Jun 4, 2018

gkalpak commented Jun 4, 2018

thorn0 commented Jun 7, 2018

gkalpak left a comment

gkalpak Jun 7, 2018

thorn0 Jun 7, 2018

gkalpak Jun 8, 2018

gkalpak Jun 8, 2018

thorn0 Jun 8, 2018

gkalpak Jun 8, 2018

thorn0 Jun 8, 2018 •

edited

Loading

gkalpak Jun 8, 2018

thorn0 Jun 8, 2018 •

edited

Loading

gkalpak Jun 8, 2018

gkalpak Jun 7, 2018

gkalpak Jun 7, 2018

gkalpak Jun 7, 2018

gkalpak Jun 7, 2018

thorn0 Jun 7, 2018 •

edited

Loading

gkalpak Jun 7, 2018

gkalpak Jun 7, 2018

thorn0 Jun 8, 2018

gkalpak Jun 8, 2018

thorn0 commented Jun 8, 2018

gkalpak commented Jun 8, 2018

thorn0 commented Jun 8, 2018

gkalpak commented Jun 8, 2018 •

edited

Loading

thorn0 commented Jun 8, 2018

thorn0 commented Jun 9, 2018 •

edited

Loading

gkalpak commented Jun 18, 2018

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

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

Conversation

thorn0 commented Jun 3, 2018

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thorn0 commented Jun 4, 2018

gkalpak commented Jun 4, 2018

thorn0 commented Jun 7, 2018

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thorn0 Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thorn0 Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thorn0 Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thorn0 commented Jun 8, 2018

gkalpak commented Jun 8, 2018

thorn0 commented Jun 8, 2018

gkalpak commented Jun 8, 2018 • edited Loading

thorn0 commented Jun 8, 2018

thorn0 commented Jun 9, 2018 • edited Loading

gkalpak commented Jun 18, 2018

thorn0 Jun 8, 2018 •

edited

Loading

thorn0 Jun 8, 2018 •

edited

Loading

thorn0 Jun 7, 2018 •

edited

Loading

gkalpak commented Jun 8, 2018 •

edited

Loading

thorn0 commented Jun 9, 2018 •

edited

Loading