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

Add tests for the cohost linked editing range endpoint #10596

Merged
merged 8 commits into from
Jul 10, 2024

Conversation

davidwengier
Copy link
Contributor

@davidwengier davidwengier commented Jul 9, 2024

Part of #9519 and #10603

Finally some cohosting tests! Some people would say that I'm being lazy in adding tests for the simplest endpoint we have. Those people have a point.

The test infra here almost entirely avoids ServiceHub, and certainly avoids the Roslyn solution sync mechanism, but it does use our real services and service factories, including the OOP services' separate MEF composition, so the services themselves are partying on a real Roslyn (test) solution and are using real implementations of all of their dependencies.

Things not covered by this test infra yet:

  • OOP initialization
  • Adding generated C# files to the Roslyn solution (IDynamicFile does this in real life)
  • Dealing with Html documents in any way

Future PRs to add tests for more endpoints should add these as needed.

@davidwengier davidwengier requested a review from a team as a code owner July 9, 2024 08:21
/// <summary>
/// An implementation of IServiceProvider that only provides a TraceSource, that writes to test output
/// </summary>
internal class TestTraceSourceProvider(ITestOutputHelper testOutputHelper) : IServiceProvider
Copy link
Member

Choose a reason for hiding this comment

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

It's not worth changing anything, but I wanted to mention that there's a handy mock helper for creating IServiceProvider: VsMocks.CreateServiceProvider(...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When its this easy to do, I'd rather avoid a mock 😛

@@ -24,6 +24,7 @@
</ItemGroup>

<ItemGroup>
<ProjectReference Include="..\..\src\Microsoft.CodeAnalysis.Remote.Razor\Microsoft.CodeAnalysis.Remote.Razor.csproj" />
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we'll be breaking out another test library later, since MS.CA.Remote.Razor should never be loaded in Visual Studio.

Copy link
Contributor Author

@davidwengier davidwengier Jul 10, 2024

Choose a reason for hiding this comment

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

I considered doing this as a separate project now, but it didn't seem to add much value. Certainly in future we'll need to as the cohosting bits will have to move somewhere other than the VS layer, so they can work in VS Code. Once that happens we'll definitely need something new, we just don't know what it looks like yet.

@davidwengier
Copy link
Contributor Author

Have responded to the feedback here, thanks very much. Going to merge this as is for now, just to keep the code flowing. Lots more to come though, outlined in #10603, so I suspect some of this infra will change soon enough.

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.

3 participants