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

Comments

@myitcv
Copy link
Member

@myitcv 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 this to the gopls/v0.3.0 milestone Jan 27, 2020
@gopherbot gopherbot added the Tools label Jan 27, 2020
@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv 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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jan 27, 2020

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 28, 2020

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

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jan 28, 2020

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

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

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Jan 28, 2020

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

@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv 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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jan 28, 2020

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

This comment has been minimized.

Copy link
Member Author

@myitcv 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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jan 30, 2020

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

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Feb 19, 2020

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

@stamblerre stamblerre closed this Feb 19, 2020
@myitcv

This comment has been minimized.

Copy link
Member Author

@myitcv myitcv commented Feb 24, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.