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.

Sign up for GitHub

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

Jump to bottom

x/tools/gopls: unskip all previously flaky regtests #53878

Closed
8 tasks done
findleyr opened this issue Jul 14, 2022 · 14 comments
Closed
8 tasks done

x/tools/gopls: unskip all previously flaky regtests #53878

findleyr opened this issue Jul 14, 2022 · 14 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. 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
gopls/v0.12.0

Comments

@findleyr
Copy link
Contributor

findleyr commented Jul 14, 2022 •
edited
Loading

This is a follow up to https://go.dev/issue/53781, a deterministic panic that made it into [email protected] in part because a flaky regression test had been skipped, and not un-skipped when the root cause of flakiness was resolved.

We have recently made improvements to several potential sources of flakiness: server shutdown races, invalidation races, and performance. We also have resources to work on flakes that weren't available during the 1.18 push. In light of #53781, we should endeavor to unskip (and further de-flake if necessary) all regression tests that have been unconditionally skipped. This issue tracks that effort:

Currently skipped tests:
@findleyr findleyr added this to the gopls/later milestone Jul 14, 2022
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Jul 14, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/417576 mentions this issue: gopls/internal/regtest: unskip TestChangePackageName

@findleyr findleyr added the Testing An issue that has been verified to require only test changes, not just a test failure. label Jul 14, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/418898 mentions this issue: gopls/internal/regtest: unskip Test_Issue38211

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/419106 mentions this issue: gopls/internal/regtest: unskip TestQuickFixEmptyFiles

gopherbot pushed a commit to golang/tools that referenced this issue Jul 22, 2022
@findleyr
gopls/internal/regtest: externalize shouldLoad tracking
a2a2477 
The fundamental bug causing TestChangePackageName to fail has been
fixed, yet unskipping it revealed a new bug: tracking whether or not a
package should be loaded requires that we actually store that package in
s.meta. In cases where we drop metadata, we also lose the information
that a package path needs to be reloaded.

Fix this by significantly reworking the tracking of pending loads, to
simplify the code and separate the reloading logic from the logic of
tracking metadata. As a nice side-effect, this eliminates the needless
work necessary to mark/unmark packages as needing loading, since this is
no longer tracked by the immutable metadata graph.

Additionally, eliminate the "shouldLoad" guard inside of snapshot.load.
We should never ask for loads that we do not want, and the shouldLoad
guard either masks bugs or leads to bugs. For example, we would
repeatedly call load from reloadOrphanedFiles for files that are part of
a package that needs loading, because we skip loading the file scope.
Lift the responsibility for determining if we should load to the callers
of load.

Along the way, make a few additional minor improvements:
 - simplify the code where possible
 - leave TODOs for likely bugs or things that should be simplified in
   the future
 - reduce the overly granular locking in getOrLoadIDsForURI, which could
   lead to strange races
 - remove a stale comment for a test that is no longer flaky.

Updates golang/go#53878

Change-Id: I6d9084806f1fdebc43002c7cc75dc1b94f8514b9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417576
Run-TryBot: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>
Reviewed-by: Suzy Mueller <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
gopherbot pushed a commit to golang/tools that referenced this issue Jul 25, 2022
@findleyr
gopls/internal/regtest: unskip TestQuickFixEmptyFiles
1cfe623 
This test ran slowly sometimes. Now that we have improved performance,
elimininated arbitrary timeouts, and improved cacheability of
computed results when running with -short, I suspect this test should no
longer flake.

If it does, we can reduce its cost in other ways rather than turning it
off entirely.

Updates golang/go#48773
Updates golang/go#53878

Change-Id: I878e78117df5a1a25f4ac5f72e02f28fc078ec73
Reviewed-on: https://go-review.googlesource.com/c/tools/+/419106
Run-TryBot: Robert Findley <[email protected]>
Reviewed-by: Bryan Mills <[email protected]>
gopls-CI: kokoro <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
gopherbot pushed a commit to golang/tools that referenced this issue Jul 25, 2022
@findleyr
gopls/internal/regtest: unskip Test_Issue38211
178fdf9 
This test was originally skipped due to deadline exceeded errors. In the
time since, we've made performance improvements, fixed races, and
altered the regtests to remove arbitrary deadlines. Unskip it to see if
it still flakes.

For golang/go#44098
For golang/go#53878

Change-Id: I06530f2bc9c6883f415dc9147cfcbf260abb2a00
Reviewed-on: https://go-review.googlesource.com/c/tools/+/418898
Run-TryBot: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Bryan Mills <[email protected]>
@findleyr findleyr mentioned this issue Jul 26, 2022
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/420718 mentions this issue: gopls/internal/regtest: unskip all of TestModFileModification

gopherbot pushed a commit to golang/tools that referenced this issue Aug 3, 2022
@findleyr
gopls/internal/regtest: unskip all of TestModFileModification
d08f5dc 
I believe the races described in the issue have been fixed: we should
invalidate mod tidy results on any metadata change. If this invalidation
doesn't work due to a race, we want to know about it.

Update the test to wait for file-related events to complete before
removing files, in an attempt to avoid windows file-locking issues.

For golang/go#40269
For golang/go#53878

Change-Id: I91f0cb4969851010b34904a0b78ab9bd2808f92e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/420718
Run-TryBot: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Suzy Mueller <[email protected]>
@findleyr findleyr modified the milestones: gopls/later, gopls/v0.10.0 Aug 8, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/422910 mentions this issue: gopls/internal/regtest: unskip TestDeleteModule_Interdependent

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/423974 mentions this issue: gopls/internal/regtest: unskip TestSumUpdateFixesDiagnostics

