-
Notifications
You must be signed in to change notification settings - Fork 160
test: Unit tests for deploy, login, package, delete and APIM plugins #148
Conversation
PIC123
left a comment
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.
LGTM overall
mydiemho
left a comment
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.
marvelous job! have some questions about some changes, let me know if any doesn't make sense
| catch (e) { | ||
| this.serverless.cli.log('Error logging into azure'); | ||
| this.serverless.cli.log(e); | ||
| this.serverless.cli.log(`${e}`); |
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.
we should throw error and short circuit when login failed.
| BindingUtils.getBindingsMetaData(sls); | ||
| expect(sls.cli.log).toBeCalledWith('Parsing Azure Functions Bindings.json...'); | ||
| }); | ||
| }); |
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.
are we testing other methods in a different PR?
| @@ -0,0 +1,24 @@ | |||
| export const constants = { | |||
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.
config.js also export constants, is this gonna clash?
|
|
||
| describe('Login Plugin', () => { | ||
|
|
||
| const authResponse = MockFactory.createTestAuthResponse(); |
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.
lots of duplicate code for setting up AzureLoginService. can we move some of them to a beforeEach?
AzureLoginServiceinvokeHook)