-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Basic unit test to go through all the cobra related code #1835
Basic unit test to go through all the cobra related code #1835
Conversation
0ceec9a
to
c7c81d6
Compare
testutil.CheckError(t, false, err) | ||
testutil.CheckContains(t, "Available Commands", output.String()) | ||
testutil.CheckDeepEqual(t, "", errOutput.String()) | ||
} |
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.
Can we also have a test case for invalid command and see if error is returned.
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.
Added!
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.
@tejal29 woot! Discovered a race condition thanks to this test
Codecov Report
@@ Coverage Diff @@
## master #1835 +/- ##
==========================================
- Coverage 47.98% 47.91% -0.08%
==========================================
Files 143 144 +1
Lines 6483 6499 +16
==========================================
+ Hits 3111 3114 +3
- Misses 3089 3101 +12
- Partials 283 284 +1
Continue to review full report at Codecov.
|
c7c81d6
to
76717ae
Compare
cmd/skaffold/app/cmd/cmd.go
Outdated
rootCmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { | ||
if err := SetUpLogs(err, v); err != nil { | ||
return err | ||
} | ||
rootCmd.SilenceUsage = true | ||
logrus.Infof("Skaffold %+v", version.Get()) | ||
|
||
// Avoid race condition |
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.
Can you describe what race condition is it avoiding for dummies like me?
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'm going to simplify the code so that this is not needed anymore.
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.
accept one nit to describe the race condition
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
f2ec08d
to
c48c484
Compare
Signed-off-by: David Gageot [email protected]