Add unit tests for CLI and Auth packages#55
Conversation
Signed-off-by: amanycodes <[email protected]>
Signed-off-by: amanycodes <[email protected]>
|
Also cc: @aleksmaus @scunningham |
Signed-off-by: amanycodes <[email protected]>
Signed-off-by: amanycodes <[email protected]>
amanycodes
changed the title
|
|
||
| // Log in for community rule updates | ||
| if token, err = auth.Login(ctx, baseAddr, ruleToken); err != nil { | ||
| if token, err = loginUserFunc(ctx, baseAddr, ruleToken); err != nil { |
There was a problem hiding this comment.
Definitely a matter of preference, but let's leave these function calls referencing the package instead of using the helper functions. Prefer to see dependencies immediately as we read the code, so seeing auth.X() or rules.Y() is helpful.
There was a problem hiding this comment.
Definitely a matter of preference, but let's leave these function calls referencing the package instead of using the helper functions. Prefer to see dependencies immediately as we read the code, so seeing auth.X() or rules.Y() is helpful.
I agree with the explicitness being more readable! But actually when I started the tests for initAndExecute I couldn't do direct calls to auth.Login and rules.GetRules since that was then depending on reaching to networks and I was having trouble as they were requiring complex setup. I would love your feedback on how to do this without the variable definition. Thanks!
There was a problem hiding this comment.
Ah that makes sense. I missed you were mocking them in the tests. All good. Can you leave comments above each call just noting why we're doing this? and note the original call in the comment?
tonymeehan
left a comment
tonymeehan
left a comment
There was a problem hiding this comment.
Looks good. Please fix up the function name helpers in cli.go and then we'll merge!
|
Added unit and integration tests for auth to:
|
|
@tonymeehan The changes are getting large, should I raise another one for the upcoming tests? |
amanycodes
changed the title
Good idea. Let's do that. This is looking good, though. Merging now. Nice work improving the coverage! |
2 participants
Added unit and integration tests for cli focusing to:
parseSourceswith valid, invalid an empty data-source files.rules.GetRulesandauth.Loginusing monkey-patching inInitAndExecuteto confirm option parsing.Since there was no dependency injection I couldn't use interfaces for rules.GetRules and auth.Login, so I wrote some package-level function variables for both helping me directly mock them in the test file and isolate them from other (network, fs, etc) dependencies.
Also. while writing tests I saw that the current
datasrc.Validate()anddatasrc.Parsefile()are more permissive and do not currently return errors for cases like: missing source key (considers file just with version: 1.0) and invalid source type (doesn't check the type field)I'll add these checks to the datasrc package once these are confirmed.
Looking forward for feedback and review! Thanks!
Partially closes: #48
cc @tonymeehan