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.
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.
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.
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.
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.
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.
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.
Yes this would work fine
There was a problem hiding this comment.
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.
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.
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.
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 testcommand to pass arguments togo test. For instance,operator-sdk -t ./test/e2e/ args -v -parallel=2.Part of issue #387.