cmd/go: exclude standard library from "all" pattern in modules

myitcv commented 6 years ago

Please answer these questions before submitting your issue. Thanks!

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

go version go1.10.3 linux/amd64
vgo commit: cdd82a7bc7c6d7c407792afe279ed74331335370

Does this issue reproduce with the latest release?

Yes.

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ubuntu/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ubuntu/gostuff"
GORACE=""
GOROOT="/home/ubuntu/usr/go1.10.3"
GOTMPDIR=""
GOTOOLDIR="/home/ubuntu/usr/go1.10.3/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
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-build858106816=/tmp/go-build -gno-record-gcc-switches"

What did you do?

cd `mktemp -d`
mkdir hello
cd hello
vgo mod -init -module github.com/you/hello
cat <<EOD > hello.go
package main

import (
        "fmt"
)

func main() {
        fmt.Println("Hello")
}
EOD
vgo test all

What did you expect to see?

I'm not totally clear what to expect because I couldn't see exactly where the special all argument is documented? But my guess is that it shouldn't include standard library packages.

What did you see instead?

?       github.com/you/hello    0.005s [no test files]
ok      fmt     0.083s
ok      errors  0.001s
ok      io      0.021s
ok      math    0.003s
ok      os      18.463s
ok      reflect 0.757s
ok      strconv 2.009s
...

myitcv commented 6 years ago

/cc @rsc @bcmills

rsc commented 6 years ago

I guess probably "all" should exclude the standard library (in all commands not just go test), on the theory that it's already tested, not subject to vendoring, and so on. It's a little weird to special case it but it really is special.

bcmills commented 6 years ago

Should ... patterns also exclude the standard library in module mode? (modload.ImportPaths currently implements all in terms of ....)

bcmills commented 6 years ago

Actually, there is currently a special postprocessing phase for all, so it seems that if we make a minimal change then patterns involving ... will continue to match the standard library.

Still, it seems like an interesting edge-case to consider.

bcmills commented 6 years ago

I stand corrected again. Patterns involving ... already do not match the standard library in module mode.

gopherbot commented 6 years ago

Change https://golang.org/cl/125835 mentions this issue: cmd/go:/internal/modload: omit the standard library from "all"

bcmills commented 6 years ago

Per discussion on https://golang.org/cl/125835, I think this is working as intended: "all" should include the subset of the standard library that is actually (transitively) imported by the current module.

To test all packages in the current module and its submodules, you can execute go test ./... from the module root, or go test $(go list -m)/... from anywhere in the module.

It's true that we don't have a good pattern for “exactly the packages in the current module”, but all is not that pattern.

myitcv commented 6 years ago

Per discussion on https://golang.org/cl/125835, I think this is working as intended: "all" should include the subset of the standard library that is actually (transitively) imported by the current module.

Thanks. What I was after was a way of testing all packages in the current module and the transitive closure of package dependencies, excluding the standard library, i.e.

go test $(go list -f "{{if not .Standard}}{{.ImportPath}}{{end}}" $(go  list -f '{{ join .Deps  "\n"}}' $(go list -m)/...) $(go list -m)/...)

Edit: updated command to include $(go list -m)/... argument to go test

Reason being, it's probably safe to assume that the standard library packages I'm using (transitively) are already tested 😄. Build caching would obviate this concern to an extent, but I'd still have to run the tests for a standard library package once per release... but then that's already been done for me. Unless I'm missing something here about the benefit of (re)testing and hence including standard library packages in this definition?

To test all packages in the current module and its submodules, you can execute go test ./... from the module root, or go test $(go list -m)/... from anywhere in the module.

I don't think ./... includes submodules, does it? Since https://go-review.googlesource.com/c/vgo/+/122397, ./... has explicitly excluded submodules. For the reason that a subdirectory is not guaranteed to be a submodule.

However ./... being defined as such is somewhat unhelpful when it is the case that subdirectories are submodules (e.g. CI testing of a mono-repo). For now I've resorted to using find -name go.mod... because I realise this is probably something of an edge case, at least for Go 1.11.

It's true that we don't have a good pattern for “exactly the packages in the current module”, but all is not that pattern.

Given the above, I think m/... does actually cover this, because it doesn't recurse into submodules.

bcmills commented 6 years ago

We do have kind of a weird inconsistency here, though. all applies to all modules and includes the standard library, but ... applies to all modules but excludes the standard library. I don't see any way to list patterns within the standard library in module mode, but maybe that's just not a thing we need to support.

gopherbot commented 6 years ago

Change https://golang.org/cl/128637 mentions this issue: cmd/go: test package patterns with multiple modules

mwf commented 6 years ago

It's a pity we staying with the behaviour go test all testing std lib.

From https://research.swtch.com/vgo-tour:

Of course, after an upgrade, it's a good idea to test that everything still works. Our dependencies rsc.io/quote and rsc.io/sampler have not been tested with the newer text module. We can run their tests in the configuration we've created:

$ vgo test all

Guys, I believe we definitely need some short pattern to test the entire project with dependencies excluding std lib. Because as @myitcv mentioned, why should we ever test it if it's been already tested in Go release?

Patterns like go test $(go list -f "{{if not .Standard}}{{.ImportPath}}{{end}}" $(go list -f '{{ join .Deps "\n"}}' $(go list -m)/...)) look too clumsy, no one will be happy using such things in their project.

And the second reason for excluding std lib - some tests are very slow, I don't want to run them even once. Consider this small example (I still have go1.11beta2, sorry):

cd `mktemp -d`
go mod -init -module example.com/hello
cat <<EOD >main.go
package main

import (
        "fmt"
        "net/http"
        _ "net/http/pprof"
)

func main() {
        addr := "localhost:6060"
        fmt.Printf("Listening at: %q\n", addr)
        http.ListenAndServe(addr, nil)
}

EOD
go test all

?       example.com/hello   [no test files]
ok      fmt 0.142s
ok      net/http    46.180s
ok      net/http/pprof  2.200s
ok      errors  0.033s
ok      io  0.061s
ok      math    0.042s
ok      os  19.777s

46 sec? 20 sec? Guys, really? I see it and I just want to cancel :)

