cmd/go: document that directory patterns only match dirs in current module

[hyangah]

This involves both documentation and the implementation (which I don't know it's intended or something to fix). Feel free to split this issue if necessary.

All experiments here are done outside GOPATH.
I have a two module testmod2 and testmod2/cmd in one single repo.
The latest testmod2/cmd (v0.0.2) depends on testmod2/v2.
There are tests in testmod2/lib and testmod2/cmd packages.

$ git clone https://github.com/hyangah/testmod2.git /tmp/testmod2
$ cd /tmp/testmod2

$ go test ./...
ok  	github.com/hyangah/testmod2/v2/lib	0.021s

$ go test ./cmd/...
go: finding github.com/hyangah/testmod2/v2/cmd latest
can't load package: package github.com/hyangah/testmod2/v2/cmd: unknown import path "github.com/hyangah/testmod2/v2/cmd": cannot find module providing package github.com/hyangah/testmod2/v2/cmd

• go test ./... doesn't cross the module boundary, so tests in testmod2/cmd didn' run. go test ./cmd/... doesn't work either because of the same reason. If we ever allow submodules in a single repo, this needs to be documented in https://tip.golang.org/cmd/go/#hdr-Package_lists_and_patterns.

• AFAIK, there is no automatic way to invoke all tests in the repo when submodules are involved. That's intended because module is like a separate workspace (as discussed in cmd/go: setting up a multi-module single repo is difficult #27056).

• As specified in the [ Package lists and patterns] section, all can be used to test all modules the current module depends on. That implies testing in the standard libraries as well.

$ cd cmd
$ go test all
$ go test all
ok  	bufio	0.141s
ok  	bytes	1.862s
ok  	compress/bzip2	0.147s
--- FAIL: TestBlockHuff (0.00s)
    huffman_bit_writer_test.go:58: Testing "testdata/huffman-null-max.in"
    huffman_bit_writer_test.go:78: Output ok
    huffman_bit_writer_test.go:93: Reset ok
    huffman_bit_writer_test.go:365: EOF ok
    huffman_bit_writer_test.go:58: Testing "testdata/huffman-pi.in"
    huffman_bit_writer_test.go:78: Output ok
    huffman_bit_writer_test.go:93: Reset ok
    huffman_bit_writer_test.go:365: EOF ok
    huffman_bit_writer_test.go:58: Testing "testdata/huffman-rand-1k.in"
    huffman_bit_writer_test.go:78: Output ok
    huffman_bit_writer_test.go:93: Reset ok
    huffman_bit_writer_test.go:365: EOF ok
    huffman_bit_writer_test.go:58: Testing "testdata/huffman-rand-limit.in"
    huffman_bit_writer_test.go:78: Output ok
    huffman_bit_writer_test.go:93: Reset ok
    huffman_bit_writer_test.go:365: EOF ok
    huffman_bit_writer_test.go:58: Testing "testdata/huffman-rand-max.in"
    huffman_bit_writer_test.go:78: Output ok
    huffman_bit_writer_test.go:93: Reset ok
    huffman_bit_writer_test.go:365: EOF ok
    huffman_bit_writer_test.go:58: Testing "testdata/huffman-shifts.in"
    huffman_bit_writer_test.go:72: "testdata/huffman-shifts.in" != "testdata/huffman-shifts.golden" (see "testdata/huffman-shifts.in.got")
    huffman_bit_writer_test.go:74: open testdata/huffman-shifts.in.got: permission denied
    huffman_bit_writer_test.go:58: Testing "testdata/huffman-text-shift.in"
    huffman_bit_writer_test.go:72: "testdata/huffman-text-shift.in" != "testdata/huffman-text-shift.golden" (see "testdata/huffman-text-shift.in.got")
    huffman_bit_writer_test.go:74: open testdata/huffman-text-shift.in.got: permission denied
    huffman_bit_writer_test.go:58: Testing "testdata/huffman-text.in"
    huffman_bit_writer_test.go:72: "testdata/huffman-text.in" != "testdata/huffman-text.golden" (see "testdata/huffman-text.in.got")
    huffman_bit_writer_test.go:74: open testdata/huffman-text.in.got: permission denied
    huffman_bit_writer_test.go:58: Testing "testdata/huffman-zero.in"
    huffman_bit_writer_test.go:78: Output ok
    huffman_bit_writer_test.go:93: Reset ok
    huffman_bit_writer_test.go:365: EOF ok
FAIL
FAIL	compress/flate	9.319s
ok  	compress/gzip	0.057s
ok  	compress/lzw	0.128s
...

This unfortunate case I encountered is the failures from the tests in the standard libraries. The above error was because I have no permission to write to the GOROOT. I think that's not a rare setup so running standard library tests doesn't seem to be a right choice.

[hyangah]

Previous bugs related to 'go test' and module
#27547: about documentation on 'go test all'
#26317: about standard library testing and also discusses the meaning of ./..., ..., all, ... in the world of module.

