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

x/tools/go/packages: no way to easily distinguish the test version of the package from the real one #27910

Open
bcmills opened this Issue Sep 27, 2018 · 4 comments

Comments

Projects
None yet
4 participants
@bcmills
Copy link
Member

bcmills commented Sep 27, 2018

When Config.Test is set to true, packages.Load may return multiple Package instances with the same PkgPath.

go list encodes the information about which package is which redundantly: both in the ForTest field and also in the reported ImportPath for test variants.

go/packages strips out the ImportPath annotations, but also fails to propagate the ForTest field. As a result, the only way to distinguish the two packages is by checking which one's GoFiles are a superset of the other's. That works most of the time, but I suspect it will fail in some cases (for example, when b imports a and a_test imports b, the test version of b for a_test is compile with exactly the same sources as the non-test copy of b).

CC: @matloob @alandonovan @ianthehat

@gopherbot gopherbot added this to the Unreleased milestone Sep 27, 2018

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Sep 28, 2018

the only way to distinguish the two packages

There are a couple other alternatives I hadn't considered:

  • I can check for files with a _test.go suffix, assuming that all build systems will produce that suffix for test files (including generated ones).

  • If the Imports field is set, I can check for an import of the testing package, assuming that all tests actually import it. (But are they required to? What happens if a test defines func main() instead?)

At any rate, if there is some specific alternative that is expected to be reliable enough for all users of go/packages, it should be described in the go/packages documentation.

@nmiyake

This comment has been minimized.

Copy link

nmiyake commented Dec 24, 2018

I'm running into this right now as well -- I'm looking at transitioning the deadcode tool to work with Go modules (#24661), and encountered this when transitioning to x/tools/go/packages.

I believe that adding a field like ForTest in the packages.Package struct would fix this.

Without this, right now I am left to depend on parsing output of the ID field to determine whether or not a package comes from a test, which is brittle (and explicitly against the advice of the documentations, which says that the ID field should not be parsed). However, my current guess is that this will have fewer edge cases than depending on the files of one version of the package being a superset of the other.

This is based on the output that I currently get from using packages.Load given a directory testdata/p3 with sample.go and sample_test.go (both of package p) in a module github.com/org/deadcode:

  • packages.Package{ID:"github.com/org/deadcode/testdata/p3", Name:"p", PkgPath:"github.com/org/deadcode/testdata/p3", ...
  • packages.Package{ID:"github.com/org/deadcode/testdata/p3 [github.com/org/deadcode/testdata/p3.test]", Name:"p", PkgPath:"github.com/org/deadcode/testdata/p3", ...
  • packages.Package{ID:"github.com/org/deadcode/testdata/p3.test", Name:"main", PkgPath:"github.com/org/deadcode/testdata/p3.test", ...
@nmiyake

This comment has been minimized.

Copy link

nmiyake commented Dec 24, 2018

@bcmills with regards to your 2 other suggestions in your follow-on comment:

  • The first approach seems like it would work (with the caveat of build systems you note), although it does also require a check to distinguish between a test and external test package (verify that package name is p AND it has _test.go files -- otherwise, if package package p_test exists, that will also match)
  • I don't think the second approach will work generally -- besides the issue you mentioned in your comment, it's also possible (albeit unlikely) for non-test files to import the testing package, in which case you could get false positives
@matloob

This comment has been minimized.

Copy link
Contributor

matloob commented Jan 10, 2019

cc @ianthehat because he had a hat

It's not totally obvious where to draw the line of test vs non-test package. By my count, there's three categories of packages that can be considered tests:

  • The test main package (ForTest edges point to these)
  • The package containing test code (the test variant of the library being tested, and the xtest if it exists)
  • and most confusingly: the test variants of the dependencies of the test code package.

It's difficult to distinguish between the second group and the third group (at least without doing failure prone package name comparisons).

I think it would be confusing and misleading to mark all three as "tests", and I also think that the ForTest field isn't quite what people want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment