atecarlos / protractor-http-mock

A library designed to work alongside Protractor for setting up mocks for your HTTP ajax requests.
MIT License
173 stars 70 forks source link

Instantiate interceptor only once #121

Closed xemle closed 7 years ago

xemle commented 7 years ago

If an interceptor is a anonymous factory (e.g. via $httpProvider.interceptors.push(['$q', function($q) {...}])), the interceptor is created multiple times during the test life cycle. If such interceptor is a stateful singleton like the Angular Loading Bar, the state is ambiguous.

The PR fixes the problem by instantiating the interceptors once and reuses the instances.

henrahmagix commented 7 years ago

Nice first PR @xemle! I noticed that your commit isn't linked to your GitHub profile:

image

I think you just need to add the email address associated with the commit to your profile. See https://help.github.com/articles/why-are-my-contributions-not-showing-up-on-my-profile/#you-havent-added-your-local-git-commit-email-to-your-profile. The email can be seen in git, or on GitHub here: https://github.com/atecarlos/protractor-http-mock/commit/b799952e96bd8868e6045b91544d3badb10ac8b1.patch

I realise this sounds like I'm a maintainer, but I'm not; I just really like this project and got a notification for it =)

atecarlos commented 7 years ago

Hi @xemle. Thank you very much for supporting this project with this PR. We've had issues with angular loading bar for a long time now, but I just never had the time to dive deep into it and find the cause. This may be it! However, I'm hesitant on merging this PR just yet until I fully understand the consequences of this - I would hate to affect users that are somehow depending on a particular order or processing for the interceptors.

I need to refresh my memory on how Angular itself works. I assume that since some projects use singleton interceptors, then the interceptors must be evaluated once in the lifetime of the app - as in your PR, and not once per request - as in the current functionality.

In the mean time, could you write a unit test for this scenario? A feature this important requires a unit test, and also, an example spec in order to do a more comprehensive integration test.

Thanks!

atecarlos commented 7 years ago

Looking at the source code for Angular here: https://github.com/angular/angular.js/blob/master/src/ng/http.js

It seems that what you are doing in this PR is correct. With that said, I would like to have the tests added before we merge this in.

xemle commented 7 years ago

@atecarlos Thank you for investigating this PR. I've added a unit test case with stateful anonymous interceptor. This test breaks in the (unfixed) master.

Regarding spec test: Do you have an idea, how to add such test for this PR? I've read the httpMock.spec.js and did not find a good 'copy-paste-pattern' for a spec test. So some help is appreciated.

While this PR fixes #112, I hope that my PR angular-loading-bar#365 (Avoid multiple interceptor instances due factory) will be accepted, to fix the other side, too :-)

atecarlos commented 7 years ago

Hi @xemle . Sorry for the delay on this. I'm out of the country for a couple of weeks. I'll get to this as soon as I'm back.

atecarlos commented 7 years ago

Merged and published as 0.10.0. Thanks!

xemle commented 7 years ago

Thanks a lot!