[thepudds]

A related comment is that the behavior of go test ./... in a modules-aware mode is not 100% clear from the documentation.

These two snippets from the documentation for go list -m https://tip.golang.org/cmd/go/#hdr-List_packages_or_modules:

The main module is the module containing the current directory. The active modules are the main module and its dependencies.
...
A pattern containing "..." specifies the active modules whose module paths match the pattern.

seems to imply that ./... would descend into another module in a subdirectory IF that module is a dependency of the current module (that is, if a module in a subdirectory is part of the "active modules"). However, it is not clear if that is specific to go list -m, or if it would also apply to go test ./.... (Or maybe I am just not parsing the documentation correctly).

(edit: as far as I can tell and as I tried to outline below in #27957 (comment), it seems there are a slightly different set of rules applied to module patterns vs. module-enabled package patterns, and ... behavior is documented for module patterns (in doc snippet above) but ... behavior does not seem to be documented for module-enabled package patterns. The above doc snippet on module patterns does not seem to not be 100% applicable to a ./... package pattern, including because ./... is rejected as an error if used as a module pattern. The net of all of this is go test ./... does not seem to cross into a submodule dependency of the current module, which was the concern raised in this comment here).

[myitcv]

Per our Slack conversation, that doesn't appear to be the case:

$ cd $HOME
$ mkdir hello
$ cd hello
$ go mod init example.com/hello
go: creating new go.mod: module example.com/hello
$ cat <<EOD >hello.go
package main

import (
        "example.com/hello/goodbye"
        "fmt"
)

func main() {
        fmt.Println(goodbye.Name)
}
EOD
$ mkdir goodbye
$ cd goodbye
$ go mod init example.com/hello/goodbye
go: creating new go.mod: module example.com/hello/goodbye
$ cat <<EOD >goodbye.go
package goodbye

const Name = "Hello"
EOD
$ cd ..
$ go mod edit -require=example.com/hello/[email protected] -replace=example.com/hello/goodbye=./goodbye
$ go run .
Hello
$ go list ./...
example.com/hello
$ go list all | grep example
example.com/hello
example.com/hello/goodbye

I'm not entirely sure whether that's a bug given the documentation you've highlighted @thepudds

[thepudds]

The clearest description I think I've seen of the meaning of common package patterns in a module world is from the mod_patterns.txt test. (I believe these are all package patterns, but with modules enabled):

https://github.com/golang/go/blob/release-branch.go1.11/src/cmd/go/testdata/script/mod_patterns.txt

env GO111MODULE=on

...

# 'go list all' should list all of the packages used (directly or indirectly) by
# the packages in the main module, but no other packages from the standard
# library or active modules.
#
# 'go list ...' should list packages in all active modules and the standard library.
# But not cmd/* - see golang.org/issue/26924.
#
# 'go list example.com/m/...' should list packages in all modules that begin with 'example.com/m/'.
#
# 'go list ./...' should list only packages in the current module, not other active modules.

And there are a slightly different set of rules applied if a module pattern is being supplied via a -m flag (rather than the examples above, which are package patterns).

One difference is go list -m seems to reject ./... (whereas ./... is a valid package pattern when modules are enabled):

$ go list -m ./...
go: cannot use relative path ./... to specify module

I'm not sure that is documented. The behavior of ... and all for a -m module pattern does seem to be documented. for example, in the go list documentation https://tip.golang.org/cmd/go/#hdr-List_packages_or_modules:

The main module is the module containing the current directory. The active modules are the main module and its dependencies.
...
The special pattern "all" specifies all the active modules, first the main module and then dependencies sorted by module path.
...
A pattern containing "..." specifies the active modules whose module paths match the pattern.

go list, go mod why, and go get all seem to support the -m flag. I would guess the -m pattern interpretation is probably consistent across those (but I don't know that to be a fact).

So summarizing in terms of what might be missing from the current documentation, it seems:

• I don't see the meaning of '...' (and variations like './...') being explicitly defined for a package pattern with modules enabled. A natural spot might be https://tip.golang.org/cmd/go/#hdr-Package_lists_and_patterns (which defines module-specific interpretation of all, but not ... and not ./...)
• I don't see ./... being documented as disallowed for -m module pattern.
• The behavior of -m patterns seems to be mainly documented in go list documentation (and it only seems to be implicit that you (probably?) get the same behavior for other -m pattern usages).

All that said, the documentation comments here are based on some quick Ctrl-F and basic grepping, so easily could have missed something that is actually there in the doc, and of course other people might have other behavior in mind that is not documented either.

[rsc]

OK, so trying to summarize this discussion, it sounds like there are three things we should do to mark this bug closed:

1. Document in the help text for package patterns that file system patterns can only ever match packages in the current module.

2. Document that tests should not assume they can write to the current directory.

3. Fix the standard library tests not to write to the current directory.

[bcmills]

Let's make this issue about documenting package patterns in module mode.

I've filed #28386 for documenting writability, and #28387 for fixing the standard library.

[hyangah]

Ensuring tests do not write to the current directory sounds ok. But, what's the point of running the tests again for the standard packages? On my workstation with google internal go distribution, I get various errors from standard package tests, not necessarily related to the permission issues. Here are some of the errors not involving permission issues.

mime/multipart: (I don't know why)
   --- FAIL: TestNested (0.00s)
      multipart_test.go:466: reading text/plain part: got "*body*\n", 
net: bad test? flakiness?
  2018/10/25 14:21:50 http: TLS handshake error from 127.0.0.1:57808: read tcp 127.0.0.1:34383- 
  >127.0.0.1:57808: use of closed network connection
  --- FAIL: TestServerValidatesHostHeader (0.00s)
     serve_test.go:4667: For HTTP/1.1 "", Status = 200; want 400

runtime: bad test or flakiness (TestSchedStat/base)

time: panic: cannot load America/Los_Angeles for testing: unknown time zone America/Los_Angeles

[bcmills]

If the standard-library tests are flaky, we should either fix them or Skip them. To the extent that re-running them in module mode helps to uncover flakiness, I think that's a good thing. 🙂

Beyond that, at some point we might want to allow golang.org/x modules to override the ones vendored in the standard library, and at that point it really will be useful to re-run the standard tests to make sure those modules are still compatible.

And if something about a particular platform or machine actually breaks non-flaky tests in the standard library, that's a useful signal too: it tells users that the error may be in their system configuration (or the standard library) rather than their application code. (Remember that these test results will be cached, so they should only re-run when the cache is cold.)

[hyangah]

That may be a good feature for Go maintainers. Ideally those flakiness should be caught before official Go release but in practice this thing happens.

For users of already released Go tools and libraries, it means they should live with the noise until the new release with the fix is available, which makes use of go test all command unpleasant. On the other hand, I don't know if I ever want to run go test all. (I ended up running the command because I wanted to find a way to run all tests in a repo that hosts multiple modules)

[myitcv]

Assuming this is all behaving as expected, I've just realised my error in #27957 (comment).

• ./... is a directory-oriented pattern that does not cross module boundaries
• $somepath/... is an import path-oriented pattern that does potentially cross module boundaries

$ cd /home/gopher
$ mkdir hello
$ cd hello
$ go mod init example.com/hello
go: creating new go.mod: module example.com/hello
$ cat <<EOD >hello.go
package main

import (
        "example.com/hello/goodbye"
        "fmt"
)

func main() {
        fmt.Println(goodbye.Name)
}
EOD
$ mkdir goodbye
$ cd goodbye
$ go mod init example.com/hello/goodbye
go: creating new go.mod: module example.com/hello/goodbye
$ cat <<EOD >goodbye.go
package goodbye

const Name = "Hello"
EOD
$ cd ..
$ go mod edit -require=example.com/hello/[email protected] -replace=example.com/hello/goodbye=./goodbye
$ go run .
Hello
$ go list ./...
example.com/hello
$ go list $(go list -m)/...
example.com/hello
example.com/hello/goodbye
$ go list all | grep example
example.com/hello
example.com/hello/goodbye

@bcmills I suspect this is what you were referring to in #27957 (comment) about clarifying that distinction in the docs.

[ronaldpetty]

go help test no longer has all documented, yet runs as argument.

% go help test | grep all
The first, called local directory mode, occurs when go test is
The second, called package list mode, occurs when go test is invoked
tests for all of the listed packages finish, and their output is
-parallel, -run, -short, and -v. If a run of go test has any test
root (usually $GOPATH) or that consult environment variables only
A cached test result is treated as executing in no time at all,
	    Install packages that are dependencies of the test.
%

[cyrusv]

@myitcv or other,
I've recently hit a similar usability issue with go test and submodules/multi-modules.

I understand that one view is that different modules are a different workspace. However, this intuition doesn't exactly hold when we are talking about v2s: https://go.dev/blog/v2-go-modules
The example for the V2 docs references gax-go, where they create a submodule.

Now, imagine your CI platform is testing the code -- the recommendation is to use go test ./... to test everything. However, it will not test the V2 module at all, giving a false sense of correctness!

It would greatly improve usability to have a go test variation that will test everything, including submodules.

I propose:

• A new argument for go test: ./.../... which will behave like ./... but further traverse/test submodules. This way, existing integrations will be unaffected, and we also have a tool to more safely test code from top level as submodules become recommended project structure as v2 documentation teaches.

• If this is a no-go, then I would otherwise propose to update documentation to go help test to emphasize that ./... does not traverse submodules, and to suggest the recommended method (though, i'm not sure what that is).

I personally would be happy to open either of these PRs, if there was some guidance from the committers that there's some support. I created a proposal issue.