bug(ngMock): errors thrown to reject promises are not caught

[bclinkinbeard]

Request Type: bug

How to reproduce: Include angular and angular-mocks on a page and run this code. Error is printed to the console even though the promise chain is configured to recover from the error.

var inject = angular.injector(['ng', 'ngMock']).invoke

inject(function ($http, $httpBackend) {

  $httpBackend.expectGET('http://example.com').respond('foo')

  $http.get('http://example.com')
    .then(function (result) {
      throw new Error('Intentional rejection')
    })
    .then(null, function (err) {
      console.log(err);
    })

  $httpBackend.flush()
})

Component(s): ngMock

Impact: small

Complexity: medium

This issue is related to:

Detailed Description:

This behavior can be corrected by wrapping a try...catch around the call to callback around line 1124 of angular-mocks, but that breaks 3 $compile tests and I'm not sure what to do with the caught error.

In its current state, unit testing promise chains must be done with ngMockE2E.

Other Comments:

[bclinkinbeard]

It seems I underestimated the scope of this issue. The real $http exhibits this behavior as well, and the ngMock module does not even allow for recovery. I've put together a full demo at https://github.com/bclinkinbeard/http-and-httpBackend-issues. Clone the repo, npm install, then npm run serve to see the various permutations of behavior.

[pkozlowski-opensource]

Effectively, this doesn't sound quite right... If we use $q.reject to denote problem in the success handler things are behaving as expected: http://plnkr.co/edit/U9HTxZKl5aYWkS7sn1Hf?p=preview

Need to dig into this one as I saw a similar issue already...

[caitp]

@pkozlowski-opensource the aplus spec basically says that if a fulfilled callback throws, we have to then turn this into a rejection.

I think the update to $q to improve aplus compat fixes this, if that wasn't merged yet

[caitp]

@lgalfaso has one PR for this at #5976 which hasn't been merged yet, and I know there was another one out there too which probably hasn't landed either. So this is a known issue which just hasn't had a fix land yet

[pkozlowski-opensource]

@caitp yup, 2.2.7.2 of http://promises-aplus.github.io/promises-spec/:

If either onFulfilled or onRejected throws an exception e, promise2 must be rejected with e as the reason.

But I was under the impression that $q behaves like this already and it was only a pb with mocks, let me quickly check

[bclinkinbeard]

To be clear, $q does reject, but the error also makes it to the console, which it shouldn't.

[pkozlowski-opensource]

Uffff, I've understood the last @caitp comment in the way that I thought that things are broken-broken and got really scared here... :-) But it is not that bad, or at least not as bad I thought it be: http://plnkr.co/edit/2fWdONIUOuNUUkBflPoS?p=preview

So, to summarise, we seem to be dealing with 2 separate (from the functional point of view) issues here:

• $q will log things thrown from a success handler which is unnecessary
• in ngMock throwing an exception will make the test fail while it shouldn't

@bclinkinbeard is my understanding correct here?

@caitp @lgalfaso what is the state of the mentioned PRs? I should have some "free" time in the coming days so could help out. Maybe we should break those PRs into smaller pieces that are easier to review?

[caitp]

I think Lucas is a bit preoccupied with getting the bind-once stuff landed today, I can look at how the aplus 1.1 PR could be split into smaller chunks with you @pkozlowski-opensource

[bclinkinbeard]

Yes, I believe that sums it up nicely.