-
Notifications
You must be signed in to change notification settings - Fork 68
Adding tests for validating remove-clusters command flags #152
Adding tests for validating remove-clusters command flags #152
Conversation
@perotinus Can you please review this one? |
1a24c68
to
23bd70f
Compare
"github.com/GoogleCloudPlatform/k8s-multicluster-ingress/app/kubemci/pkg/kubeutils" | ||
) | ||
|
||
// Test to verify validate. |
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.
nit: Go style is to have the function name as the first word in the comment.
TestValidateRemoveClusterArgs tests all flags...
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.
Done
func TestValidateRemoveClustersArgs(t *testing.T) { | ||
// validateRemoveClustersArgs should return an error with empty options. | ||
options := removeClustersOptions{} | ||
if err := validateRemoveClustersArgs(&options, []string{}); err == nil { |
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.
This looks like it could be a table-style test. But I suppose that could come in a follow-up PR.
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.
ack
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.
ack
/lgtm unless you want to modify to table-based tests. |
23bd70f
to
05a3b5a
Compare
Thanks @perotinus |
Follow up to #146.
Adding tests to verify flags are validated as expected. This is similar to tests for other commands.
cc @csbell @G-Harmon @perotinus