Skip to content

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

Closed
@findleyr

Description

@findleyr

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.
  • Add more documentation about how to develop new regtests (tricks like using -regtest_timeout to avoid waiting for the default regtest timeout).
  • 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.
  • Fix the 'NoOutstandingWork' expectation, by making sure that 'progress finished' messages never fail to be sent due to a cancelled context.
  • Somehow fix regtest cleanup on Windows (which runs into problems with file locking from the go command). Perhaps awaiting 'NoOutstandingWork' could help here.
  • 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).
  • 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).
  • Explicitly set GOPACKAGESDRIVER=off for all regtests.
  • 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.
  • 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.
  • Fix handling of multi-byte runes in source text.
  • Remove the need to count didChange/didOpen requests when writing expectations.

CC @pjweinbgo @stamblerre @heschik

Metadata

Metadata

Assignees

No one assigned

    Labels

    FrozenDueToAgeTestingAn issue that has been verified to require only test changes, not just a test failure.ToolsThis label describes issues relating to any tools in the x/tools repository.goplsIssues related to the Go language server, gopls.

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions