-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix($mdIcon): prevent "Possibly unhandled rejection" errors #8468
fix($mdIcon): prevent "Possibly unhandled rejection" errors #8468
Conversation
…cing more consistent
The `announceNotFound()` function was creating and returning a rejected promise that was neither handled nor exposed to the user, causing "Possibly unhandled rejection" errors.
$httpBackend.flush(); | ||
|
||
expect(caughtRejection).toBe(true); | ||
expect($log.warn.logs[0]).toEqual([errorMessage]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a new feature? I did not know that that a log stack was maintained ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it's not new !
But even some tests in Angular itself seem to ignore this feature :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(BTW, this is only for tests obviously.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I.E. ngMock
provides a mock version of $log
that keeps a collection of the calls that are made to each of its logging methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this learning tips!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gkalpak @petebacondarwin Is this different than the $exceptionHandler
stack that is maintained?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@topherfangio: It isn't related to $exceptionHandler
.
ngMock
's $log
service will keep track of the arguments passed when calling any of its methods. (It also has additional, handy methods for resetting and asserting emptyness).
Although the core (production) $exceptionHandler
basically delegates to $log.error()
, ngMock
's $exceptionHandler
doesn't use $log
at all. It just stores errors in an array (exposed as $exceptionHandler.errors
) and might also throw if the mode is set to rethrow
(the default).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In tests $exceptionHandler
is overridden by ngMock
. This test version has two modes log
and rethrow
. In both cases the exception is written to the $exceptionHandler.errors
array.
Also in tests the $log
is overridden by ngMock
. This test version does not write to the console but instead writes to arrays, one for each of the possible logging methods, log
, warn
, info
, error
, debug
.
In this particular test, I believe that we are not hitting $exceptionHandler
at all as we are handling the error and calling $log.warn
.
The
announceNotFound()
function was creating and returning a rejected promise that was neitherhandled nor exposed to the user, causing "Possibly unhandled rejection" errors.
This should fix the failing tests against Angular snapshot on CI.
This PR also includes a `refactor` commit (removing some unnecessary module loading and making whitespace a little more consistent 😛) and a `test` commit that more thoroughly covers the actual behavior when icons/groups are not found (i.e. logging + rejecting).