x/tools/gopls: TestResolveImportCycle is flaky

[findleyr]

This test was added newly in https://golang.org/cl/326589, and appears to be flaky.

Sample failure:
https://build.golang.org/log/1919547c028c2fd16c892afb37ab336de61675ae

It may be that the fix for import cycle resolution is racy.

[bcmills]

Ah, that explains the TryBot failure in https://storage.googleapis.com/go-build-log/011d0a8d/linux-amd64_e6bc22ff.log (CL 328231)!

[gopherbot]

Change https://golang.org/cl/328369 mentions this issue: gopls/internal/regtest: skip the flaky TestResolveImportCycle

[LeGEC]

A number of tests seem to fail with the error : err:context deadline exceeded (see logs mentioned in the current issue, or #46930 or any of the logs I could inspect in #46183).

In the mentioned logs : all tests that fail mention that some test took longer than 20s, which is the default context timeout for regtest.Run()

# example from the log mentioned in this issue :
   --- FAIL: TestResolveImportCycle/experimental_workspace_module (20.00s)
   
# other issues mention other failing tests, with a run time of ~20s and something

When inspecting these tests individually (I ran TestResolveImportCycle and TestProgressBarErrors on my machine), I wasn't able to trigger a context deadline exceeded issue when running repeatedly the test sequentially :

# I wasn't able to trigger the error this way :
for i in {1..100}; do
    go test -count=1 -run TestResolveImportCycle golang.org/x/tools/gopls/internal/regtest/diagnostics
done

However I was able to trigger the error when running several times the tests in parallel :

# this triggered the error, not always but with some non 0 probability :
for i in {1..10}; do
    go test -count=1 -run TestResolveImportCycle golang.org/x/tools/gopls/internal/regtest/diagnostics &  # <- in parallel
done
wait

# this also highlighted the TestProgressBarErrors bug

it looks like, in some case, the runner started by internal/lsp/regtest/Runner.Run just gets stuck.

haven't dug deeper yet

[findleyr]

@LeGEC thanks for looking into this. I suspect due to the flake frequency of this particular test that there is an underlying race in gopls itself -- haven't had time to look into it yet. Other regtest failures are simply due to resource contention on the builders. The gopls regtests spawn a lot of subprocesses and are very memory intensive, which affects various builders differently. I try to be conservative and disable, shard, or rewrite tests as soon as they prove to be flaky, but hesitate to disable the regtests entirely when run with -short, because they catch a lot of bugs.

Are you just looking into this to help out, or are the regtests causing you problems in some way? Either way, thanks again.

[LeGEC]

@findleyr : I was picking some issues just as a way to start looking into gopls codebase.

These tests do not cause specific issues on my side.

[gopherbot]

Change https://golang.org/cl/333289 mentions this issue: internal/lsp/regtest: allow for unsent diagnostics in TestResolveImportCycle

[findleyr]

It appears the failure mode fixed by CL 333289 is not the only one. Sample:
https://build.golang.org/log/b9e55f800f2b3728fd22e9322b67eeb780ceb2f4

[gopherbot]

Change https://golang.org/cl/333510 mentions this issue: internal/lsp/regtest: fix a panic TestResolveImportCycle

[findleyr]

No failures since July 9th (when the above CL was submitted). Closing.