feature: pass test flags through to go test

[mattbnz]

Description

buffalo test (in both the existing stable, and upcoming v2 CLI branch AFAICT) heavily restricts what test flags can be passed through to the underling go test run which makes it hard to take advantage of standard go testing features via buffalo (e.g. -short to trigger a cut-down set of tests).

Could buffalo test be modified so that any unknown flag encountered in the list of arguments be treated as an entry for commandArgs, rather than the packageArgs lists and therefore be correctly passed through to go test ?

Additional Information

No response

[paganotoni]

I agree with this one. I will work on it and keep you posted @mattbnz.

[sio4]

Yeah, I agree too. definitely, we need to fix it.

However, I would like to approach this issue in a slightly different direction from the perspective of command line syntax along with the style of "POSIX vs. Go".

Previously before #168, the boundary of global flags and sub-command flags was not clear and some of the sequences worked but some did not. Inconsistent. Now we have the order of flags like below but still, we don't have a kind of pass-through flags that is related to this issue.

Currently, we have the following syntax:

$ buffalo [global flags...] subcommand [subcommand flags...]

For the test command, we support some pass-through flags (-v, -timeout, and modified -run or -m) as a form of the sub-command flag since they are hardcoded to be allowed or manipulated.

This is the current condition.

Now, I would like to suggest introducing the "end of options" flag which is -- (POSIX standard as far as I know), so we can be more flexible for supporting detailed command line compositing and for future changes on Go. The proposed command line syntax is the following:

$ buffalo [global flags...] subcommand [subcommand flags...] [-- arguments should be passed as is]

With this proposed change, users can provide unlimited flags and arguments to the go test command after the buffalo test does its own tasks such as DBMS preparation for testing and adding some Buffalo-specific flags to the command automatically.

Additionally, the current command line for testing could be something like the below:

$ buffalo test --force-migrations -run TestMyMethod

As you see, the --force-migrations flag is the style of POSIX but the -run flag is of Go. (Regardless I personally don't like Go's command line style, 😄) it could be confusing and inconsistent.

The proposed command line example could be:

$ buffalo test --force-migrations -- -cover -timeout 60s -v -run TestCase

(let go test handles -run part as is)
or to support testify hack,

$ buffalo test --force-migrations --run TestCase -- -cover -timeout 60s -v

(do -testify.m hack per package basis)

Related issues/PRs:

