Skip to content
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

cmd/go: "go test -notrealflag ./..." fails silently if the current directory has no tests #35097

Closed
MStoykov opened this issue Oct 23, 2019 · 6 comments
Labels

Comments

@MStoykov
Copy link

@MStoykov MStoykov commented Oct 23, 2019

What version of Go are you using (go version)?

$ go version
go version go1.13.3 linux/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/mstoykov/.cache/go-build"
GOENV="/home/mstoykov/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/mstoykov/.gvm/pkgsets/go1.13.3/global"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/mstoykov/.gvm/gos/go1.13.3"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/mstoykov/.gvm/gos/go1.13.3/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build162107444=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Running go test -notrealflag ./... in a directory with no tests doesn't not report that there is no such flag.
In the real case I hit I was mistakenly providing -fastfail instead of -failfast and this did not run any tests and gave me no reason why it didn't.

There is an error if the current directory has tests.

What did you expect to see?

An error and the help of go test

What did you see instead?

$ go test -notrealflag ./...
?       github.com/loadimpact/k6        [no test files]
$ echo $?
0
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 23, 2019

Per https://golang.org/cmd/go/#hdr-Testing_flags:

The 'go test' command takes both flags that apply to 'go test' itself and flags that apply to the resulting test binary.

Since the pattern did not match any test binaries, no such binaries were run. However, you could just as easily run go test -notrealflag on a package that actually does define -notrealflag, and that would not be an error.

Since go test did explicitly tell you that there were [no test files] in this case, I don't think there is anything more to be done here: it would be inconsistent (and oddly specific) to reject additional flags only when go test has no other actual work to do.

@bcmills bcmills closed this Oct 23, 2019
@MStoykov

This comment has been minimized.

Copy link
Author

@MStoykov MStoykov commented Oct 23, 2019

@bcmills if you run it without the flag there are a tons of sub packages that will be tested. And if that same command is ran inside a directory which there are tests it will actually correctly report that this flag is not there

$ go test -count 0 ./...
?       github.com/loadimpact/k6        [no test files]
ok      github.com/loadimpact/k6/api    0.038s [no tests to run]
?       github.com/loadimpact/k6/api/common     [no test files]
ok      github.com/loadimpact/k6/api/v1 0.029s [no tests to run]
?       github.com/loadimpact/k6/api/v1/client  [no test files]
ok      github.com/loadimpact/k6/api/v1 0.029s [no tests to run]                                                                                                                                           [47/2129]
?       github.com/loadimpact/k6/api/v1/client  [no test files]
ok      github.com/loadimpact/k6/cmd    0.033s [no tests to run]
ok      github.com/loadimpact/k6/converter/har  0.020s [no tests to run]
ok      github.com/loadimpact/k6/core   0.013s [no tests to run]
ok      github.com/loadimpact/k6/core/local     0.015s [no tests to run]
ok      github.com/loadimpact/k6/js     0.013s [no tests to run]
ok      github.com/loadimpact/k6/js/common      0.006s [no tests to run]
ok      github.com/loadimpact/k6/js/compiler    0.012s [no tests to run]
?       github.com/loadimpact/k6/js/lib [no test files]
?       github.com/loadimpact/k6/js/modules     [no test files]
ok      github.com/loadimpact/k6/js/modules/k6  0.032s [no tests to run]
ok      github.com/loadimpact/k6/js/modules/k6/crypto   0.010s [no tests to run]
ok      github.com/loadimpact/k6/js/modules/k6/crypto/x509      0.016s [no tests to run]
ok      github.com/loadimpact/k6/js/modules/k6/encoding 0.020s [no tests to run]
ok      github.com/loadimpact/k6/js/modules/k6/html     0.041s [no tests to run]
?       github.com/loadimpact/k6/js/modules/k6/html/gen [no test files]
ok      github.com/loadimpact/k6/js/modules/k6/http     0.009s [no tests to run]
ok      github.com/loadimpact/k6/js/modules/k6/metrics  0.034s [no tests to run]
ok      github.com/loadimpact/k6/js/modules/k6/ws       0.018s [no tests to run]
ok      github.com/loadimpact/k6/lib    0.023s [no tests to run]
?       github.com/loadimpact/k6/lib/consts     [no test files]
ok      github.com/loadimpact/k6/lib/fsext      0.009s [no tests to run]
?       github.com/loadimpact/k6/lib/metrics    [no test files]
?       github.com/loadimpact/k6/lib/netext     [no test files]
ok      github.com/loadimpact/k6/lib/netext/httpext     0.005s [no tests to run]
ok      github.com/loadimpact/k6/lib/scheduler  0.021s [no tests to run]
?       github.com/loadimpact/k6/lib/testutils  [no test files]
?       github.com/loadimpact/k6/lib/testutils/httpmultibin     [no test files]
ok      github.com/loadimpact/k6/lib/types      0.003s [no tests to run]
ok      github.com/loadimpact/k6/loader 0.005s [no tests to run]
ok      github.com/loadimpact/k6/stats  0.015s [no tests to run]
ok      github.com/loadimpact/k6/stats/cloud    0.005s [no tests to run]
ok      github.com/loadimpact/k6/stats/csv      0.032s [no tests to run]
ok      github.com/loadimpact/k6/stats/datadog  0.037s [no tests to run]
ok      github.com/loadimpact/k6/stats/dummy    0.005s [no tests to run]
ok      github.com/loadimpact/k6/stats/influxdb 0.031s [no tests to run]
ok      github.com/loadimpact/k6/stats/json     0.027s [no tests to run]
ok      github.com/loadimpact/k6/stats/kafka    0.048s [no tests to run]
ok      github.com/loadimpact/k6/stats/statsd   0.005s [no tests to run]
ok      github.com/loadimpact/k6/stats/statsd/common    0.005s [no tests to run]
?       github.com/loadimpact/k6/stats/statsd/common/testutil   [no test files]
ok      github.com/loadimpact/k6/ui     0.004s [no tests to run]
[mstoykov@himinbjorg k6]$ cd js
[mstoykov@himinbjorg js]$ go test -notrealflag ./...
flag provided but not defined: -notrealflag
Usage of /tmp/go-build525921648/b001/js.test:
  -test.bench regexp
        run only benchmarks matching regexp
... more test flags
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 23, 2019

The flag provided but not defined error that you see there comes from the binary under test, not the go test command itself.

Compare:

~/go/src/testing$ go test -c testing

~/go/src/testing$ ./testing.test -notrealflag
flag provided but not defined: -notrealflag
Usage of ./testing.test:
  -test.bench regexp
        run only benchmarks matching regexp
[…]
@MStoykov

This comment has been minimized.

Copy link
Author

@MStoykov MStoykov commented Oct 24, 2019

The problem is not where it comes from, the problem is that in the one case the tests are not ran for any of the other directories which leads to the user having no idea why test haven't ran ...

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jan 22, 2020

#36527 reported a similar issue, although with an actual test file in the top-level directory (so there actually was a test to pass the flag to), and that test file actually did define the flag in question.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jan 22, 2020

Thinking about this some more, I still don't think there's anything we can do about it without breaking existing use-cases. Any special case to reject unknown flags if there is no work at all would still fail for cases like #36527 where there actually is work to do.

It seems like the only robust way to ensure that the user's intended package argument is actually parsed as such is to require a package argument in all cases, but lots of users today have already learned to run commands like go test -v with the implicit . package argument. We shouldn't break the workflows of those users without extremely compelling evidence, and so far we have only two reports of issues.

CC @jayconrod @matloob

@bcmills bcmills closed this Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.