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

x/tools/go/analysis/internal/checker: TestApplyFixes broken at head #34265

Closed
bcmills opened this issue Sep 12, 2019 · 4 comments
Closed

x/tools/go/analysis/internal/checker: TestApplyFixes broken at head #34265

bcmills opened this issue Sep 12, 2019 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations) Testing An issue that has been verified to require only test changes, not just a test failure. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Sep 12, 2019

Lots of breakage showing on https://build.golang.org/?repo=golang.org%2fx%2ftools.

It appears that the tests are ok with release-branch.go1.12, but builds using a go build from head are consistently failing in TestApplyFixes.

Example (https://build.golang.org/log/f7cec2ac4b2787f43974ffa14f8bb2a5929c50ec):

2019/09/11 20:10:38 file=/workdir/tmp/analysistest525415992/src/rename/test.go matched no packages
--- FAIL: TestApplyFixes (0.06s)
    checker_test.go:61: contents of rewritten file
        got: package rename
        
        func Foo() {
        	bar := 12
        	_ = bar
        }
        
        // the end
        
        want: package rename
        
        func Foo() {
        	baz := 12
        	_ = baz
        }
        
        // the end
FAIL
FAIL	golang.org/x/tools/go/analysis/internal/checker	0.080s

From the matched no packages line, I wonder if this was introduced by the fix for #32483.

CC @jayconrod @ianthehat @matloob

@gopherbot gopherbot added this to the Unreleased milestone Sep 12, 2019
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations) Testing An issue that has been verified to require only test changes, not just a test failure. and removed Tools This label describes issues relating to any tools in the x/tools repository. labels Sep 12, 2019
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@jayconrod
Copy link
Contributor

This reproduces with go1.13.

Bisect in x/tools points to CL 186337 as the first failing commit. @matloob @jirfag please fix or revert. (And please, please run TryBots before merging).

@matloob
Copy link
Contributor

matloob commented Sep 12, 2019

Thanks for finding this. I'm going to revert for now.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/195061 mentions this issue: go/analysis/internal/checker: set GO111MODULE to off

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/195065 mentions this issue: go/packages: suppress go list -e error when directory outside modules

clintjedwards pushed a commit to clintjedwards/tools that referenced this issue Sep 19, 2019
If an absolute directory path being listed is outside any modules,
go list -e returns a non-zero exit status and non-empty stderr, but
should suppress the error. This was causing a weird bug when golang.org/cl/186337
was submitted because that changed the conditions when -export was passed,
which in turn affected how we suppressed the go list -e error (because
-export causes a compile it overtriggers errors, so we explicitly
suppress errors in that case). The way the error was being suppressed,
no error was generated, and no fake package was generated (which go list
is supposed to do), so the contains query fallback code wasn't run.

Fixes golang/go#34265
Updates golang/go#34273

Change-Id: I1213cff0e03a62c6976e50db5b2d805aa3ddbb7a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/195065
Run-TryBot: Michael Matloob <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Rebecca Stambler <[email protected]>
@golang golang locked and limited conversation to collaborators Sep 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations) Testing An issue that has been verified to require only test changes, not just a test failure. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants