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/gopls: quickfix_empty_files (govim) test flaky #36795

Closed
myitcv opened this issue Jan 27, 2020 · 11 comments
Closed

x/tools/gopls: quickfix_empty_files (govim) test flaky #36795

myitcv opened this issue Jan 27, 2020 · 11 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@myitcv
Copy link
Member

myitcv commented Jan 27, 2020

What version of Go are you using (go version)?

$ go version
go version devel +73d213708e Sat Jan 25 16:34:18 2020 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20200124220429-8fe064f891f2
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.1.8-0.20200124220429-8fe064f891f2

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/govim/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build059125969=/tmp/go-build -gno-record-gcc-switches"

What did you do?

This is somewhat similar to #36661

We have just added a new govim test that seeks to verify diagnostics are as expected where initially empty .go files exist on disk, but changes to populate those files are then made (but not saved) within the editor.

Like #36661, the scenario involves creating a package p with a simple function DoIt. p is imported by a main package. p also has a test file that exercises DoIt, and an external test file that does the same.

Initially all call sites for DoIt incorrectly pass an integer argument, meaning we should have error diagnostics for all call sites. Then we correct the definition of DoIt to take an integer argument at which point all diagnostics should disappear.

The test can be seen here:

https://github.com/govim/govim/blob/cmd_govim_add_tests_where_files_initially_empty/cmd/govim/testdata/scenario_default/quickfix_empty_files.txt

What did you expect to see?

The diagnostic error:

cannot convert 5 (untyped int constant) to string

for each of the files main.go, p/x_test.go and p/p_test.go

What did you see instead?

Random results

  • Sometimes we are missing any diagnostics for main.go
  • Sometimes we get package p; expected p_test for p/p.go
  • Sometimes we get "could not import mod.com/p (no parsed files for package mod.com/p, expected: [], errors: [], list errors: [-: p/p.go:1:1: expected 'package', found 'EOF']) for p/x_test.go

One such gopls log file:
gopls.log

Tentatively marking as v0.3.0


cc @stamblerre

FYI @leitzler

@myitcv myitcv added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. gopls Issues related to the Go language server, gopls. labels Jan 27, 2020
@myitcv myitcv added this to the gopls/v0.3.0 milestone Jan 27, 2020
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jan 27, 2020
@myitcv
Copy link
Member Author

myitcv commented Jan 27, 2020

Note the test linked above is not yet merged, but can be run locally via:

cd $(mktemp -d)
git clone https://github.com/govim/govim
cd govim
git fetch origin pull/727/head && git checkout FETCH_HEAD
sed -i '/skip/d' cmd/govim/testdata/scenario_default/quickfix_empty_files.txt
go test -exec="dockexec govim/govim:latest-vim -t -e VIM_FLAVOR=vim" -count=1 -v -run "TestScripts/scenario_default/quickfix_empty_files" ./cmd/govim

If you want to use gopls from your PATH, change that last line to:

go test -exec="dockexec govim/govim:latest-vim -t -e VIM_FLAVOR=vim -v $(which gopls):/usr/local/bin/gopls -e GOVIM_USE_GOPLS_FROM_PATH=true" -count=1 -v -run "TestScripts/scenario_default/quickfix_empty_files" ./cmd/govim

@stamblerre
Copy link
Contributor

I believe that https://golang.org/cl/216310 will have fixed this issue.

@stamblerre stamblerre changed the title x/tools/gopls: incorrect diagnostics received with empty files on disk x/tools/gopls: flaky diagnostics for overlays only on disk Jan 27, 2020
@stamblerre stamblerre changed the title x/tools/gopls: flaky diagnostics for overlays only on disk x/tools/gopls: quickfix_empty_files (govim) test flaky Jan 28, 2020
@gopherbot
Copy link

Change https://golang.org/cl/216637 mentions this issue: internal/lsp: recover from a view initialization failure

@stamblerre
Copy link
Contributor

I spoke too soon, but I was able to reproduce and debug this test. The CL above fixed it for me.

@stamblerre stamblerre removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 28, 2020
myitcv pushed a commit to myitcvforks/tools that referenced this issue Jan 28, 2020
If an orphaned file is used to recover a workspace package, we should
remove the initialization error and treat the view as correctly
initialized.

Also, stop caching metadata for packages with no files. We have no way
to invalidate it, and it's useless, so just re-load those files as
needed.

Fixes golang/go#36795.

Change-Id: I0aee5a43401517b6073d27136cca533160effef2
@myitcv
Copy link
Member Author

myitcv commented Jan 28, 2020

We're also seeing issues here that look remarkably similar to #36661 (comment). I'll gather log files.

@myitcv
Copy link
Member Author

myitcv commented Jan 28, 2020

Here are log files with verbose output turned on:

myitcv pushed a commit to myitcvforks/tools that referenced this issue Jan 28, 2020
If an orphaned file is used to recover a workspace package, we should
remove the initialization error and treat the view as correctly
initialized.

Also, stop caching metadata for packages with no files. We have no way
to invalidate it, and it's useless, so just re-load those files as
needed.

Fixes golang/go#36795.
Fixes golang/go#36671.
Fixes golang/go#36772.

Change-Id: I0aee5a43401517b6073d27136cca533160effef2
@stamblerre
Copy link
Contributor

Just wanted to confirm: In the pass case, the files have the path /tmp/blah/govim/cmd/govim/scenario_default/script-quickfix_empty_files/, but in the fail case they have the path /artefacts/govim/cmd/govim/scenario_default/script-quickfix_empty_files/. Is that relevant here at all?

@myitcv
Copy link
Member Author

myitcv commented Jan 30, 2020

Is that relevant here at all?

No. The /artefacts path is from a failed run on CI; the /tmp/blah path is from a passing run on my local machine.

@myitcv myitcv reopened this Jan 30, 2020
@stamblerre
Copy link
Contributor

@myitcv: Are you seeing this issue again with gopls at master?

@stamblerre stamblerre added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Feb 4, 2020
@stamblerre
Copy link
Contributor

Closing this issue due to lack of activity. Please re-open if this remains a problem.

@myitcv
Copy link
Member Author

myitcv commented Feb 24, 2020

Sorry, my bad @stamblerre - totally didn't get round to replying here. All confirmed as good thanks.

@golang golang locked and limited conversation to collaborators Feb 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

3 participants