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

Adding client capabilities to OOP client initialization data #11129

Merged
merged 12 commits into from
Nov 1, 2024

Conversation

alexgav
Copy link
Contributor

@alexgav alexgav commented Oct 31, 2024

Summary of the changes

  • Pass client capabilities to OOP just once, during the initial client collection
  • Proffer client capabilities to other remote services via RemoteClientCapabitiliesService
  • Move common code for the cohost and remote ClientCapabilitiesService to a base class in Workspaces layer
  • Fixup tests to init necessary capabilities

Fixes: #11102

The service gets initialized once during initial connection / init and provides client capabilities to other remote services.
- Adding client capabilities to RemoteClientLSPInitializationOptions
- Converting IRemoteClientIntializationService to be a JSON service for simplicity of data serialization
- Converting client initialization code to use JSON client to call IRemoteClientIntializationService
@alexgav alexgav requested a review from a team as a code owner October 31, 2024 17:25
Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this! I'm really glad to see it happening. I have a few suggestions, but it looks like there are test failures. (Note that you can always check for test failures by running build.cmd -test at the command-line before submitting.)

@alexgav
Copy link
Contributor Author

alexgav commented Oct 31, 2024

but it looks like there are test failures.

thanks, I ran and fixed cohosting tests, missed that one. just submitted a fix.

(Note that you can always check for test failures by running build.cmd -test at the command-line before submitting.)

thanks, didn't know about that parameter

@davidwengier
Copy link
Contributor

Does that do something different to just running tests in Test Explorer??

Only consumers that are initializing capabiilities service by calling SetCapabilities should be importing it via RemoteClientCapabilitiesService. All other consumers should be using IClientCapabilitiesService.
Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely! Just a few minor tweaks to make everything really nice and neat.

Also at some point we need to be a bit smarter about the LSP initialization, but I'll log a separate issue for it.

@DustinCampbell
Copy link
Member

Does that do something different to just running tests in Test Explorer??

Technically, yes; practically, no. If something throws on a thread after a thread has finished running, it'll fail at the command-line, but Test Explorer will eat the exceptions. That's pretty uncommon though, so practically, there's no real difference. 😄

Personally, I like to run them at the command-line so they run just as they do in CI. That gives me some confidence that any test failures in CI for Window-Debug or Windows-Release aren't mine. Plus, I get to see if there any warnings (such as unused using) that would break the CI build before pushing. Also, I always trip over the fact that our integration tests our included in the Test Explorer. Too often when making a compiler change, I'll forget and Ctrl+A to select all tests and accidentally start integration tests running. build.cmd -test just runs unit tests.

@davidwengier
Copy link
Contributor

Also, I always trip over the fact that our integration tests our included in the Test Explorer. Too often when making a compiler change, I'll forget and Ctrl+A to select all tests and accidentally start integration tests running. build.cmd -test just runs unit tests.

May I recommend a "RazorToolingTests.playlist" playlist file, that includes everything but compiler and integration tests, so one click of the "Run All In View" button runs only the interesting things.

@DustinCampbell
Copy link
Member

Also, I always trip over the fact that our integration tests our included in the Test Explorer. Too often when making a compiler change, I'll forget and Ctrl+A to select all tests and accidentally start integration tests running. build.cmd -test just runs unit tests.

May I recommend a "RazorToolingTests.playlist" playlist file, that includes everything but compiler and integration tests, so one click of the "Run All In View" button runs only the interesting things.

I do know about that, but I did say "too often when making a compiler change." 😄

- Renaming a variable
- Made UpdateClientLSPInitializationOptions mode complete (so it updates RemoteSemanticTokensLegendService now with initialization data)
- Removed limitation of single update on RemoteSemanticTokensLegendService
- Use UpdateClientLSPInitializationOptions  in cohost semantic tokens test
@alexgav alexgav enabled auto-merge (squash) November 1, 2024 00:17
@alexgav alexgav merged commit 6ff6566 into main Nov 1, 2024
12 checks passed
@alexgav alexgav deleted the dev/alexgav/RemoteClientCapabilities branch November 1, 2024 00:38
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass VSInternalClientCapabilies (or needed parts) in RemoteClientLSPInitializationOptions
4 participants