fix($httpBackend): Add missing expectHEAD() and expectOPTIONS() methods #7320
Conversation
| @@ -1520,7 +1520,7 @@ function createHttpBackendMock($rootScope, $delegate, $browser) { | |||
|
|
|||
|
|
|||
| function createShortMethods(prefix) { | |||
| angular.forEach(['GET', 'DELETE', 'JSONP'], function(method) { | |||
| angular.forEach(['GET', 'DELETE', 'JSONP', 'HEAD', 'OPTIONS'], function(method) { | |||
There was a problem hiding this comment.
OPTIONS is perfectly capable of containing a body, per the RFC. It's not clear that it's appropriate to be added here, because these short methods never include their body. It might be more suitable to created it with createShortMethodsWithData() if we have that in angular-mocks
There was a problem hiding this comment.
Good catch. Perhaps I should move OPTIONS to the section below that handles PUT, POST, and PATCH?
There was a problem hiding this comment.
Actually, I'm fairly sure we don't want these, yeah :( I can understand wanting to expect those requests, but they aren't going to be made with angular-mocks unless you ask for them, because no real XHR request actually gets made.
So unless your app manually tries to make an OPTIONS or HEAD request using the options/head methods (which aren't included, afaik), then this doesn't really accomplish anything
There was a problem hiding this comment.
Ah, apparently the 'head' method is created as a short method. So maybe it's just the options one we don't want.
There was a problem hiding this comment.
I added HEAD support because I am using $http({method: "HEAD", url: ...}) in my application, but when I went to unit test it with $httpBackend, I found that $httpBackend.expectHEAD was undefined. Since it was in the docs, I assumed this must be a bug: https://docs.angularjs.org/api/ngMock/service/$httpBackend#expectHEAD
There was a problem hiding this comment.
Angular-mocks doesn't create a real request, so the browser isn't going to jump in and say "no, do an OPTIONS request first" as it occasionally will for real requests, case depending. But yes you're correct that we do want expectHEAD()
There was a problem hiding this comment.
Well, unless your app is actually sending an "OPTIONS" request on its own (which in my opinion, is a fairly rare occurrence), then you probably don't need it.
If you do want to add it, since it can contain data, it should be moved below to the second forEach, since it can have a body.
There was a problem hiding this comment.
Ok, I'll nuke this PR and create a new one that only adds expectHEAD(). Stand by for another exciting installment!
There was a problem hiding this comment.
Er, you don't need to nuke the PR, just commit --amend and force push ;)
|
alright, it generally LGTM, lets just wait for travis (as a formality!) |
|
btw, I got some jshint failures while running the grunt test in code unrelated to my changes. Not sure if that will fail the Travis build. |
| expect(typeof hb.expectPATCH).toBe("function"); | ||
| expect(typeof hb.expectDELETE).toBe("function"); | ||
| expect(typeof hb.expectHEAD).toBe("function"); | ||
| }); |
There was a problem hiding this comment.
The jshint error is here, it doesn't like your indentation. The expect(...) calls should be indented 2 spaces, rather than 4
There was a problem hiding this comment.
After checking it out, it looks like the lines don't have a real 0x10 newline character between them, so they all appear on the same line.
There was a problem hiding this comment.
According to xxd, it has the proper 0x0a (a.k.a "\n") char at the end of each line.
This was documented but not implemented. With accompanying unit test to ensure the $httpBackend.expect*() methods exist.
|
Amended to fix spacing. |
|
Did I say |
|
well, it's merged. github is being laggy showing it, but it's merged |
|
Oh my goodness. It's like a dream come true! My first AngularJS PR. Thanks for the guidance. |
This was documented but not implemented. With accompanying unit test to ensure the $httpBackend.expect*() methods exist. Closes angular#7320
|
Is there any chance this can get into 1.2.x? It didn't make it into 1.2.18 or 1.2.17. Is it planned for 1.2.19? |
This method was documented but not implemented.
With accompanying unit test.
CLA signed as [email protected].