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.

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

x/tools/gopls: add a custom command to ask the server if diagnostics are all sent #36518

Closed
stamblerre opened this issue Jan 12, 2020 · 6 comments
Closed
Assignees
Milestone

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jan 12, 2020

Right now, there is no way for the client to tell if all possible diagnostics have been sent for a given file. This is fine for normal editing, but it makes tests difficult to write. We thought that having a policy of always sending diagnostics on textDocument/didOpen for a file would be sufficient, but it's hard to justify that approach other than as an invariant for testing. It would be simpler to give tests a special method for detecting this rather than sending unnecessary empty diagnostics.

/cc @myitcv

@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Jan 13, 2020

FWIW some background from the govim side on how we deal with this problem.

We split the govim integration into two "buckets":

  1. regular integration tests where we assume gopls is being well behaved, i.e. that we receive diagnostics when we should and don't when we shouldn't. We block on govim events to know when a test can proceed, e.g. https://github.com/govim/govim/blob/29461d37de75d59c26e20a4216110ad4461bad69/cmd/govim/testdata/scenario_default/quickfix.txt#L7 is used to wait until the quickfix window is populated with errors (the quickfix window is where govim lists diagnostics errors). Then we assert based on the content of the quickfix window to ensure the user is seeing what we expect
  2. tests that specifically assert behaviour of diagnostics

I think the problem you are describing is covered by the second bucket:

https://github.com/govim/govim/tree/29461d37de75d59c26e20a4216110ad4461bad69/cmd/govim/testdata/scenario_diagnostic_assertions

The second bucket of tests is not complete, but currently we assert that:

  • initial diagnostics will send diagnostics for all files in the workspace that contain non-analysis errors and nothing for other files in the workspace
  • on opening any file we get (possibly empty) diagnostics for that file
  • on opening any of a package's files for the first time, diagnostics for all files in the package containing analysis errors will be augmented to existing non-analysis diagnostics and sent. For the file that has been opened, a single combined notification will be sent
  • when making changes to a file, diagnostics will only be resent if the diagnostics for that file change
  • when making changes to a file (file1), diagnostics for another file (file2) will only be resent if the diagnostics for that file (file2) change
  • ...

The tricky part in all of this is asserting that we have not received a diagnostic in a given situation. For that we use a sleep (configurable, but it is a generous 30s on CI) followed by a check that the count of diagnostics notifications (since the last check) is zero:

https://github.com/govim/govim/blob/29461d37de75d59c26e20a4216110ad4461bad69/cmd/govim/testdata/scenario_diagnostic_assertions/multiple_files_no_errors.txt#L7-L8

We skip these tests if go test is run with -short.

Whilst not yet complete, I'm fairly comfortable that we have the machinery required to get full coverage via this second bucket. Yes the sleep approach is not ideal, but it at least has the benefit of not requiring further changes to gopls. We are ultimately sure that we end up in the "right" state regardless of whether any diagnostic calculations might have been cancelled, and hence not sent for a given version for example.

Indeed any method of the sort you propose would presumably need to block on any in-flight diagnostics calculations before returning?

All that said we'd be very open to improving our approach and making our tests more reliable.

We thought that having a policy of always sending diagnostics on textDocument/didOpen for a file would be sufficient

Can you explain a bit more about how this is related? Much like our approach in bucket 1 above, if you assume that gopls is behaving well then really there should be no need to wait for diagnostics to be received for a file when you open it. The "happens before" guarantee should be enough.

It would be simpler to give tests a special method for detecting this rather than sending unnecessary empty diagnostics

If actually you were suggesting above that we should drop the re-sending of diagnostics on file open then I agree, for the reasons I explained.

But then also for the reasons I explained I'm not sure we need a method for our tests to tell if all possible diagnostics have been sent for a given file. It might help clean up/simplify the tests a bit but I'm not sure it's worth the effort at this point if we can get coverage by other means. But I obviously say that with far less experience of the gopls code base, only experience from govim and hence defer to you on the former 😄

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 13, 2020

Change https://golang.org/cl/214583 mentions this issue: internal/lsp: add and use nonstandard gopls/diagnoseFiles

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Jan 13, 2020

The CL above is primarily to make the command line client sane in the case where a file doesn't have diagnostics published for whatever reason. I don't really have an opinion on how tests should work in general, I'll let you two discuss that :)

@findleyr

This comment has been minimized.

Copy link

@findleyr findleyr commented Jan 14, 2020

Can you explain a bit more about how this is related? Much like our approach in bucket 1 above, if you assume that gopls is behaving well then really there should be no need to wait for diagnostics to be received for a file when you open it

For govim tests I agree that this command is probably unnecessary. As you said, govim is typically asserting on the receipt (or non-receipt) of diagnostics. In most of these cases, the govim tests shouldn't really care about the exact content of these diagnostics.

On the other hand, this is useful for the type of tests @heschik was looking at, which are testing the specific contents of the diagnostics.

@findleyr

This comment has been minimized.

Copy link

@findleyr findleyr commented Jan 15, 2020

Eh, I think what I said above is a bit of a mischaracterization. Both govim and gopls have at least one common concern with respect to diagnostics: knowing whether all diagnostics have been received. IIUC both are currently using a timeout to achieve this.

Indeed any method of the sort you propose would presumably need to block on any in-flight diagnostics calculations before returning?

This mechanism already exists (diagnostics are ~idempotent), so what @heschik did was pretty simple: https://golang.org/cl/214583. Seems like that's a relatively small cost for the benefit of not having to rely on timeouts.

I'll defer to @heschik regarding whether this should be used by govim, or whether we prefer this remain an internal API.

@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Jan 16, 2020

Quick update here based on my comment above (no need to re-open).

When govim/govim#678 lands we will move almost entirely away from blocking based on diagnostic messages from gopls.

Instead, per the commit message, we will use vimexprwait:

vimexprwait repeatedly evaluates a Vim expression (where the expression
is like that passed to the vim expr command) until the result matches
the contents of a golden file.

    vimexprwait golden.file expr

e.g.

    vimexprwait errors.golden getqflist()

will wait until the quickfix list contents match the contents of
errors.golden.

By default vimexprwait JSON indent-formats the result
(this can be turned off with -noindent).

By default vimexprwait waits (with exponential backoff) for up to
testdriver.DefaultErrLogMatchWait.

As part of this change we also simplify all tests that previously used
an approach that was a mix of errlogmatch, vim expr, copen etc. The
semantics of the changed tests have not changed, only the way in which
we assert.

For file watcher-based tests, we move away from waiting for diagnostics
for a file spotted by the watcher to instead specifically looking for
didOpen/didChange events being sent to gopls. Given the happens-before
relationship that didOpen/didChange affords us, again this doesn't
result in any change in semantics.

Per @findleyr's comment above, this is much better suited (thanks for the suggestion, @findleyr!) to govim/Vim, because we are instead asserting based on what the user can see, and waiting until Vim gets to a given state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.