-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
commands/.../test.go: allow passing of flags to go test
#392
Conversation
commands/operator-sdk/cmd/test.go
Outdated
if verbose { | ||
testArgs = append(testArgs, "-v") | ||
if args[0] != "args" { | ||
log.Fatalf("Unrecognized argument `%s`; please use the `args` argument if passing arguments/flags to `go test`", args[0]) |
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 see that the error message starts with mix case as indicated #380. We should define a convention to follow to make the code style more consistent.
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.
Maybe I could make a test for to check for this in travis.
@@ -49,20 +49,21 @@ func NewTestCmd() *cobra.Command { | |||
testCmd.Flags().StringVarP(&crdManifestPath, "crd", "c", "deploy/crd.yaml", "Path to CRD manifest") |
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.
go test also takes in -c
and -o
flags. Will that conflicts to the operator-sdk test flags -c
and -o
?
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.
That won't cause conflict. The way that Flags().SetInterspersed(false)
works is that any arguments/flags given after the first argument it sees after flags (argument being anything that doesn't start with a -
) is not processed and instead is given to us in args []string
. For instance, if I run operator-sdk test -t ./test/e2e -k ~/.kube/config args -v -parallel 2
, I get this as my args []string
: ["args" "-v" "-parallel" "2"]
.
commands/operator-sdk/cmd/test.go
Outdated
@@ -49,20 +49,21 @@ func NewTestCmd() *cobra.Command { | |||
testCmd.Flags().StringVarP(&crdManifestPath, "crd", "c", "deploy/crd.yaml", "Path to CRD manifest") | |||
testCmd.Flags().StringVarP(&opManifestPath, "operator", "o", "deploy/operator.yaml", "Path to operator manifest") | |||
testCmd.Flags().StringVarP(&rbacManifestPath, "rbac", "r", "deploy/rbac.yaml", "Path to RBAC manifest") | |||
testCmd.Flags().BoolVarP(&verbose, "verbose", "v", false, "Enable verbose go test") | |||
testCmd.Flags().SetInterspersed(false) |
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.
How does this work?
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.
See above comment
commands/operator-sdk/cmd/test.go
Outdated
func NewTestCmd() *cobra.Command { | ||
testCmd := &cobra.Command{ | ||
Use: "test --test-location <path to tests directory> [flags]", | ||
Use: "test --test-location <path to tests directory> [flags] args [go test 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.
Can I do operator-sdk test --test-location=e2e -c=deploy/crd.yaml -o=deploy/operator.yaml args -c -o=e2e.test
? Where the latter -c
and -o
are the go test 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.
Yes this would work fine
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 see. Also why can't we use a passthough
flag instead? It seems to me that it is not too clear on how to use the args
for passing go test flags; without seeing an example, I might not know the proper way to pass go test flags. It might be more clear if we have a explicit passthrough flags such as --go-test-flags
to indicate that the value is for go test flags. For instance, we use operator-flags
for operator-sdk up local
. Maybe we should do the same for operator-sdk test
.
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.
Yeah, there's 2 ways of handling this right now. We either use args
as a separator between the go test args and the sdk's args or we ask people to put all of their args into a string (so we would have something like operator-sdk test -t ./test/e2e --passthrough "-v -parallel 2"
).
@hasbro17 what do you think would be the better choice for this?
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 vote for using the passthrough flag for a potential better user experience and the consistency with operator-sdk up local --operator-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.
Yeah we can use an explicit flag to pass on the go test flags. Lets call it --go-test-flags
instead of --passthrough
though to be clear.
Updated to work with |
LGTM |
lgtm, but we also should add a more detailed documentation on how to use the |
This allows users of the
operator-sdk test
command to pass arguments togo test
. For instance,operator-sdk -t ./test/e2e/ args -v -parallel=2
.Part of issue #387.