search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.19k stars 1.56k forks source link

Add integration tests to ensure that error context data is being sent to clients (both LSP and legacy protocol) #44907

Open stereotype441 opened 3 years ago

stereotype441 commented 3 years ago

(Parent issue #44897)

As of a42244f73b27256eca44def4daa552e91c2dbc55, the analyzer now generates context messages in some circumstances explaining why type promotion failed. We should ensure that we have adequate integration tests to verify that these context messages are being delivered to analysis server clients. We should test both the LSP protocol (used by VSCode) and the legacy protocol (used by IntelliJ).

Related: #44901, #44902.

stereotype441 commented 3 years ago

I'm not sure who to assign this to--perhaps @bwilkerson?

stereotype441 commented 3 years ago

@devoncarew do you think anyone will have a chance to work on this before the branch cut?

devoncarew commented 3 years ago

Do we not already have coverage here? This isn't about testing for the presence of the messages in general - that's covered by https://github.com/dart-lang/sdk/issues/45068 and the test.py work (https://github.com/dart-lang/sdk/issues/44905).

This is just to make sure that we're populating the information in the protocol messages to IDEs? @bwilkerson / @DanTup - is this already covered by existing tests?

DanTup commented 3 years ago

There's a test in LSP here:

https://github.com/dart-lang/sdk/blob/3ce196935ee16081cf4710436ad50aaae2b4adb8/pkg/analysis_server/test/lsp/diagnostic_test.dart#L129-L144

It's not currently testing the specific text/location (it may be worth extending to do that), though it is ensuring there is a relatedInformation (which is where the context message shows up if I understand correctly).

bwilkerson commented 3 years ago

I don't think we have an "integration" test for the legacy protocol (where "integration" here means end-to-end). We do have tests to ensure that we correctly convert analysis errors from the analyzer into the protocol object representing analysis errors (https://github.com/dart-lang/sdk/blob/master/pkg/analyzer_plugin/test/utilities/analyzer_converter_test.dart#L98).

stereotype441 commented 3 years ago

@bwilkerson I'm not sure what you mean by "end-to-end", but we do have several tests in pkg/analysis_server/test/integration/ that call themselves integration tests; those are the kinds of tests I was thinking of.

I think it would be an adequate test to make a test similar to pkg/analysis_server/test/integration/analysis/error_test.dart, but where the error being tested has context information, so that we can make sure that the context information is included in the information the analysis server sends back to its client.

bwilkerson commented 3 years ago

... tests in pkg/analysis_server/test/integration/ that call themselves integration tests ...

Yep, those are what I was talking about as well.

DanTup commented 3 years ago

I missed the "integration" part. I've opened https://dart-review.googlesource.com/c/sdk/+/193800/ that adds an LSP integration test that checks the contexts come through (unlike the test I noted above, these spawn the server in another process and then communicate with it exactly as a client would - although the in-process tests for the LSP server exercise essentially the same stack including the full protocol classes, just without the process boundary and using an in-memory filesystem).