x/tools/gopls: quickfix_new_files (govim) test flaky #36661
Comments
myitcv
added
NeedsInvestigation
gopherbot
added
the
Tools
|
I tried reproducing this, but I wasn't able to get a failing test. Is this test expected to fail or will the logs just show the incorrect output? |
stamblerre
added
WaitingForInfo
|
The test is not merged yet, but you can do: and then remove the Full repro steps using gives |
stamblerre
added
NeedsInvestigation
|
Thanks for the repro! I'll look into it ASAP. |
|
The test is now merged so repro steps are: |
|
The problem here is that the initial workspace load finds nothing, and AFAIK we currently have no mechanism for adding packages to the workspace set, so they don't ever end up getting diagnosed in the background. |
|
FWIW this will be a common situation for Vim (and other non-VSCode) developers where the editor has an in-memory buffer that can exist without the file needing to exist on disk. |
|
Change https://golang.org/cl/216141 mentions this issue: |
|
Re-opening because we're seeing some flakey behaviour here. About 20% of the time in We are seeing these failures again 8fe064f The test mentioned above is failing with the following diff: [stderr]
[
{
"bufname": "main.go",
- "col": 8,
- "lnum": 3,
+ "col": 9,
+ "lnum": 6,
"module": "",
"nr": 0,
"pattern": "",
- "text": "could not import mod.com/p (no package for import mod.com/p)",
+ "text": "cannot convert 5 (untyped int constant) to string",
"type": "",
"valid": 1,
"vcol": 0
},
{
"bufname": "p/p_test.go",
"col": 7,
"lnum": 6,
"module": "",
"nr": 0,
"pattern": "",
"text": "cannot convert 5 (untyped int constant) to string",
"type": "",
"valid": 1,
"vcol": 0
},
{
"bufname": "p/x_test.go",
"col": 9,
"lnum": 10,
"module": "",
"nr": 0,
"pattern": "",
"text": "cannot convert 5 (untyped int constant) to string",
"type": "",
"valid": 1,
"vcol": 0
}
]The |
|
@myitcv: Is there a way to run these tests with the "verboseOutput" setting on? I'd need to see what the output of the |
|
Does the following give you what you need? |
|
Thanks, yes, these are helpful. Is there a Also, worth noting are these Do they appear in passing logs? I'll investigate this further when I get a chance to reproduce this locally. |
|
Yes, there is a Here is a log from a passing test: pass.log Those But just to point out, that a |
|
Jotting down some notes as I investigate this: The passing case manages to get all 3 packages for the Whereas the failing case only loads 2: This looks like a case of go/packages overlay flakiness. |
stamblerre
changed the title
|
After our CLs today, neither @heschik nor I were able to reproduce this locally, and we didn't see it happen in our CI. If it's still happening in your CI, could you share the logs (for both pass and fail) with |
stamblerre
removed
the
NeedsInvestigation
stamblerre
added
the
WaitingForInfo
stamblerre
changed the title
The test is currently skipped so you won't see it fail in your CI:
Pass: gopls.log Fail: gopls.log Note that to try our this CL I'm using the following replaces: |
stamblerre
removed
the
WaitingForInfo
|
Thanks for the update. It's a similar situation as before, but now we're at least getting closer to the correct number of packages. Passing gets 4 packages for this query (as expected): Whereas failing only gets 3: The missing package is always |
|
Change https://golang.org/cl/216717 mentions this issue: |
The core of processGolistOverlay was a map iteration, which is nondeterministic. When creating both a non-test and test package, we would sometimes encounter the test file before the non-test file. In that case, for reasons I didn't bother tracking down, we would never create the non-test package. Rather than doing complicated bookkeeping to fix the problem, simply process non-test before test, and make the loop deterministic to save all of our sanity. Updates golang/go#36661. Change-Id: I1f76869fa52794ac8ae96e22ad06a2b1e1861995 Reviewed-on: https://go-review.googlesource.com/c/tools/+/216717 Reviewed-by: Rebecca Stambler <rstambler@golang.org> Run-TryBot: Heschi Kreinick <heschi@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>
|
I believe that the above CL should've resolved this as well. Closing. |
What version of Go are you using (
go version)?Note that per govim/govim@c7a6cc3,
github.com/myitcvforks/tools/gopls@v0.0.0-20200119092928-0fd5434cd1bais 0cba7a3 with a partial revert of CL214586 on top (the result of which can be seen in https://go-review.googlesource.com/c/tools/+/215239)The issue described below also exists when tested against 0cba7a3.
Does this issue reproduce with the latest release?
Yes.
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
We have just added a new
govimtest that seeks to verify diagnostics are as expected for new files that do not, initially, exist on disk.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_testscript_new_files_quickfix/cmd/govim/testdata/scenario_default/quickfix_new_files.txt
What did you expect to see?
main.go,p/p_test.go, andp/x_test.goonce the files have been created.DoIt.What did you see instead?
Marking as
v0.3.0because this is a fairly fundamental test case.cc @stamblerre
FYI @leitzler
The text was updated successfully, but these errors were encountered: