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

Jump to bottom

x/tools/go/packages: returns package with no sources in a directory that only contains a test #36292

Closed
stamblerre opened this issue Dec 27, 2019 · 6 comments
Closed

x/tools/go/packages: returns package with no sources in a directory that only contains a test #36292

stamblerre opened this issue Dec 27, 2019 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Backlog

Comments

@stamblerre
Copy link
Contributor

stamblerre commented Dec 27, 2019 •
edited

If you have a directory that only contains a test variant of a package, go list still returns a package for the non-test variant. This ends up being a bit misleading, since the package actually has no sources. We can, of course, ignore packages that have no sources in gopls, but I was still surprised to see this behavior.

To reproduce:

$ cd $(mktemp -d)
$ cat >>foo_test.go<<EOF
package foo

import "testing"

func TestFoo(t *testing.T) {}
EOF
$ cat >>go.mod<<EOF
module foo
EOF
$ go list -e -json -test
{
	"Dir": "/tmp/tmp.JncAr4CsDl",
	"ImportPath": "foo",
	"Name": "foo",
	"Root": "/tmp/tmp.JncAr4CsDl",
	"Module": {
		"Path": "foo",
		"Main": true,
		"Dir": "/tmp/tmp.JncAr4CsDl",
		"GoMod": "/tmp/tmp.JncAr4CsDl/go.mod",
		"GoVersion": "1.13"
	},
	"Match": [
		"."
	],
	"TestGoFiles": [
		"foo_test.go"
	],
	"TestImports": [
		"testing"
	]
}
{
	"Dir": "/tmp/tmp.JncAr4CsDl",
	"ImportPath": "foo.test",
	"Name": "main",
	"Root": "/tmp/tmp.JncAr4CsDl",
	"Stale": true,
	"StaleReason": "stale dependency: foo",
	"GoFiles": [
		"[redacted]/.cache/go-build/9a/9aadedc84db6d9c3bceccc8928474b4458080a549bce2fe36a95526e5b523c0f-d"
	],
	"Imports": [
		"foo [foo.test]",
		"os",
		"testing",
		"testing/internal/testdeps"
	],
	"ImportMap": {
		"foo": "foo [foo.test]"
	},
	"Deps": [
		"bufio",
		"bytes",
		"compress/flate",
		"compress/gzip",
		"context",
		"encoding/binary",
		"errors",
		"flag",
		"fmt",
		"foo [foo.test]",
		"hash",
		"hash/crc32",
		"internal/bytealg",
		"internal/cpu",
		"internal/fmtsort",
		"internal/oserror",
		"internal/poll",
		"internal/race",
		"internal/reflectlite",
		"internal/syscall/unix",
		"internal/testlog",
		"io",
		"math",
		"math/bits",
		"os",
		"reflect",
		"regexp",
		"regexp/syntax",
		"runtime",
		"runtime/debug",
		"runtime/internal/atomic",
		"runtime/internal/math",
		"runtime/internal/sys",
		"runtime/pprof",
		"runtime/trace",
		"sort",
		"strconv",
		"strings",
		"sync",
		"sync/atomic",
		"syscall",
		"testing",
		"testing/internal/testdeps",
		"text/tabwriter",
		"time",
		"unicode",
		"unicode/utf8",
		"unsafe"
	]
}
{
	"Dir": "/tmp/tmp.JncAr4CsDl",
	"ImportPath": "foo [foo.test]",
	"Name": "foo",
	"Root": "/tmp/tmp.JncAr4CsDl",
	"ForTest": "foo",
	"Module": {
		"Path": "foo",
		"Main": true,
		"Dir": "/tmp/tmp.JncAr4CsDl",
		"GoMod": "/tmp/tmp.JncAr4CsDl/go.mod",
		"GoVersion": "1.13"
	},
	"Match": [
		"."
	],
	"Stale": true,
	"StaleReason": "build ID mismatch",
	"GoFiles": [
		"foo_test.go"
	],
	"Imports": [
		"testing"
	],
	"Deps": [
		"bytes",
		"context",
		"errors",
		"flag",
		"fmt",
		"internal/bytealg",
		"internal/cpu",
		"internal/fmtsort",
		"internal/oserror",
		"internal/poll",
		"internal/race",
		"internal/reflectlite",
		"internal/syscall/unix",
		"internal/testlog",
		"io",
		"math",
		"math/bits",
		"os",
		"reflect",
		"runtime",
		"runtime/debug",
		"runtime/internal/atomic",
		"runtime/internal/math",
		"runtime/internal/sys",
		"runtime/trace",
		"sort",
		"strconv",
		"strings",
		"sync",
		"sync/atomic",
		"syscall",
		"testing",
		"time",
		"unicode",
		"unicode/utf8",
		"unsafe"
	],
	"TestGoFiles": [
		"foo_test.go"
	],
	"TestImports": [
		"testing"
	]
}
@cagedmantis cagedmantis added this to the Backlog milestone Dec 30, 2019
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 30, 2019
@cagedmantis
Copy link
Contributor

/cc @bcmills @jayconrod

@jayconrod
Copy link
Contributor

It seems like this is working as intended on the go list side. The TestGoFiles, TestImports, XTestGoFiles XTestImports fields are populated. Changing this would break something like go list -f {{.TestGoFiles}} ./....

I'm not sure it makes sense for golang.org/x/tools/go/packages to return an empty package for this though. So maybe this is a bug there? cc @matloob to confirm.

@jayconrod jayconrod changed the title cmd/go: list returns package with no sources in a directory that only contains a test x/tools/go/packages: returns package with no sources in a directory that only contains a test Jan 3, 2020
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jan 3, 2020
@bcmills
Copy link
Contributor

bcmills commented Jan 6, 2020

From the perspective of go list, we at least know that if the given package were to exist, it would have to be in that directory and declare itself to be package foo (rather than, say, package main).

For example, if module example.com includes a test-only package definition for example.com/foo (as ./foo/foo_test.go), then we would not allow module example.com/foo to provide a non-test implementation of that package (as ./foo.go).

@stamblerre
Copy link
Contributor Author

That makes sense - it does sound like a go/packages issue. @matloob, what do you think about how go/packages should handle this?

@matloob
Copy link
Contributor

matloob commented Jan 22, 2020

My philosophy is that go/packages should depart as little as possible, so my preference is that we consider this intended behavior.

To me it seems like go list is trying to satisfy two different models of packages: the one where test packages aren't distinct from non-test packages, before -test existed (where fields like TestGoFiles make sense) and the model when the -test flag is passed where we consider the test and non test packages as independent packages. So this creates a bit of a conflict: go/packages wants to completely exist in the world of the second model, while go list needs to be backwards compatible. I think it's okay to compromise and have weird warts like this in the name of simplicity, though I could be convinced otherwise.

@stamblerre
Copy link
Contributor Author

Thinking about this more, I agree that this is working as intended. This behavior actually allows us to create correct test variants for packages that only have overlays.

@golang golang locked and limited conversation to collaborators Jan 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@cagedmantis @jayconrod @bcmills @stamblerre @gopherbot @matloob