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: regtest flakes on android-amd64-emu #43554

Open
findleyr opened this issue Jan 6, 2021 · 7 comments
Open

x/tools/gopls: regtest flakes on android-amd64-emu #43554

findleyr opened this issue Jan 6, 2021 · 7 comments
Assignees
Labels
Milestone

Comments

@findleyr
Copy link
Contributor

@findleyr findleyr commented Jan 6, 2021

At least 2 regtests are flaking with some regularity on android-amd64-emu: TestWorkspaceDirAccess and TestUnimportedCompletion.

Due to the nature of these failures (they look like races), I don't think we should just ignore them. However, android is a low priority for gopls and they are causing noise on the builders.

I think we should exclude them from -short for now, but follow up.

CC @stamblerre

@gopherbot gopherbot added this to the Unreleased milestone Jan 6, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 6, 2021

Change https://golang.org/cl/281857 mentions this issue: gopls/internal/regtest: skip regtests on android-amd64-emu

@stamblerre stamblerre modified the milestones: Unreleased, gopls/vscode-go Jan 6, 2021
@stamblerre stamblerre added this to Needs Triage in vscode-go: gopls by default via automation Jan 6, 2021
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jan 6, 2021

Agreed that we should look at these tests ASAP.

gopherbot pushed a commit to golang/tools that referenced this issue Jan 6, 2021
For golang/go#43554

Change-Id: If833da80784833eb355d8a616fdc778207f9b682
Reviewed-on: https://go-review.googlesource.com/c/tools/+/281857
Run-TryBot: Robert Findley <rfindley@google.com>
Trust: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
@stamblerre stamblerre moved this from Needs Triage to Critical in vscode-go: gopls by default Jan 7, 2021
@findleyr findleyr self-assigned this Jan 7, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 11, 2021

Change https://golang.org/cl/283052 mentions this issue: internal/lsp/fake: use hash rather than mtime to identify workdir files

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 11, 2021

Change https://golang.org/cl/283053 mentions this issue: internal/lsp/cache: compare file size when invalidating file cache

gopherbot pushed a commit to golang/tools that referenced this issue Jan 11, 2021
For golang/go#43554

Change-Id: If833da80784833eb355d8a616fdc778207f9b682
Reviewed-on: https://go-review.googlesource.com/c/tools/+/281857
Run-TryBot: Robert Findley <rfindley@google.com>
Trust: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 12, 2021

Change https://golang.org/cl/283352 mentions this issue: gopls/internal/regtest: avoid flake in TestGoModInvalidesOnSave

gopherbot pushed a commit to golang/tools that referenced this issue Jan 12, 2021
On builders with low resolution clocks (e.g. WSL), it's possible for us
to miss file events that occur within a single 'tick'.

Fix this by instead tracking full file identity. Since we should have
only a few relatively small files in tests, the additional overhead
should be small.

For golang/go#43554

Change-Id: I05a48567f83007ff2278145638547c6ebb2523fd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/283052
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Trust: Robert Findley <rfindley@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Jan 12, 2021
Our filesystem caching layer uses file modification time to invalidate
file contents. This is an imperfect heuristic, and on certain operating
systems with low resolution filesystem clocks (such as WSL), this can be
broken in practice.

A proper fix would be to just read the file contents directly and rely
on the snapshot to optimize file access, but we don't know that this is
a safe change. Instead, try to reduce the likelihood of false cache hits
by also checking the file size reported by Stat.

For golang/go#43554

Change-Id: I1af384db532725e84fa6f3a2e5469d10b43fee92
Reviewed-on: https://go-review.googlesource.com/c/tools/+/283053
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Trust: Robert Findley <rfindley@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Jan 12, 2021
Investigating flakes in TestGoModInvalidatesOnSave highlighted a couple
bugs:
 + A didOpen on a go.mod file invalidates the workspace, since it is
   treated as a saved change. This, in itself, is probably not such a
   big deal, except that...
 + When metadata is deleted on this didOpen, we break the workspace
   before it can be recomputed.

Fix this by awaiting diagnostics from the didOpen, but it would be
better to have a more robust fix (e.g. CL 271477).

For golang/go#43554

Change-Id: I75d49b818ae2f3730a48ac6a473c24c664227523
Reviewed-on: https://go-review.googlesource.com/c/tools/+/283352
Run-TryBot: Robert Findley <rfindley@google.com>
Trust: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 13, 2021

Change https://golang.org/cl/283512 mentions this issue: gopls/internal/regtest: fix synchronization for TestUseGoplsMod

gopherbot pushed a commit to golang/tools that referenced this issue Jan 13, 2021
Jumping to definition in a regtest can indirectly lead to a didOpen
call, so the awaits added to TestUseGoplsMod to synchronize metadata
were ineffectual. On Android, this can lead to the race described in
golang/go#43652.

But in any case, all this bookkeeping of notifications is fragile. Avoid
it entirely by having the fake editor keep track of notification
statistics. In the future, we should use this to clean up many existing
regtests.

For golang/go#43554
For golang/go#39384

Change-Id: Icd1619bd5cdd2f646d1a0050f5beaf2ab1c27f37
Reviewed-on: https://go-review.googlesource.com/c/tools/+/283512
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Trust: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Jan 13, 2021

I think this is now fixed, but I'll wait for more evidence that flakes are gone before closing.

@stamblerre stamblerre modified the milestones: gopls/v0.6.3, gopls/v1.0.0 Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
vscode-go: gopls by default
  
To Investigate
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.