Fix for gotype bug against tests #105

akutz · 2016-03-21T00:17:06Z

This patch fixes issue #91 and and implements this workaround.

Essentially go test -i PKG_PATH is executed against any path prior to gotype being executed against that same path when the gometalinter flag -tests is used. This ensures that the test dependencies are installed as go packages, enabling gotype to resolve the imports.

To verify the patch I executed the following steps:

Remove Previous Packages

$ rm -fr $GOPATH/pkg/*/github.com/emccode/libstorage

Execute `gometalinter`

Note in the output below the debug statements:

DEBUG: executing 'go test -i api/utils/schema'
DEBUG: executing 'go test -i .'

$ gometalinter --enable=gotype --tests --json --vendor -errors --debug . ./api/utils/schema/
DEBUG: PATH=/Users/akutz/Projects/go/bin:/Users/akutz/Library/Python/2.7/bin:/Library/Java/JavaVirtualMachines/jdk1.8.0_20.jdk/Contents/Home/bin:/Users/akutz/Scripts/devops/tools:/Users/akutz/Scripts:/Users/akutz/Scripts/krakatau:/Users/akutz/Source/launch4j:/Users/akutz/Projects/ruby/igit:/Users/akutz/.brew/bin:/Users/akutz/.rubies/ruby-2.1.2/bin:/Users/akutz/Servers/mysql/5.7.7/bin:/Users/akutz/.brew/Cellar/maven/3.3.1/libexec:/Users/akutz/Projects/go/bin:/Users/akutz/.go/1.6/bin:/Users/akutz/.brew/bin:/Users/akutz/.opt/make/4.1/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/X11/bin
DEBUG: linting path .
DEBUG: linting path api/utils/schema
DEBUG: linting with gotype: gotype -e {tests=-a} . (on .)
DEBUG: linting with varcheck: varcheck . (on api/utils/schema)
DEBUG: linting with gocyclo: gocyclo -over {mincyclo} . (on api/utils/schema)
DEBUG: linting with gocyclo: gocyclo -over {mincyclo} . (on .)
DEBUG: linting with varcheck: varcheck . (on .)
DEBUG: linting with gotype: gotype -e {tests=-a} . (on api/utils/schema)
DEBUG: linting with deadcode: deadcode . (on .)
DEBUG: linting with deadcode: deadcode . (on api/utils/schema)
DEBUG: linting with dupl: dupl -plumbing -threshold {duplthreshold} ./*.go (on api/utils/schema)
DEBUG: linting with unconvert: unconvert . (on .)
DEBUG: linting with vetshadow: go tool vet --shadow ./*.go (on api/utils/schema)
DEBUG: linting with goconst: goconst -min-occurrences {min_occurrences} . (on .)
DEBUG: linting with unconvert: unconvert . (on api/utils/schema)
[
DEBUG: linting with goconst: goconst -min-occurrences {min_occurrences} . (on api/utils/schema)
DEBUG: linting with dupl: dupl -plumbing -threshold {duplthreshold} ./*.go (on .)
DEBUG: executing 'go test -i api/utils/schema'
DEBUG: executing 'go test -i .'
DEBUG: linting with vetshadow: go tool vet --shadow ./*.go (on .)
DEBUG: executing /Users/akutz/Projects/go/bin/unconvert ["."]
DEBUG: executing /Users/akutz/Projects/go/bin/deadcode ["."]
DEBUG: executing /Users/akutz/Projects/go/bin/varcheck ["."]
DEBUG: executing /Users/akutz/Projects/go/bin/dupl ["-plumbing" "-threshold" "50" "schema.go" "schema_generated.go" "schema_test.go"]
DEBUG: executing /Users/akutz/Projects/go/bin/gocyclo ["-over" "10" "."]
DEBUG: executing /Users/akutz/Projects/go/bin/gocyclo ["-over" "10" "."]
DEBUG: executing /Users/akutz/Projects/go/bin/goconst ["-min-occurrences" "3" "."]
DEBUG: deadcode hits 0: ^deadcode: (?P<path>[^:]+):(?P<line>\d+):(?P<col>\d+):\s*(?P<message>.*)$
DEBUG: deadcode linter took 19.564549ms
DEBUG: linting with golint: golint -min_confidence {min_confidence} . (on .)
DEBUG: executing /Users/akutz/Projects/go/bin/varcheck ["."]
DEBUG: executing /Users/akutz/Projects/go/bin/dupl ["-plumbing" "-threshold" "50" "libstorage.go" "libstorage_run_config_test.go" "libstorage_run_config_tls_test.go" "libstorage_run_test.go" "libstorage_test.go" "libstorage_tests_test.go"]
DEBUG: executing /Users/akutz/Projects/go/bin/goconst ["-min-occurrences" "3" "."]
DEBUG: executing /Users/akutz/Projects/go/bin/unconvert ["."]
DEBUG: executing /Users/akutz/.go/1.6/bin/go ["tool" "vet" "--shadow" "schema.go" "schema_generated.go" "schema_test.go"]
DEBUG: executing /Users/akutz/Projects/go/bin/deadcode ["."]
DEBUG: executing /Users/akutz/.go/1.6/bin/go ["tool" "vet" "--shadow" "libstorage.go" "libstorage_run_config_test.go" "libstorage_run_config_tls_test.go" "libstorage_run_test.go" "libstorage_test.go" "libstorage_tests_test.go"]
DEBUG: executing /Users/akutz/Projects/go/bin/golint ["-min_confidence" "0.800000" "."]
DEBUG: gocyclo hits 0: ^(?P<cyclo>\d+)\s+\S+\s(?P<function>\S+)\s+(?P<path>[^:]+):(?P<line>\d+):(\d+)$
DEBUG: gocyclo linter took 58.081438ms
DEBUG: linting with golint: golint -min_confidence {min_confidence} . (on api/utils/schema)
DEBUG: executing /Users/akutz/Projects/go/bin/golint ["-min_confidence" "0.800000" "."]
DEBUG: gocyclo hits 0: ^(?P<cyclo>\d+)\s+\S+\s(?P<function>\S+)\s+(?P<path>[^:]+):(?P<line>\d+):(\d+)$
DEBUG: gocyclo linter took 58.74632ms
DEBUG: linting with vet: go tool vet ./*.go (on .)
DEBUG: executing /Users/akutz/.go/1.6/bin/go ["tool" "vet" "libstorage.go" "libstorage_run_config_test.go" "libstorage_run_config_tls_test.go" "libstorage_run_test.go" "libstorage_test.go" "libstorage_tests_test.go"]
DEBUG: dupl hits 2: ^(?P<path>[^\s][^:]+?\.go):(?P<line>\d+)-\d+:\s*(?P<message>.*)$
  {"linter":"dupl","severity":"warning","path":"api/utils/schema/schema_test.go","line":41,"col":0,"message":"duplicate of schema_test.go:158-227"}DEBUG: dupl linter took 66.761502ms
,
  {"linter":"dupl","severity":"warning","path":"api/utils/schema/schema_test.go","line":158,"col":0,"message":"duplicate of schema_test.go:41-134"}DEBUG: linting with vet: go tool vet ./*.go (on api/utils/schema)
DEBUG: dupl hits 4: ^(?P<path>[^\s][^:]+?\.go):(?P<line>\d+)-\d+:\s*(?P<message>.*)$
DEBUG: executing /Users/akutz/.go/1.6/bin/go ["tool" "vet" "schema.go" "schema_generated.go" "schema_test.go"]
DEBUG: dupl linter took 64.311889ms
DEBUG: linting with interfacer: interfacer ./ (on .)
,
  {"linter":"dupl","severity":"warning","path":"libstorage_tests_test.go","line":11,"col":0,"message":"duplicate of libstorage_tests_test.go:27-41"},
  {"linter":"dupl","severity":"warning","path":"libstorage_tests_test.go","line":27,"col":0,"message":"duplicate of libstorage_tests_test.go:11-25"},
  {"linter":"dupl","severity":"warning","path":"libstorage_test.go","line":273,"col":0,"message":"duplicate of libstorage_test.go:379-405"},
  {"linter":"dupl","severity":"warning","path":"libstorage_test.go","line":379,"col":0,"message":"duplicate of libstorage_test.go:273-299"}DEBUG: executing /Users/akutz/Projects/go/bin/interfacer ["./"]
DEBUG: goconst hits 0: ^(?P<path>[^\s][^\r\n:]+?\.go):(?P<line>\d+):(?P<col>\d+):\s*(?P<message>.*)$
DEBUG: goconst linter took 110.685737ms
DEBUG: linting with interfacer: interfacer ./ (on api/utils/schema)
DEBUG: executing /Users/akutz/Projects/go/bin/interfacer ["./"]
DEBUG: deadcode hits 0: ^deadcode: (?P<path>[^:]+):(?P<line>\d+):(?P<col>\d+):\s*(?P<message>.*)$
DEBUG: deadcode linter took 143.240785ms
DEBUG: linting with ineffassign: ineffassign -n . (on .)
DEBUG: executing /Users/akutz/Projects/go/bin/ineffassign ["-n" "."]
DEBUG: goconst hits 0: ^(?P<path>[^\s][^\r\n:]+?\.go):(?P<line>\d+):(?P<col>\d+):\s*(?P<message>.*)$
DEBUG: goconst linter took 201.32094ms
DEBUG: linting with ineffassign: ineffassign -n . (on api/utils/schema)
DEBUG: executing /Users/akutz/Projects/go/bin/ineffassign ["-n" "."]
DEBUG: ineffassign hits 0: ^(?P<path>[^\s][^\r\n:]+?\.go):(?P<line>\d+):(?P<col>\d+):\s*(?P<message>.*)$
DEBUG: ineffassign linDEBUG: ineffassign hits 0: ^(?P<path>[^\s][^\r\n:]+?\.go):(?P<line>\d+):(?P<col>\d+):\s*(?P<message>.*)$
DEBUG: ineffassign linter took 21.243519ms
ter took 69.322684ms
DEBUG: linting with aligncheck: aligncheck . (on .)
DEBUG: linting with aligncheck: aligncheck . (on api/utils/schema)
DEBUG: executing /Users/akutz/Projects/go/bin/aligncheck ["."]
DEBUG: executing /Users/akutz/Projects/go/bin/aligncheck ["."]
DEBUG: golint hits 0: ^(?P<path>[^\s][^\r\n:]+?\.go):(?P<line>\d+):(?P<col>\d+):\s*(?P<message>.*)$
DEBUG: golint linter took 179.338297ms
DEBUG: linting with structcheck: structcheck {tests=-t} . (on .)
DEBUG: executing /Users/akutz/Projects/go/bin/structcheck ["-t" "."]
DEBUG: warning: go tool vet --shadow ./*.go returned exit status 1
DEBUG: vetshadow hits 1: ^(?P<path>[^\s][^\r\n:]+?\.go):(?P<line>\d+):\s*(?P<message>.*)$
DEBUG: vetshadow linter took 377.53227ms
DEBUG: linting with structcheck: structcheck {tests=-t} . (on api/utils/schema)
,
  {"linter":"vetshadow","severity":"warning","path":"api/utils/schema/schema.go","line":143,"col":0,"message":"declaration of err shadows declaration at schema.go:131: "}DEBUG: executing /Users/akutz/Projects/go/bin/structcheck ["-t" "."]
DEBUG: vet hits 0: ^(?P<path>[^\s][^\r\n:]+?\.go):(?P<line>\d+):\s*(?P<message>.*)$
DEBUG: vet linter took 425.997133ms
DEBUG: linting with errcheck: errcheck -abspath . (on .)
DEBUG: executing /Users/akutz/Projects/go/bin/errcheck ["-abspath" "."]
DEBUG: vet hits 0: ^(?P<path>[^\s][^\r\n:]+?\.go):(?P<line>\d+):\s*(?P<message>.*)$
DEBUG: vet linter took 1.300189613s
DEBUG: linting with errcheck: errcheck -abspath . (on api/utils/schema)
DEBUG: executing /Users/akutz/Projects/go/bin/errcheck ["-abspath" "."]
DEBUG: golint hits 0: ^(?P<path>[^\s][^\r\n:]+?\.go):(?P<line>\d+):(?P<col>\d+):\s*(?P<message>.*)$
DEBUG: golint linter took 1.581671452s
DEBUG: vetshadow hits 0: ^(?P<path>[^\s][^\r\n:]+?\.go):(?P<line>\d+):\s*(?P<message>.*)$
DEBUG: vetshadow linter took 2.232722602s
DEBUG: executing /Users/akutz/Projects/go/bin/gotype ["-e" "-a" "."]
DEBUG: executing /Users/akutz/Projects/go/bin/gotype ["-e" "-a" "."]
DEBUG: gotype hits 0: ^(?P<path>[^\s][^\r\n:]+?\.go):(?P<line>\d+):(?P<col>\d+):\s*(?P<message>.*)$
DEBUG: gotype linter took 4.733503826s

]
WARNING: deadline exceeded by linter errcheck on api/utils/schema (try increasing --deadline)
WARNING: deadline exceeded by linter gotype on . (try increasing --deadline)
WARNING: deadline exceeded by linter varcheck on . (try increasing --deadline)
WARNING: deadline exceeded by linter varcheck on api/utils/schema (try increasing --deadline)
WARNING: deadline exceeded by linter aligncheck on api/utils/schema (try increasing --deadline)
WARNING: deadline exceeded by linter structcheck on . (try increasing --deadline)
WARNING: deadline exceeded by linter structcheck on api/utils/schema (try increasing --deadline)
WARNING: deadline exceeded by linter errcheck on . (try increasing --deadline)
WARNING: deadline exceeded by linter unconvert on api/utils/schema (try increasing --deadline)
WARNING: deadline exceeded by linter unconvert on . (try increasing --deadline)
WARNING: deadline exceeded by linter interfacer on . (try increasing --deadline)
WARNING: deadline exceeded by linter interfacer on api/utils/schema (try increasing --deadline)
WARNING: deadline exceeded by linter aligncheck on . (try increasing --deadline)
DEBUG: total elapsed time 5.017612716s

The fact that the linter executed successfully proves that the patch works as without this patch the gotype linter would have failed with an error regarding imports in the test sources.

This patch fixes the issue described at alecthomas#91 and implements the workaround described by alecthomas#91 (comment). Essentially `go test -i PKG_PATH` is executed against any path prior to `gotype` being executed against that same path when the `gometalinter` flag `-tests` is used. This ensures that the test dependencies are installed as go packages, enabling `gotype` to resolve the imports.

alecthomas · 2016-03-21T00:48:10Z

I appreciate the PR, but there are some problems.

This is a kludge. Iff this is the solution, it should be done in a general purpose way and not special cased for gotype.
This doesn't fix gotype for non-test packages.
I generally don't think that gometalinter should be working around issues in underlying linters. That is a slippery slope.
I'm not convinced that gometalinter should be installing packages at all.

It's a good point though that there is basically no way to compile check a test without either a) using gotype after installing all deps, or b) running the test. This is definitely not ideal.

akutz · 2016-03-21T00:55:48Z

Hi @alecthomas,

Thank you for the response. Regarding your comments; all valid. Some feedback though:

According to the Google dev this is a case unique to gotype.
For non-test packages you can simply do go install. I just assumed that's part of people's default workflows anyway, but you're right, for this case it would need to be added for non test packages.
I don't either, but gotype seems to be an important linter, and I received just no feedback in the Atom go-plus package with gotype disabled thanks to its role with gometalinter. You seem to have been willing to remove gotype in favor of some other linter, but that didn't happen?
Agreed. However the tool is broken in a fundamental way without gotype, and that's broken in a fundamental way without go test -i or, to your point, go install.

akutz · 2016-03-21T00:59:07Z

Hi @alecthomas,

Let me be the first to admit that I'm definitely approaching this issue from the context of using gometalinter as part of go-plus for Atom. I realize my statement about "broken in a fundamental way" doesn't really apply external to that context.

akutz · 2016-03-21T00:59:48Z

Hmmm, so perhaps I just answered my own question. Perhaps the fix should be with Atom's go-plus package instead... @alecthomas, what are your thoughts?

alecthomas · 2016-03-21T01:02:40Z

According to the Google dev this is a case unique to gotype.

What I was referring to here was that instead of special-casing gotype, change gometalinter so that it supports running multiple commands in sequence when linting. Then go test -i just becomes another command that runs.

alecthomas · 2016-03-21T01:21:46Z

Hmmm, so perhaps I just answered my own question. Perhaps the fix should be with Atom's go-plus package instead... @alecthomas, what are your thoughts?

I'm honestly not sure. I think there are some fairly fundamental problems with the way go/types works when it comes to linters.

Linters don't generally care about anything except the package/code they're immediately linting. go/types on the other hand, can/will parse/compile all dependencies. This can be very very slow and consume a lot of RAM.
Linters want to just process the source and report issues, not generate compiler artifacts. gotype in particular expects installed packages, and that also seems to be the case for the new incarnation of gotype that uses go/types.

A lot of people seem to use gometalinter in CI, so I'm guessing they must go test -i and go build -i before running it.

akutz · 2016-03-21T01:50:01Z

Hi @alecthomas,

I would absolutely use the suggested approach to CI, but it's an issue of gometalinter being incorporated into the de facto go plug-in for Atom that's the problem.

akutz · 2016-03-21T01:51:47Z

Honestly, I think you provided the correct resolution path by virtue of your statement:

Linters don't generally care about anything except the package/code they're immediately linting.

True, you said package as well, but in the second point you once again highlighted source code:

Linters want to just process the source and report issues, not generate compiler artifacts.

From these statements one could draw the conclusion that gotype is just a bad tool to use as a linter and thus should be replaced.

akutz mentioned this pull request Mar 21, 2016

x/tools/cmd/gotype: "could not import" packages golang/go#12703

Closed

alecthomas closed this Mar 21, 2016

alecthomas mentioned this pull request Mar 21, 2016

Replace gotype #91

Closed

zoumo mentioned this pull request Nov 21, 2017

Feat: add cli pkg caicloud/nirvana#25

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for gotype bug against tests #105

Fix for gotype bug against tests #105

akutz commented Mar 21, 2016

alecthomas commented Mar 21, 2016

akutz commented Mar 21, 2016

akutz commented Mar 21, 2016

akutz commented Mar 21, 2016

alecthomas commented Mar 21, 2016

alecthomas commented Mar 21, 2016

akutz commented Mar 21, 2016

akutz commented Mar 21, 2016

Fix for gotype bug against tests #105

Fix for gotype bug against tests #105

Conversation

akutz commented Mar 21, 2016

Remove Previous Packages

Execute gometalinter

alecthomas commented Mar 21, 2016

akutz commented Mar 21, 2016

akutz commented Mar 21, 2016

akutz commented Mar 21, 2016

alecthomas commented Mar 21, 2016

alecthomas commented Mar 21, 2016

akutz commented Mar 21, 2016

akutz commented Mar 21, 2016

Execute `gometalinter`