Show TODO Razor comments in the Task List in VS #11540
Conversation
# Conflicts: # src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/LanguageClient/Cohost/CohostDocumentPullDiagnosticsEndpoint.cs # src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.Test/Cohost/CohostDocumentPullDiagnosticsTest.cs
|
Test insertion to make sure I don't annoy RPS: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/614387 |
DustinCampbell
left a comment
DustinCampbell
left a comment
There was a problem hiding this comment.
Generally looks good! A couple of points:
- Consider whether you want to add a new integration test. I think this feature probably warrants one to verify that the TODO comments actually show up in VS Task List.
- It looks like the test insertion uncovered a regression in non-devdiv methods JIT'd. I'm not sure how that could happen from this PR, but it looks like the regression was in all five iterations of the run.
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Hosting/RazorLSPOptions.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Hosting/RazorLSPOptions.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Diagnostics/TaskListDiagnosticProvider.cs
Outdated
Show resolved
Hide resolved
...r/src/Microsoft.VisualStudio.LanguageServices.Razor/LanguageClient/Options/OptionsStorage.cs
Outdated
Show resolved
Hide resolved
...r/src/Microsoft.VisualStudio.LanguageServices.Razor/LanguageClient/Options/OptionsStorage.cs
Outdated
Show resolved
Hide resolved
...soft.AspNetCore.Razor.LanguageServer.Test/Diagnostics/DocumentPullDiagnosticsEndpointTest.cs
Outdated
Show resolved
Hide resolved
src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/RazorLSPOptionsMonitorTest.cs
Outdated
Show resolved
Hide resolved
...est/Microsoft.VisualStudio.LanguageServices.Razor.Test/Settings/ClientSettingsManagerTest.cs
Outdated
Show resolved
Hide resolved
|
The RPS and Speedometer regressions are also present in our main branch insertions and my System.Text.JSON test insertions, so whatever they are, I'm confident they're not caused by this change. |
Awesome! Adding my approval then, though do consider whether an integration test for the end-to-end would be valuable. |
| Assert.NotNull(serverCapabilities.DiagnosticProvider); | ||
| Assert.NotNull(serverCapabilities.DiagnosticProvider.DiagnosticKinds); | ||
| Assert.Single(serverCapabilities.DiagnosticProvider.DiagnosticKinds); | ||
| Assert.Equal(2, serverCapabilities.DiagnosticProvider.DiagnosticKinds.Length); |
There was a problem hiding this comment.
FWIW, I don't think this assert is strictly necessary. I'm pretty sure that Assert.Collection will fail if the length doesn't match the number of item assertions.
3 participants
Fixes #4446