Let Serverless Framework handle error #238
Conversation
medikoo
force-pushed
the
improve-exit-on-error
branch
from
3bae69e to
bfedea0
Compare
medikoo
force-pushed
the
improve-exit-on-error
branch
3 times, most recently
from
bf23aeb to
03bee2c
Compare
tbarlow12
left a comment
tbarlow12
left a comment
There was a problem hiding this comment.
Just a couple items to consider, otherwise looks great.
| await invokeLoginHook(false, sls); | ||
| try { | ||
| await invokeLoginHook(false, sls); | ||
| throw new Error("Unexpected"); |
There was a problem hiding this comment.
This error shouldn't need to be thrown. The invocation of the login hook should throw the error.
There was a problem hiding this comment.
This is only to ensure that test will fail if for some reason invokeLoginHook wouldn't crash (see that expect at L96 won't be invoked if we do not throw in try clause - and not invoking expect means that we do not confirm on that behavior).
There was a problem hiding this comment.
Right, but I think if that were the case and this error were actually thrown, the expect assertion within the cache would fail because it's the wrong error. Because we're substituting the interactiveLogin function with an anonymous function that throws an error, it should throw every time. If it doesn't, we have other issues
There was a problem hiding this comment.
Does this test fail if you remove L94?
There was a problem hiding this comment.
Right, but I think if that were the case and this error were actually thrown, the expect assertion within the cache would fail because it's the wrong error.
Exactly, that's the intention
Because we're substituting the interactiveLogin function with an anonymous function that throws an error, it should throw every time. If it doesn't, we have other issues
Yes, and it's exactly why I configured the test to fail, and not to silently pass if unexpectedly invokeLoginHook doesn't crash
Does this test fail if you remove L94?
It wouldn't fail if invokeLoginHook would unexpectedly resolve with success (and other constraints will be met) - and that would be the test setup error.
Anyway I've improved the test by using more appropriate construct, which also should make the intention (what we're actually testing) clear.
| expect(AzureLoginService.servicePrincipalLogin).not.toBeCalled(); | ||
| expect(sls.cli.log).lastCalledWith(`Error: ${errorMessage}`); | ||
| expect(process.exit).toBeCalledWith(0); | ||
| expect(sls.cli.log).lastCalledWith(`Error logging into azure`); |
There was a problem hiding this comment.
nit: Use " instead of ` since there is no string formatting required here
medikoo
force-pushed
the
improve-exit-on-error
branch
2 times, most recently
from
97da989 to
e359795
Compare
medikoo
force-pushed
the
improve-exit-on-error
branch
from
e359795 to
0888fdb
Compare
4 participants
I think plugin should not exit process abruptly. Technically it leaves Framework stopped without any possibility to do cleanup (e.g. send some statistics info).
Additionally Framework implements error reporting in user friendly way, so I believe it's better to just let the error surface, and let Framework pick it.