And signal.test runs almost forever eating all CPU :(

$ ps aux | grep signal
ikorolev         66777 185.8  0.0  4380320   1932 s009  R+    2:58PM  12:51.24 /var/folders/_b/d1934m9s587_8t_6ngv3hnc00000gp/T/go-build777311748/b810/signal.test -test.testlogfile=/var/folders/_b/d1934m9s587_8t_6ngv3hnc00000gp/T/go-build777311748/b810/testlog.txt

PS Just tried to run it on go1.11beta3, got an error:

$ go test all
build fmt_test: cannot find module for path fmt_test

Maybe it's connected with #26722 and fixed on tip. Will check

mwf commented 6 years ago

Yeap, working on tip.

Actually I can't understand why the tests are so slow. If the tests are run when building go from source they are much faster:

ok      net 1.985s
ok      net/http    4.355s
ok      net/http/cgi    0.684s
ok      net/http/cookiejar  0.028s
ok      net/http/fcgi   0.023s
ok      net/http/httptest   0.042s
ok      net/http/httptrace  0.019s
ok      net/http/httputil   0.061s
ok      net/http/internal   0.015s
ok      net/http/pprof  2.032s

$ time go test net/http
ok      net/http    40.735s
go test net/http  4.84s user 2.45s system 17% cpu 41.623 total

Why are the same tests so much faster when building go vs go test net/http?

Anyway with so slow stdlib tests go test all seems to be useless :(

mwf commented 6 years ago

This is the full timing of go test all for the same example project:

$ time go test all
...

*** Test killed: ran too long (10m0s).
FAIL    os/signal   605.012s
...
go test all  1521.37s user 69.95s system 228% cpu 11:35.33 total

Strange thing that testing os/signal fails on my machine. Though it passes when building go. And it passes in go 1.10.3 I'll file a separate issue for that.

Anyway, if we don't count the os/signal stuff, the longest tests taking >10s are:

ok      net/http    44.172s
ok      os  18.907s
ok      net 30.170s
ok      runtime 176.980s
ok      runtime/pprof   42.158s
ok      runtime/trace   10.776s
ok      regexp  19.281s
ok      compress/flate  11.576s
ok      crypto/dsa  29.287s

Does it worth testing them at all?

myitcv commented 6 years ago

@bcmills just to confirm my understanding (because this is the first time I'd seen the ... in isolation, i.e. with no prefix):

go list ./...   // all packages in the current directory and below,
                // excluding modules in subdirectories (which may be submodules)

go list m/...   // all packages in module m (by definition this does not
                // include submodules)

go list ...     // all packages in the current module and its module dependencies

go list all     // the transitive set of package imports, including std lib

go test all     // equivalent to go test $(go list all)

I don't see any way to list patterns within the standard library in module mode, but maybe that's just not a thing we need to support.

True, but we also don't have a way to easily list (and therefore test) the subset of go list all that are not standard library packages. Rather selfishly, this is what I need now; I'm relying on the standard library tests already having been run.

I don't actually find go list ... particularly useful. Let me demonstrate:

cd `mktemp -d`
export GOPATH=$(mktemp -d)
mkdir hello
cd hello
go mod init example.com/hello
cat <<EOD > main.go
package main

import _ "rsc.io/quote"

func main() {}
EOD
go list ...

gives:

Lots of output

``` example.com/hello golang.org/x/crypto/acme golang.org/x/crypto/acme/autocert golang.org/x/crypto/acme/autocert/internal/acmetest golang.org/x/crypto/argon2 golang.org/x/crypto/bcrypt golang.org/x/crypto/blake2b golang.org/x/crypto/blake2s golang.org/x/crypto/blowfish golang.org/x/crypto/bn256 golang.org/x/crypto/cast5 golang.org/x/crypto/chacha20poly1305 golang.org/x/crypto/cryptobyte golang.org/x/crypto/cryptobyte/asn1 golang.org/x/crypto/curve25519 golang.org/x/crypto/ed25519 golang.org/x/crypto/ed25519/internal/edwards25519 golang.org/x/crypto/hkdf golang.org/x/crypto/internal/chacha20 golang.org/x/crypto/internal/subtle golang.org/x/crypto/md4 golang.org/x/crypto/nacl/auth golang.org/x/crypto/nacl/box golang.org/x/crypto/nacl/secretbox golang.org/x/crypto/nacl/sign golang.org/x/crypto/ocsp golang.org/x/crypto/openpgp golang.org/x/crypto/openpgp/armor golang.org/x/crypto/openpgp/clearsign golang.org/x/crypto/openpgp/elgamal golang.org/x/crypto/openpgp/errors golang.org/x/crypto/openpgp/packet golang.org/x/crypto/openpgp/s2k golang.org/x/crypto/otr golang.org/x/crypto/pbkdf2 golang.org/x/crypto/pkcs12 golang.org/x/crypto/pkcs12/internal/rc2 golang.org/x/crypto/poly1305 golang.org/x/crypto/ripemd160 golang.org/x/crypto/salsa20 golang.org/x/crypto/salsa20/salsa golang.org/x/crypto/scrypt golang.org/x/crypto/sha3 golang.org/x/crypto/ssh golang.org/x/crypto/ssh/agent golang.org/x/crypto/ssh/knownhosts golang.org/x/crypto/ssh/terminal golang.org/x/crypto/ssh/test golang.org/x/crypto/tea golang.org/x/crypto/twofish golang.org/x/crypto/xtea golang.org/x/crypto/xts golang.org/x/net/bpf golang.org/x/net/context golang.org/x/net/context/ctxhttp golang.org/x/net/dict golang.org/x/net/dns/dnsmessage golang.org/x/net/html golang.org/x/net/html/atom golang.org/x/net/html/charset golang.org/x/net/http/httpguts golang.org/x/net/http/httpproxy golang.org/x/net/http2 golang.org/x/net/http2/h2c golang.org/x/net/http2/h2i golang.org/x/net/http2/hpack golang.org/x/net/icmp golang.org/x/net/idna golang.org/x/net/internal/iana golang.org/x/net/internal/nettest golang.org/x/net/internal/socket golang.org/x/net/internal/socks golang.org/x/net/internal/sockstest golang.org/x/net/internal/timeseries golang.org/x/net/ipv4 golang.org/x/net/ipv6 golang.org/x/net/nettest golang.org/x/net/netutil golang.org/x/net/proxy golang.org/x/net/publicsuffix golang.org/x/net/trace golang.org/x/net/webdav golang.org/x/net/webdav/internal/xml golang.org/x/net/websocket golang.org/x/net/xsrftoken golang.org/x/sys/cpu golang.org/x/sys/unix golang.org/x/sys/windows golang.org/x/sys/windows/registry golang.org/x/text golang.org/x/text/cases golang.org/x/text/cmd/gotext golang.org/x/text/collate golang.org/x/text/collate/build golang.org/x/text/collate/tools/colcmp golang.org/x/text/currency golang.org/x/text/encoding golang.org/x/text/encoding/charmap golang.org/x/text/encoding/htmlindex golang.org/x/text/encoding/ianaindex golang.org/x/text/encoding/internal golang.org/x/text/encoding/internal/enctest golang.org/x/text/encoding/internal/identifier golang.org/x/text/encoding/japanese golang.org/x/text/encoding/korean golang.org/x/text/encoding/simplifiedchinese golang.org/x/text/encoding/traditionalchinese golang.org/x/text/encoding/unicode golang.org/x/text/encoding/unicode/utf32 golang.org/x/text/feature/plural golang.org/x/text/internal golang.org/x/text/internal/catmsg golang.org/x/text/internal/colltab golang.org/x/text/internal/export/idna golang.org/x/text/internal/format golang.org/x/text/internal/gen golang.org/x/text/internal/number golang.org/x/text/internal/stringset golang.org/x/text/internal/tag golang.org/x/text/internal/testtext golang.org/x/text/internal/triegen golang.org/x/text/internal/ucd golang.org/x/text/internal/utf8internal golang.org/x/text/language golang.org/x/text/language/display golang.org/x/text/message golang.org/x/text/message/catalog golang.org/x/text/number golang.org/x/text/runes golang.org/x/text/search golang.org/x/text/secure golang.org/x/text/secure/bidirule golang.org/x/text/secure/precis golang.org/x/text/transform golang.org/x/text/unicode golang.org/x/text/unicode/bidi golang.org/x/text/unicode/cldr golang.org/x/text/unicode/norm golang.org/x/text/unicode/rangetable golang.org/x/text/unicode/runenames golang.org/x/text/width golang.org/x/tools/benchmark/parse golang.org/x/tools/blog golang.org/x/tools/blog/atom golang.org/x/tools/cmd/benchcmp golang.org/x/tools/cmd/bundle golang.org/x/tools/cmd/callgraph golang.org/x/tools/cmd/compilebench golang.org/x/tools/cmd/cover golang.org/x/tools/cmd/digraph golang.org/x/tools/cmd/eg golang.org/x/tools/cmd/fiximports golang.org/x/tools/cmd/getgo golang.org/x/tools/cmd/getgo/server golang.org/x/tools/cmd/go-contrib-init golang.org/x/tools/cmd/godex golang.org/x/tools/cmd/godoc golang.org/x/tools/cmd/goimports golang.org/x/tools/cmd/gomvpkg golang.org/x/tools/cmd/gorename golang.org/x/tools/cmd/gotype golang.org/x/tools/cmd/goyacc golang.org/x/tools/cmd/guru golang.org/x/tools/cmd/guru/serial golang.org/x/tools/cmd/heapview golang.org/x/tools/cmd/heapview/internal/core golang.org/x/tools/cmd/html2article golang.org/x/tools/cmd/present golang.org/x/tools/cmd/ssadump golang.org/x/tools/cmd/stress golang.org/x/tools/cmd/stringer golang.org/x/tools/cmd/tip golang.org/x/tools/cmd/toolstash golang.org/x/tools/container/intsets golang.org/x/tools/cover golang.org/x/tools/go/ast/astutil golang.org/x/tools/go/buildutil golang.org/x/tools/go/callgraph golang.org/x/tools/go/callgraph/cha golang.org/x/tools/go/callgraph/rta golang.org/x/tools/go/callgraph/static golang.org/x/tools/go/gccgoexportdata golang.org/x/tools/go/gcexportdata golang.org/x/tools/go/internal/cgo golang.org/x/tools/go/internal/gccgoimporter golang.org/x/tools/go/internal/gcimporter golang.org/x/tools/go/loader golang.org/x/tools/go/packages golang.org/x/tools/go/packages/golist golang.org/x/tools/go/packages/gopackages golang.org/x/tools/go/packages/raw golang.org/x/tools/go/pointer golang.org/x/tools/go/ssa golang.org/x/tools/go/ssa/interp golang.org/x/tools/go/ssa/ssautil golang.org/x/tools/go/types/typeutil golang.org/x/tools/go/vcs golang.org/x/tools/godoc golang.org/x/tools/godoc/analysis golang.org/x/tools/godoc/redirect golang.org/x/tools/godoc/static golang.org/x/tools/godoc/util golang.org/x/tools/godoc/vfs golang.org/x/tools/godoc/vfs/gatefs golang.org/x/tools/godoc/vfs/httpfs golang.org/x/tools/godoc/vfs/mapfs golang.org/x/tools/godoc/vfs/zipfs golang.org/x/tools/imports golang.org/x/tools/internal/fastwalk golang.org/x/tools/playground golang.org/x/tools/playground/socket golang.org/x/tools/present golang.org/x/tools/refactor/eg golang.org/x/tools/refactor/importgraph golang.org/x/tools/refactor/rename golang.org/x/tools/refactor/satisfy rsc.io/quote rsc.io/quote/buggy rsc.io/sampler ```

Whereas what I'm actually interested in is (ignoring build constraints for now):

$ go list -f "{{if not .Standard}}{{.ImportPath}}{{end}}" $(go  list -f '{{ join .Deps  "\n"}}' $(go list -m)/...) $(go list -m)/...
golang.org/x/text/internal/tag
golang.org/x/text/language
rsc.io/quote
rsc.io/sampler
example.com/hello

bcmills commented 6 years ago

The more I think about it, the more I think the current patterns are correct. Here's my reasoning:

all is the pattern you want for guru and other analysis tools. They need to scan and index the relevant parts of the standard library in order to make behaviors like “jump to definition” work uniformly.
... is the pattern you want for pre-release testing. Your dependencies likely have a few integration tests, and for large integration tests, the packages that provide those integration tests might be distinct from the packages being tested. And integration tests are exactly the ones that are important to run when you're testing new combinations of dependency versions.
./... is the pattern you want for day-to-day feature testing. You know your dependencies are fine, and you just want to re-test the module you're working on.

So I think this is mostly just a documentation issue: we should be recommending go test ./... or go test ..., not go test all.

bcmills commented 6 years ago

@myitcv

go list ./...   // all packages in the current directory and below,
                // excluding modules in subdirectories (which may be submodules)

Not just subdirectories, but excluding submodules in general (even if they aren't stored in subdirectories).

go list m/...   // all packages in module m (by definition this does not
                // include submodules)

No, this one is “all packages in all modules with prefix m/”

go list ...     // all packages in the current module and its module dependencies

go list all     // the transitive set of package imports, including std lib

Yep.

go test all     // equivalent to go test $(go list all)

Correct: go test <pattern> should always be equivalent to go test $(go list <pattern>).

myitcv commented 6 years ago

@bcmills

... is the pattern you want for pre-release testing.

Is this really what you want for pre-release testing though? Because given my example in https://github.com/golang/go/issues/26317#issuecomment-411754782 this would involve me testing golang.org/x/tools/playground which (amongst other packages in that list) is totally irrelevant unless I'm missing something obvious here?

bcmills commented 6 years ago

Given that we don't have any other means to specify “package example.com/x/y is an integration test for package example.com/z/w”, I think so, yes. (Figuring out some sort of specification for that is a whole other can of worms.)

If caching is working properly, the overhead of “running” those tests should generally be fairly minimal — especially if we split the x/ repos into smaller modules and/or start giving more of them semantic version tags.

myitcv commented 6 years ago

@bcmills

Given that we don't have any other means to specify “package example.com/x/y is an integration test for package example.com/z/w”, I think so, yes.

I see what I was missing, thanks.

(Figuring out some sort of specification for that is a whole other can of worms.)

True, but an interesting/worthwhile idea though.

split the x/ repos into smaller modules and/or start giving more of them semantic version tags.

Indeed.

In addition to this point, the bit I clearly forgot in all of this is the first line of go help modules:

A module is a collection of related Go packages.

which helps to make more sense of the definition for ....

Thanks for taking the time to talk this through.

Do you want to leave this issue open in order to address:

So I think this is mostly just a documentation issue: we should be recommending go test ./... or go test ..., not go test all.

??

Will leave up to you.

rsc commented 6 years ago

The standard library is incorrectly excluded from ... in module mode. It should be included.

We should be encouraging people to use all to test the current module and its dependencies. Testing ... instead will test lots of completely irrelevant packages. One of the main reasons for redefining all was precisely to make it useful for go test [-short] all.

mwf commented 6 years ago

We should be encouraging people to use all to test the current module and its dependencies.

@rsc do you mean we should exclude standard library from all then? Otherwise it could be painful to use even with -short mode on.

rsc commented 6 years ago

all means "all your dependencies - the things that actually get built into the packages in your modules". I don't see a compelling reason to exclude fmt if you use fmt but not exclude something else 10 levels down. Especially if you use -short, it should not be painful. And it should cache very well.

mwf commented 6 years ago

Thanks for the clarification!

OK, let's try to live with it. Only time could judge wether the community accepts this behaviour or not 😄

Maybe it's reasonable for a case, when there is really something wrong in your OS, making standard library tests to fail :) And it could help us to debug some crazy cases in future go releases.

rsc commented 6 years ago

I think this is now working as documented, so closing.

anacrolix commented 5 years ago

I'm on a very recent Macbook (Pro, 2018, 6 cores): Running go test std with GO111MODULE=on takes (even with everything cached):

real    1m43.740s
user    3m43.726s
sys 0m48.724s

because of

--- FAIL: TestCrashDumpsAllThreads (1.75s)
    crash_unix_test.go:70: building source: exit status 1
        go: cannot find main module; see 'go help modules'
FAIL
FAIL    runtime 98.016s

Without the tests cached:

real    10m23.312s
user    27m35.928s
sys 1m34.860s

I think there's a bad assumption here that the std tests "should" always succeed (and that we can handwave them away with caching), because they do not, and from cold-start they are very time-consuming.

I'd like to do go test all as a quick check that everything I'm depending on works together, it's not convenient or a quick part of my workflow if it can blow out like this so easily.

bcmills commented 5 years ago

I think there's a bad assumption here that the std tests "should" always succeed (and that we can handwave them away with caching), because they do not

If you find that a test in std is consistently failing, please file an issue with steps to reproduce the failure.

golang / go