fix(state): fail transition on exceptions in transition handler

[metamatt]

closes #2772

[christopherthielen]

This change broke all the unit tests. Would you please:

1. Figure out how this broke the unit tests
2. Add a test case that throws an error in onEnter and successfully listens for $stateChangeError

[metamatt]

Thanks @christopherthielen.

Hmm, the tests pass for me locally. I'll look into why Travis is unhappy. Assuming I find and fix the problem, do you prefer a series of separate commits in this PR, or squash (git commit --amend and git push -f) so this PR contains just one working commit?

[christopherthielen]

I prefer one working commit, thanks!

[metamatt]

$q's support for promise.catch seems to have been introduced in Angular 1.3. The travis test suite nicely runs regression tests with all supported versions of Angular. If I just invoke grunt locally I guess it picks one of the newer versions of Angular. The suite that failed in travis was "karma:ng108" (given the $q history, I would expect karma:ng115 and karma:ng1214 to also fail, karma:ng130 and karma:ng149 and karma:150 to succeed).

[metamatt]

Note to self: grunt integrate is what I should have been running, not just grunt default aka grunt.

[christopherthielen]

Ah, thanks for looking into that!

You can run the 'travis style' test suite using grunt integrate on legacy branch and npm run test:integrate on master.

Let's change from .catch(handler) to .then(null, handler)

[metamatt]

Trying to write the requested unit test for the new fix and I'm getting hung up on another $q-ism. I don't understand the $q mocks (test/testUtils.js in ui-router, and qFactory in angular proper) very well. Presumably the ui-router tests are using qFactory to provide a different nextTick because I see it continuing synchronously but I don't know where that happens.

Anyway, the $q processQueue function has some special semantics for exceptions thrown in promise .then routines -- it catches the exception, rejects the promise, but also calls the exceptionHandler (specified to qFactory). And that results in my test failing with

    Error: negative onEnter
        at /home/matt/src/open/ui-router/test/stateSpec.js:125
        at invoke (/home/matt/src/open/ui-router/lib/angular-1.4.9/angular.js:4535)
        at /home/matt/src/open/ui-router/src/state.js:1133
        at processQueue (/home/matt/src/open/ui-router/lib/angular-1.4.9/angular.js:14991)
        at /home/matt/src/open/ui-router/lib/angular-1.4.9/angular.js:15007
        at /home/matt/src/open/ui-router/lib/angular-1.4.9/angular.js:16251
        at /home/matt/src/open/ui-router/lib/angular-1.4.9/angular.js:16069
        at /home/matt/src/open/ui-router/test/testUtils.js:11
        at /home/matt/src/open/ui-router/test/stateSpec.js:567
        at invoke (/home/matt/src/open/ui-router/lib/angular-1.4.9/angular.js:4535)
        at workFn (/home/matt/src/open/ui-router/lib/angular-1.4.9/angular-mocks.js:2517)

where (at the top) stateSpec.js:125 is in my intentionally faulty onEnter callback, (at the bottom) stateSpec.js:567 is the call to $q.flush() in my test which is necessary to note the rejected promise, and (in the middle) angular.js:14991 is the $q glue code for promise.then that should catch the exception and convert it to a promise rejection (which it does, but it also essentially rethrows the exception).

Can you point me at where the ui-router tests set up qFactory?

[metamatt]

I repushed the branch with the updated change (including the test that will fail as described above).

[christopherthielen]

@metamatt I'm in the same boat here, I'm not sure why it's set up to re-throw. The mocks docs imply it's because a throw in a promise is probably a bug...

The key is to call $exceptionHandlerProvider.mode('log'), (instead of rethrow).

  beforeEach(module(function ($stateProvider, $provide, $exceptionHandlerProvider) {
    var x = this;
    var foo = jasmine;
    $exceptionHandlerProvider.mode('log'); // <-- 
    angular.forEach([ A, B, C, D, DD, E, H, HH, HHH ], function (state) {
      state.onEnter = callbackLogger('onEnter');
      state.onExit = callbackLogger('onExit');
    });
    stateProvider = $stateProvider;

    $stateProvider
      .state('A', A)
      .state('B', B)
      .state('C', C)
      .state('D', D)
      .state('DD', DD)
...

However, I'm worried that this could lead to false positives in other tests. I think we should create a new top-level beforeEach(module()) block for just this test

[metamatt]

Great suggestions. I'll try that next time I get a chance. Thanks.

[christopherthielen]

merged