gopherbot pushed a commit to golang/tools that referenced this issue Aug 15, 2022
@findleyr
gopls/internal/regtest: unskip TestSumUpdateFixesDiagnostics
8c83056 
Metadata reloading has been significantly refactored recently. Unskip
this test to see if it still flakes.

For golang/go#51352
For golang/go#53878

Change-Id: I9f2ae1835bbace1b5095c2d45db082c4e709437b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/423974
Run-TryBot: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
gopls-CI: kokoro <[email protected]>
Reviewed-by: Peter Weinberger <[email protected]>
gopherbot pushed a commit to golang/tools that referenced this issue Aug 15, 2022
@findleyr
gopls/internal/regtest: unskip TestDeleteModule_Interdependent
938e162 
Reloading has been significantly refactored recently. Unskip this test
to see if it flakes:
 - If it does not flake, that is a useful signal.
 - If it does flake, that is also a useful signal.

Notably, following CL 419500 we allow network when reloading the
workspace, and so do not need to apply quick-fixes in order to download
the new module from the proxy.

For golang/go#46375
For golang/go#53878

Change-Id: Idde7195730c32bdb434a26b28aac82649dd1b5ac
Reviewed-on: https://go-review.googlesource.com/c/tools/+/422910
Run-TryBot: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Peter Weinberger <[email protected]>
gopls-CI: kokoro <[email protected]>
@findleyr findleyr self-assigned this Aug 15, 2022
@mknyszek
Copy link
Contributor

mknyszek commented Sep 6, 2022

FWIW it looks like TestCreateDependency had a flake:

2022-09-01T03:24:42-49ab44d-86e9e0e/openbsd-amd64-70

Should this get a new issue or is it relevant enough to this?

@mknyszek mknyszek mentioned this issue Sep 6, 2022
Closed
@findleyr
Copy link
Contributor Author

findleyr commented Sep 6, 2022

@mknyszek thanks for reporting.

Hmm. I don't think that flake is actually related to TestCreateDependency: it is simply an error deleting the test files during cleanup.

greplogs also turned up the following recent failure in go/packages tests on openbsd:
https://build.golang.org/log/d9906d7b11d2e43bded9199a00d40d2f8ac47f31

I wonder if this is a recent regression on openbsd? Unclear, so probably best not to assume, and instead file separate new issues for gopls and go/packages.

@bcmills
Copy link
Contributor

bcmills commented Sep 6, 2022

The unlinkat failure looks like #49751 to me.

@findleyr
Copy link
Contributor Author

findleyr commented Sep 6, 2022

Thanks @bcmills. Yes, that looks related, so let's not file new issues.

There is a fair bit of activity on #36435 that may be related.

@findleyr findleyr modified the milestones: gopls/v0.10.0, gopls/v0.10.1 Sep 9, 2022
@findleyr findleyr modified the milestones: gopls/v0.10.1, gopls/v0.10.2 Nov 1, 2022
@findleyr findleyr removed this from the gopls/v0.11.0 milestone Dec 8, 2022
@findleyr findleyr added this to the gopls/v0.12.0 milestone Dec 8, 2022
@findleyr findleyr modified the milestones: gopls/v0.12.0, gopls/v0.13.0 Mar 28, 2023
@findleyr findleyr removed their assignment May 11, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/496882 mentions this issue: gopls/internal/regtest/misc: update some unilaterally skipped tests

gopherbot pushed a commit to golang/tools that referenced this issue May 22, 2023
@findleyr
gopls/internal/regtest/misc: update some unilaterally skipped tests
a12ee94 
Remove skips for two tests related to line directives (now fixed), and
delete a test related to the old parse cache, which no longer exists.

Updates golang/go#53878

Change-Id: I15b1e5d72f5ccc8c094eaa43e73a9bcc1f75c031
Reviewed-on: https://go-review.googlesource.com/c/tools/+/496882
Reviewed-by: Peter Weinberger <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/496883 mentions this issue: gopls/internal/regtest/misc: unskip TestMajorOptionsChange

gopherbot pushed a commit to golang/tools that referenced this issue May 22, 2023
@findleyr
gopls/internal/regtest/misc: unskip TestMajorOptionsChange
6997d19 
This test was fixed in CL 494675, which forced snapshots to observe all
overlays when updating the view.

Updates golang/go#53878
Fixes golang/go#57934

Change-Id: I018bdd260255d6a630c7fc8788935fd69f5e7477
Reviewed-on: https://go-review.googlesource.com/c/tools/+/496883
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
gopls-CI: kokoro <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/496885 mentions this issue: gopls/internal/regtest/misc: update and unskip TestHoverIntLiteral

gopherbot pushed a commit to golang/tools that referenced this issue May 22, 2023
@findleyr
gopls/internal/regtest/misc: update and unskip TestHoverIntLiteral
a70f2bc 
This test is updated to exercise hover over literals, not vars, as was
decided in golang/go#58220.

Updates golang/go#53878

Change-Id: Ic70d3492f28580ebfea24ec08dc47b1ad385c2ff
Reviewed-on: https://go-review.googlesource.com/c/tools/+/496885
Run-TryBot: Robert Findley <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
gopls-CI: kokoro <[email protected]>
Auto-Submit: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
@findleyr findleyr modified the milestones: gopls/v0.13.0, gopls/v0.12.0 May 22, 2023
@findleyr
Copy link
Contributor Author

We have unskipped all flaky regtests.

@golang golang locked and limited conversation to collaborators May 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. 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
Milestone
gopls/v0.12.0
Development

No branches or pull requests
4 participants
@mknyszek @bcmills @gopherbot @findleyr