Skip to content
This repository has been archived by the owner on Mar 27, 2024. It is now read-only.

Task improving test #247

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ func cmd() *cobra.Command {
generateCmd := generate.Cmd()
destroyCmd := destroy.Cmd()
versionCmd := version.Cmd()
testCmd := test.Cmd()
testCmd := test.Cmd
devCmd := dev.Cmd()
taskCmd := task.Cmd()
routesCmd := routes.Cmd()
Expand Down
112 changes: 112 additions & 0 deletions internal/cmd/test/build_cmd.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
package test

import (
"os"
"os/exec"
"strings"

"github.com/gobuffalo/envy"
"github.com/gobuffalo/meta"
)

var (
// if GO_BIN is set, use it, otherwise use "go"
goBinary = envy.Get("GO_BIN", "go")

// testValuedFlags are the flags on the go test command that
// receive a value, we use this to determine if the last argument
// is a flag or a package path
testValuedFlags = strings.Join([]string{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if having a static list of flags is a good way to maintain especially since we support multiple go versions which could have different sets of flags. For example, -covermode and -coverpkg are missing.

Also, this approach does not support -args flag even though I am not sure how many developers use this flag with the apps built with Buffalo.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I don't like this part. I added this to be able to understand when a flag received values, this is important to determine if the test command is being passed packages to test. p.e:

go test //
go test -run xxx // No packages
go test -run xxx -p 1 // No packages
go test ./... // packages specified

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I know this is added to check if the user gave the module list to test or just gave flags (with values).

My idea on this is simple. I think we can just ignore adding an automatically found module list when the user gave additional flags for go test. Previously when we need to walk through the given (or automatically collected) modules for -m supporting before, with switching between -testify.m and -run, so the list of target modules was important. However, if we deprecate the behavior of -m we had and just use the standard -run, we don't need to care about it anymore and we can let the user complete the command line as the same as when the user runs go test directly.

Briefly, I would like to make the command the same or similar to the go test command line:

  • buffalo test -- -cover ./... == go test -cover ./... after preparing test db (just use the user-provided args as is)
  • buffalo test == go test after preparing test db (just use the user-provided args as is, which is none)
    • or buffalo test == go test ./... as a special exception (add ./... for when there is no args at all)

So make buffalo test [...] equivalent to go test [...] except for the database preparing part, it could be easy to understand the behavior since all Buffalo users are Go developers, and it is more expectable.

`-p`, `-asmflags`, `-buildmode`, `-compiler`, `-gccgoflags`,
`-gcflags`, `-installsuffix`, `-mod`, `-modfile`, `-overlay`,
`-pkgdir`, `-tags`, `-trimpath`, `-toolexec`, `-o`, `-exec`,
`-bench`, `-benchtime`, `-blockprofile`, `-blockprofilerate`,
`-count`, `-coverprofile`, `-cpu`, `-cpuprofile`, `-fuzz `,
`-fuzzcachedir`, `-fuzzminimizetime`, `-fuzztime`, `-list`,
`-memprofile`, `-memprofilerate`, `-mutexprofile`, `-mutexprofilefraction`,
`-outputdir`, `-parallel`, `-run`, `-shuffle`, `-testlogfile`,
`-timeout`, `-trace`,
}, "|")
)

// buildCmd builds the test command to be passed to the go test command
// after cutting some of the arguments that are specific to buffalo and adding
// packages if missing.
func buildCmd(args []string) (*exec.Cmd, error) {
app := meta.New(".")

ccar := clean(args)
cargs := append([]string{
"test",
// run sequentially
"-p", "1",
// add build tags
"-tags", app.BuildTags("development").String(),
//TODO: Should we merge it with passed tags?
}, ccar...)
paganotoni marked this conversation as resolved.
Show resolved Hide resolved

// if no packages are specified, add the current directory
lastIsFlag := len(ccar) > 0 && strings.HasPrefix(args[len(ccar)-1], "-")
lastIsFlagValue := len(ccar) >= 2 && strings.Contains(testValuedFlags, ccar[len(ccar)-2])
if len(ccar) == 0 || lastIsFlag || lastIsFlagValue {
pkgs, err := findPackages()
if err != nil {
return nil, err
}

cargs = append(cargs, pkgs...)
}

// Add extra args (-testify.m) to the command
cargs = append(cargs, extraArgs(args)...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that the function clean() (only) removes -m and -testify.m from the args and the function extraArgs() appends -testify.m at the end of the command line after the list of packages when one of -m, -testify.m, or -run was specified. However, it will not work if the user provides -run TestMethod since when a user wants to run a specific test case in a single Suite, the proper command line should be something like go test -run TestSuite -testify.m TestMethod. (see another comment for the old code)

Also, -run does not always mean to be -testify.m. E.g. I have a package named utils in my app which does not use testify (suite) based test cases which to be run with the standard -run TestUtilsFunc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The biggest issue I'm facing is maintaining a functionality that I don't see documented by our tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, we just mentioned the feature in short words, we just supported a simple use case only, and that is for the season of v0.10, which is the season around Go v1.7 or v1.8 maybe. I think we could adapt the testing to the modern version of Go and Buffalo.

https://gobuffalo.io/documentation/guides/testing/#execute-a-single-test

Copy link
Member Author

@paganotoni paganotoni Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. One consideration to bring here is that the -m feature can be addressed with -run, developers just have to enter a larger name for the tests. In general I would love to see Buffalo being closer to the stdlib for our upcomming versions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I would like to make it easy for Go developers who already know how go works.


cmd := exec.Command(goBinary, cargs...)
cmd.Stdin = os.Stdin
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr

return cmd, nil
}

// Removes flags that are only used by the Buffalo test command and should not be passed to
// the Go test command. Those flags are:
// * --force-migrations
// * -m
// * -testify.m
func clean(args []string) []string {
var cleaned []string
for ix, v := range args {
if v == "--force-migrations" {
continue
}

if v == "-m" || (ix > 0 && !strings.HasPrefix(v, "-") && args[ix-1] == "-m") {
continue
}

if v == "-testify.m" || (ix > 0 && !strings.HasPrefix(v, "-") && args[ix-1] == "-testify.m") {
continue
}

cleaned = append(cleaned, v)
}

return cleaned
}

// Adds extra flags to the go test command for the testify functionality if the
// -testify.m or -m flags are passed.
func extraArgs(args []string) []string {
var extra []string
for ix, v := range args {
if ix >= len(args)-1 || strings.HasPrefix(args[ix+1], "-") {
continue
}

if v == "-m" || v == "-testify.m" || v == "-run" {
extra = append(extra, "-testify.m", args[ix+1])
}
}

return extra
}
99 changes: 99 additions & 0 deletions internal/cmd/test/build_cmd_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
package test

import (
"strings"
"testing"
)

func TestBuildCmd(t *testing.T) {
tcases := []struct {
name string
args []string
want []string
contains string
}{
{
name: "no args",
args: []string{},
want: []string{"go", "test", "-p", "1", "-tags", "development"},
},

{
name: "path specified",
args: []string{"./cmd/..."},
want: []string{"go", "test", "-p", "1", "-tags", "development", "./cmd/..."},
},

{
name: "path specified and flags",
args: []string{"--count=1", "./cmd/..."},
want: []string{"go", "test", "-p", "1", "-tags", "development", "--count=1", "./cmd/..."},
},

{
name: "flags but no path",
args: []string{"--count=1", "-v"},
want: []string{"go", "test", "-p", "1", "-tags", "development", "--count=1", "-v"},
},

{
name: "flags but no path",
args: []string{"--count=1", "-v"},
want: []string{"go", "test", "-p", "1", "-tags", "development", "--count=1", "-v"},
},

{
name: "flags no path with -tags",
args: []string{"--count=1", "-tags", "foo,bar"},
want: []string{"go", "test", "-p", "1", "-tags", "development", "--count=1", "-tags", "foo,bar"},
},

{
name: "force migrations should be removed",
args: []string{"--force-migrations", "-tags", "foo,bar"},
want: []string{"go", "test", "-p", "1", "-tags", "development", "-tags", "foo,bar"},
},

{
name: "testify.m through -m",
args: []string{"--force-migrations", "-m", "Something"},
want: []string{"go", "test", "-p", "1", "-tags", "development"},
contains: "-testify.m Something",
},

{
name: "testify.m through -run",
args: []string{"--force-migrations", "-run", "Something"},
want: []string{"go", "test", "-p", "1", "-tags", "development", "-run", "Something"},
contains: "-testify.m Something",
},

{
name: "testify.m should go at the end",
args: []string{"--force-migrations", "-testify.m", "Something"},
want: []string{"go", "test", "-p", "1", "-tags", "development"},
contains: "-testify.m Something",
},
}

for _, tc := range tcases {
t.Run(tc.name, func(t *testing.T) {
cmd, err := buildCmd(tc.args)
if err != nil {
t.Fatal(err)
}

wantcmd := strings.Join(tc.want, " ")
resultcmd := strings.Join(cmd.Args, " ")

prefixMatches := strings.HasPrefix(resultcmd, wantcmd)
if !prefixMatches {
t.Errorf("prefix `%s` not found in `%s`", wantcmd, resultcmd)
}

if tc.contains != "" && !strings.Contains(resultcmd, tc.contains) {
t.Errorf("string `%s` not found in `%s`", tc.contains, resultcmd)
}
})
}
}
14 changes: 0 additions & 14 deletions internal/cmd/test/cmd.go

This file was deleted.

29 changes: 29 additions & 0 deletions internal/cmd/test/find_packages.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package test

import (
"golang.org/x/tools/go/packages"
)

// findPackages in the current directory using the x/tools/go/packages API
func findPackages() ([]string, error) {
cfg := &packages.Config{
Mode: packages.NeedName,
Dir: ".",
}

pkgs, err := packages.Load(cfg, "./...")
if err != nil {
return []string{}, err
}

if packages.PrintErrors(pkgs) > 0 {
return []string{}, err
}

xx := []string{}
for _, pkg := range pkgs {
xx = append(xx, pkg.ID)
}

return xx, nil
}
13 changes: 13 additions & 0 deletions internal/cmd/test/longhelp.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
Runs the Buffalo tests. This command sets the GO_ENV to be `test`
and sets up the database in preparation for the tests.

--force-migrations forces the migrations to run even if the schema.sql is found
in the migrations folder

-testify.m Only run tests matching the regular expression
-m Only run tests matching the regular expression

The value passed to the -run flag is also passed to the -testify.m flag.
Any other flag is directly passed to the `go test` command.


77 changes: 77 additions & 0 deletions internal/cmd/test/setup_database.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package test

import (
"bytes"
"fmt"
"io"
"os"
"path/filepath"
"strings"

"github.com/gobuffalo/pop/v6"
)

func setupDatabase(args []string) error {
if _, err := os.Stat("database.yml"); err != nil {
return err
}

test, err := pop.Connect("test")
if err != nil {
return err
}

// drop the test db:
if err := test.Dialect.DropDB(); err != nil {
// not an error, since the database will be created in the next step anyway
fmt.Println("INFO: no test database to drop.")
}

// create the test db:
err = test.Dialect.CreateDB()
if err != nil {
return err
}

forceMigrations := strings.Contains(strings.Join(args, " "), "--force-migrations")
if forceMigrations {
fm, err := pop.NewFileMigrator("./migrations", test)
if err != nil {
return err
}

return fm.Up()
}

if schema := findSchema(); schema != nil {
return test.Dialect.LoadSchema(schema)
}

return nil
}

func findSchema() io.Reader {
if f, err := os.Open(filepath.Join("migrations", "schema.sql")); err == nil {
return f
}

if dev, err := pop.Connect("development"); err == nil {
schema := &bytes.Buffer{}
if err = dev.Dialect.DumpSchema(schema); err == nil {
return schema
}
}

if test, err := pop.Connect("test"); err == nil {
fm, err := pop.NewFileMigrator("./migrations", test)
if err != nil {
return nil
}

if err := fm.Up(); err == nil {
return nil
}
}

return nil
}
Loading