• should not be allowed flags for subcommand before the subcommand itself on the command line #167
• fixed not to allow sub flags before sub command (fixes #167) #168
• buffalo test -m Test commands fail when using sqlite3 databases #180
• fixed broken behavior of buffalo test -m #181

Will continue with summarizing the current behavior.

[sio4]

The current behavior as a record:

Supported pass-through flags are -v and -timeout. They just added as is to the command line before the list of packages. When the user does not provide a list of packages, it could be simply ./... but we add all found packages.

$ buffalo test -v
XXX commandArgs [-v]
XXX packageArgs []
XXX cmd.Run in /home/sio4/git/bt/coco
INFO[0000] go test -p 1 -tags development sqlite -v coco/actions coco/cmd/app coco/grifts coco/locales coco/models coco/public coco/templates

$ buffalo test -timeout 5s
XXX commandArgs [-timeout 5s]
XXX packageArgs []
XXX cmd.Run in /home/sio4/git/bt/coco
INFO[0000] go test -p 1 -tags development sqlite -timeout 5s coco/actions coco/cmd/app coco/grifts coco/locales coco/models coco/public coco/templates

When we use unsupported flags like -cover, it is treated as packageArgs and the command does not work as intended. The following shows the go test ran in the app's root without the package list since it treated -cover as them.
--> need to be fixed, the main topic of this issue.

$ buffalo test -cover
XXX commandArgs []
XXX packageArgs [-cover]
XXX cmd.Run in /home/sio4/git/bt/coco
INFO[0000] go test -p 1 -tags development sqlite -cover 
no Go files in /home/sio4/git/bt/coco

But if we also provide ./... at the end of the above example, it runs as our wish even though it is a bug of the bad behavior 😆

$ buffalo test -cover ./...
XXX commandArgs []
XXX packageArgs [-cover ./...]
XXX cmd.Run in /home/sio4/git/bt/coco
INFO[0000] go test -p 1 -tags development sqlite -cover ./...

-run (or -m) works differently. They are not pass-through flags but will be manipulated by buffalo CLI as follow:

$ buffalo test -run Test_NameOthername
XXX commandArgs []
XXX packageArgs []
XXX cmd.Run in /home/sio4/git/bt/coco/actions
INFO[0001] go test -p 1 -tags development sqlite -testify.m Test_NameOthername
XXX cmd.Run in /home/sio4/git/bt/coco/cmd/app
INFO[0003] go test -p 1 -tags development sqlite -run Test_NameOthername
XXX cmd.Run in /home/sio4/git/bt/coco/grifts
INFO[0004] go test -p 1 -tags development sqlite -run Test_NameOthername
...

CLI will walk through found packages (the package detection is actually required for this) and runs go test with -testify.m when if testify was detected (for actions and models usually) or with -run otherwise (for cmd/app and grifts in this case), as shown above.

Providing the package name at the end of the command line works like the below, the same as the previous buggy example:

$ buffalo test -run Test_NameOthername actions
XXX commandArgs []
XXX packageArgs [actions]
XXX cmd.Run in /home/sio4/git/bt/coco/actions
INFO[0001] go test -p 1 -tags development sqlite -testify.m Test_NameOthername 

However, ./... does not work since it cannot change the working directory to ./... (not a valid directory).

$ buffalo test -run Test_NameOthername ./...
XXX commandArgs []
XXX packageArgs [./...]
XXX cmd.Run in /home/sio4/git/bt/coco
INFO[0000] go test -p 1 -tags development sqlite -run Test_NameOthername
no Go files in /home/sio4/git/bt/coco

Those are some examples of the current behavior.

[sio4]

I wondered if we really need to use the flag -testify.m for method selection since I feel like I usually was able to select a specific method(s) with -run TestSuite/TestMethod. So I researched the histories (even though I cannot fully sure...)

Go versions

It seems like Go 1.7 started to support '/' separation. I am not fully sure what was the status of the support by the way. The release date of the version is 2016-08-15.
https://pkg.go.dev/cmd/[email protected]#hdr-Description_of_testing_flags

Testify

Testify introduced the -testify.m flag (previously -m flag) by stretchr/testify#56 on 2014-06-17. It seems like the flag was introduced to support "method selection" since the standard -run does not support '/' separation so the flag acts as Suite selection when using Testify. No linked issue on the PR, and no details.

Buffalo

The related code with package looping for -testify.m and -run flag switching (the below) was introduced by PR gobuffalo/buffalo#772 on 2017-11-28 when the -run flag already support '/' separation, and was become the current structure mostly by gobuffalo/buffalo#1034 on 2018-04-20. Not fully sure about the details but it seems like either the -run flag support was not fully convenient or missed the change.

https://github.com/gobuffalo/cli/blob/83b148418e7e2ac43e7f6d700f6ace999893fc12/internal/cmd/test/test.go#L152:175

Anyway, I feel like we can deprecate the switching and just support -run to simplify the matching support without loss.

[sio4]

Roughly, I tested with this patch, and it could be OK for all the use cases I described above.

https://gist.github.com/sio4/0d892ca8413e994c3ccb99af1ca744bb

[paganotoni]

Hi @sio4, I saw your patch and I think it would work well only if we want to keep the users limited in terms of the flags that could be passed to underlying the go test command. That is because Cobra will not allow us to pass unknown flags if we set DisableFlagParsing to false.

More broadly, I think we should consider if we should support Testify instead of the go test command. IMO the go test command -run flag is far more standard than testify.m and I will be up for dropping that behavior.

[sio4]

(As a cross note) I wrote detailed comments and my thoughts on #247