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 😄

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

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 :)

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.

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.

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.