x/tools/internal/lsp/regtest: tracking various regtest improvements

findleyr commented 4 years ago

This is a follow-up to #36879. We've been using the gopls regtests for a few months now, and several usability bugs / missing features / overall themes have emerged. None of these seemed worthy of an issue, but I want to track them in aggregate. I've been trying to let the regtests soak in their current form, but plan to take another pass soon.

Here's a list of what I've noted along the way, either from my own usage or feedback from others'. Feel free to comment with more, and I'll edit this comment to keep it up to date.

[ ] Once @ianthehat's jsonrpc2 shutdown fixes are merged, we should t.Parallel the regtests once more.
[x] Add more documentation about how to develop new regtests (tricks like using -regtest_timeout to avoid waiting for the default regtest timeout).
[x] Having the run options after the test body in runner.Run is hard to spot. Use a different options pattern to allow passing them before the test body.
[x] Fix the 'NoOutstandingWork' expectation, by making sure that 'progress finished' messages never fail to be sent due to a cancelled context.
[x] Somehow fix regtest cleanup on Windows (which runs into problems with file locking from the go command). Perhaps awaiting 'NoOutstandingWork' could help here.
[x] Move regtests into the gopls module, so that they can run staticcheck.
[ ] Once regtests are in the gopls module, replace the synthetic file watching with actual file watching, using fsnotify. The fact that file-watching is faked has been a source of bugs. The file watching should also respect the GlobPattern provided in the file watching capability registration.
[ ] Make the DiagnosticAtRegexp expectation lazy, so that it doesn't eagerly evaluate the regexp position (and therefore no longer needs the Env receiver).
[ ] Make DiagnosticAtRegexp match the full range of the regexp match, not just the starting position (this was a cause of some confusion in a CL).
[x] ~Return a copy of State from Await: whatever caused expectations to be be met~ (the underlying requirement was instead achieved with less effort by adding a 'ReadDiagnostics' expectation that atomically reads from State).
[x] Explicitly set GOPACKAGESDRIVER=off for all regtests.
[x] Use a top-level TMPDIR for the entire regtest suite, rather than letting each regtest create it's own sandbox in /tmp. This reduces cruft in /tmp when tests are ctrl-c'ed before they can clean themselves up.
[x] Add continuous integration to run the regtests against a true daemon, not an in-process daemon. This is not done currently because it will be problematic on Windows, and would increase the test duration.
[x] Fix handling of multi-byte runes in source text.
[x] Remove the need to count didChange/didOpen requests when writing expectations.

CC @pjweinbgo @stamblerre @heschik

gopherbot commented 4 years ago

Change https://golang.org/cl/240059 mentions this issue: internal/lsp/regtest: use a common directory regtests sandboxes

gopherbot commented 4 years ago

Change https://golang.org/cl/240058 mentions this issue: internal/lsp/fake: explicitly set GOPACKAGESDRIVER=off in regtests

gopherbot commented 4 years ago

Change https://golang.org/cl/241739 mentions this issue: internal/lsp/regtest: refactor to use a different options pattern

gopherbot commented 4 years ago

Change https://golang.org/cl/243057 mentions this issue: internal/lsp/regtest: simpler way to invert options

gopherbot commented 4 years ago

Change https://golang.org/cl/252683 mentions this issue: integration/regtest: use gopls hooks and add a test for staticcheck

gopherbot commented 4 years ago

Change https://golang.org/cl/252682 mentions this issue: gopls/integration/regtest: move regtests to the gopls module

gopherbot commented 4 years ago

Change https://golang.org/cl/255126 mentions this issue: gopls/internal/regtest: simplify expectation return values

gopherbot commented 3 years ago

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

gopherbot commented 3 years ago

Change https://golang.org/cl/287572 mentions this issue: gopls/internal/regtest: split regtests up into multiple packages

gopherbot commented 2 years ago

Change https://go.dev/cl/416876 mentions this issue: internal/lsp/regtest: simplify, consolidate, and document settings

gopherbot commented 2 years ago

Change https://go.dev/cl/417587 mentions this issue: internal/lsp/regtest: allow sharing memoized results across regtests

gopherbot commented 2 years ago

Change https://go.dev/cl/418774 mentions this issue: internal/lsp/regtest: only run /singleton tests with -short