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

feat(ngMocks): add $httpBackend.whenOverride method #16251

Conversation

szimek
Copy link

@szimek szimek commented Oct 2, 2017

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

What is the current behavior? (You can also link to an open issue here)
As discussed in #11637 currently $httpBackend.when adds new definitions to the end of the list, making it hard to override any existing definitions.

What is the new behavior (if this is a feature change)?
This PR adds a new method $httpBackend.whenOverride and its short methods (whenOverrideGET etc.) that work almost exactly like $httpBackend.when, but they add definition the the beginning of the list, allowing to override the existing definitions.

Does this PR introduce a breaking change?
Nope.

Please check if the PR fulfills these requirements

Other information:
I'm not sure if I should simply copy docs for $httpBackend.when method and all the "short" methods (e.g. $httpBackend.whenGET etc.) or if I should somehow modify/simplify them.

CC @petebacondarwin

@petebacondarwin petebacondarwin self-assigned this Oct 2, 2017
@petebacondarwin
Copy link
Contributor

Cool @szimek - I'll take a look tomorrow

@szimek szimek force-pushed the add-ngmock-httpbackend-whenoverride-method branch from 203f4c5 to 7193eae Compare October 2, 2017 21:19
@szimek szimek changed the title feat(ngMocks): Add $httpBackend.whenOverride method feat(ngMocks): add $httpBackend.whenOverride method Oct 2, 2017
@@ -1106,6 +1116,22 @@ describe('ngMock', function() {
});


it('should respond with last matched definition', function() {
Copy link
Member

Choose a reason for hiding this comment

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

Mention whenOverride in the description or it is contradicting the one above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like "should match against whenOverride handlers before normal when handlers"?

* @ngdoc method
* @name $httpBackend#whenOverride
* @description
* Creates a new backend definition that will override any already existing definitions with the same parameters
Copy link
Member

Choose a reason for hiding this comment

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

I think it is important t mention that it will take precendence over all previous definitions (even they have different parameters).

Copy link
Contributor

Choose a reason for hiding this comment

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

Something along these lines:

Creates a new backend definition that is put at the front of the queue for matching.
This means that a  matching `whenOverride` definition will always respond even if
there is a more specific `when` definition.

And then give some examples such as (thanks to @gkalpak):

Expected usage:

// Without "override":
$httpBackend.whenPOST('/foo').respond(1);
$httpBackend.whenPOST('/foo', {id: 1}).respond(2);
$http.post('/foo');            // --> 1
$http.post('/foo', {id: 1});   // --> 1

// With "overrride":
$httpBackend.whenPOST('/foo').respond(1);
$httpBackend.whenOverridePOST('/foo', {id: 1}).respond(2);
$http.post('/foo');            // --> 1
$http.post('/foo', {id: 1});   // --> 2

Potentially confusing usage:

// Without "override":
$httpBackend.whenPOST('/foo', {id: 1}).respond(1);
$httpBackend.whenPOST('/foo').respond(2);
$http.post('/foo');            // --> 2
$http.post('/foo', {id: 1});   // --> 1

// With "overrride":
$httpBackend.whenPOST('/foo', {id: 1}).respond(1);
$httpBackend.whenOverridePOST('/foo').respond(2);
$http.post('/foo');            // --> 2
$http.post('/foo', {id: 1});   // --> 2

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Great fast work @szimek.
The code looks good. If we can tighten up the docs then happy to land this.

This PR adds a new method to $httpBackend that allows to override any existing definition,
by adding the new definition at the beginning of the definition list, not at the end of it,
like $httpBackend.when does.

Closes angular#11637
@szimek szimek force-pushed the add-ngmock-httpbackend-whenoverride-method branch from 7193eae to fa04335 Compare October 3, 2017 09:58
@szimek
Copy link
Author

szimek commented Oct 3, 2017

@petebacondarwin I've updated the docs according to your and @gkalpak's suggestions. Thanks! Let me know, if there's anything else.

@petebacondarwin
Copy link
Contributor

petebacondarwin commented Oct 3, 2017

Hi @szimek - thanks for doing that.
I am sorry to be a pain but we have been thinking about the API some more. Not everyone is happy with my suggestion of whenOverride - not even me.
What do you think about the following idea...

// Without "priority":
$httpBackend.whenPOST('/foo').respond(1);
$httpBackend.whenPOST('/foo', {id: 1}).respond(2);
$http.post('/foo');            // --> 1
$http.post('/foo', {id: 1});   // --> 1

// With "priority":
$httpBackend.whenPOST('/foo').respond(1);
$httpBackend.whenPOST('/foo', {id: 1}).priority(10).respond(2);
$http.post('/foo');            // --> 1
$http.post('/foo', {id: 1});   // --> 2

In other words we chain a method call onto the definition that allows us to set its "priority". Then handlers are sorted by this priority.

@szimek
Copy link
Author

szimek commented Oct 3, 2017

No problem ;)

The priority() method looks a bit overcomplicated for me. I guess in the end it gives the same end result, but it reminds me of z-index in CSS, where everyone just adds 1000, 10000, etc. to make sure it doesn't conflict with any previous values ;) If these mocks are reset before every test, then, assuming that the default priority is 0,

$httpBackend.whenPOST('/foo', {id: 1}).priority(1).respond(2)

and

$httpBackend.whenOverridePOST('/foo', {id: 1}).respond(2)

are basically equivalent, because I don't think I'd need to mock the same request with different priorities in a single test. If I need to override a mock defined earlier in the same test, calling whenOverride* again will work anyway.

In our project we simply change definitions.push to definitions.unshift whenever there's a new release of Angular and we never needed to prioritize these mocks. We just got a bunch of default mocks e.g. for different users' login requests and we simply redefine them in specific tests when needed. Of course, I can understand that others might have more sophisticated mocking needs ;) but, I'm not really sure if this priority() method would be that useful.

@petebacondarwin
Copy link
Contributor

Hold off on more changes @szimek before we can nail down something that everyone is happy with.

@petebacondarwin
Copy link
Contributor

@szimek - OK, so sorry about the indecision here. The core team have discussed it and have decided that the best strategy is to add a new configuration switch to ngMock.$httpBackendProvider; it should be something like $httpBackendProvider.matchMostRecentHandlersFirst() or $httpBackendProvider.handlerMatching(mostRecentFirst: true) etc. Feel free to come up with a sensible function.

The default will be the current handling strategy and you have to opt-in to get the latest-first strategy. This will ensure backward compatibility.

Let me know what you think. Feel free to discuss a bit more here before implementing anything, if you want.

@szimek
Copy link
Author

szimek commented Oct 4, 2017

@petebacondarwin Sounds good. I'm going away for 2 weeks, so I'll try to do that once I'm back.

@szimek
Copy link
Author

szimek commented Oct 28, 2017

@petebacondarwin Hey, I might finally find some time tomorrow to tackle it. One question - do you think it's better to change the order of adding new definitions to the definitions array (i.e. https://github.com/angular/angular.js/blob/master/src/ngMock/angular-mocks.js#L1502), or order in which they are matched (https://github.com/angular/angular.js/blob/master/src/ngMock/angular-mocks.js#L1429-L1430)? If it doesn't matter, then the former looks easier.

Regarding function name - what do you think about:

$httpBackendProvider.handlerMatchingOrder('oldestFirst') // default setting
$httpBackendProvider.handlerMatchingOrder('latestFirst')

?

I'm not sure about httpBackendProvider.matchMostRecentHandlersFirst(), because it's "one way" function - there's no way to switch back to the default behaviour, though I don't think anyone will need to switch between the matching order in the middle of running tests.

On the other hand in httpBackendProvider.handlerMatching(mostRecentFirst: true), I'm not sure if it makes sense to accept an object, if there's going to be only one option.

Anyway, I'll already start working on it and the provider function name can be, if needed, renamed later.

EDIT: It might take a bit longer than expected, because I need to figure out how to add $httpBackendProvider along the existing $httpBackendDecorator :/

@petebacondarwin
Copy link
Contributor

Great news @szimek!
Regarding the API name, I think I would like a getter/setter approach. Since we can then flip the default and still allow people to "revert" to the old way of doing it in the future. Perhaps something like: $httpBackendProvider.matchHandlersInDefinitionOrder(value: boolean)?
That way the API call just takes a false (or a true to revert to compatibility mode).

Regarding working around the decorator, perhaps we could afford to put this as method on the mock service itself? As long as it is set before you start adding matchers then it would be fine. You could just set it in an extra module, with a run block...

angular.module('http-test-helper', ['ngMock'])
  .run(function($httpBackend) {
    $httpBackend.$matchHandlersInDefinitionOrder(false);
  });

beforeEach(module('http-test-helper'));

@Narretz
Copy link
Contributor

Narretz commented Feb 28, 2018

@szimek are you still planning on finishing this PR? Would be good to know so we can plan accordingly.

@Narretz Narretz modified the milestones: 1.6.x, 1.7.x Apr 12, 2018
@Narretz Narretz self-assigned this May 3, 2018
Narretz added a commit to Narretz/angular.js that referenced this pull request May 9, 2018
Narretz added a commit to Narretz/angular.js that referenced this pull request May 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants