[LSP] Small cleanup for pull diagnostics logging (#61417)

* Small logging cleanup on pull diagnostics code * Update tests to account for lsp diagnostics throwing when mismatch in diagnostic mode
dotnet · May 20, 2022 · 92ebb62 · 92ebb62
1 parent 758eeed
commit 92ebb62
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 37 deletions.
diff --git a/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs b/src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs
@@ -109,6 +109,11 @@ protected AbstractPullDiagnosticHandler(
         {
             context.TraceInformation($"{this.GetType()} started getting diagnostics");
 
+            var diagnosticMode = GetDiagnosticMode(context);
+            // For this handler to be called, we must have already checked the diagnostic mode
+            // and set the appropriate capabilities.
+            Contract.ThrowIfFalse(diagnosticMode == DiagnosticMode.Pull, $"{diagnosticMode} is not pull");
+
             // The progress object we will stream reports to.
             using var progress = BufferedProgress.Create(diagnosticsParams.PartialResultToken);
 
@@ -133,11 +138,6 @@ protected AbstractPullDiagnosticHandler(
 
             foreach (var document in orderedDocuments)
             {
-                context.TraceInformation($"Processing: {document.FilePath}");
-
-                // not be asked for workspace docs in razor.
-                // not send razor docs in workspace docs for c#
-
                 var encVersion = _editAndContinueDiagnosticUpdateSource.Version;
 
                 var project = document.Project;
@@ -149,8 +149,7 @@ protected AbstractPullDiagnosticHandler(
                     cancellationToken).ConfigureAwait(false);
                 if (newResultId != null)
                 {
-                    context.TraceInformation($"Diagnostics were changed for document: {document.FilePath}");
-                    progress.Report(await ComputeAndReportCurrentDiagnosticsAsync(context, document, newResultId, context.ClientCapabilities, cancellationToken).ConfigureAwait(false));
+                    progress.Report(await ComputeAndReportCurrentDiagnosticsAsync(context, document, newResultId, context.ClientCapabilities, diagnosticMode, cancellationToken).ConfigureAwait(false));
                 }
                 else
                 {
@@ -189,12 +188,7 @@ private static Dictionary<Document, PreviousPullResult> GetDocumentToPreviousDia
             return result;
         }
 
-        private async Task<TReport> ComputeAndReportCurrentDiagnosticsAsync(
-            RequestContext context,
-            Document document,
-            string resultId,
-            ClientCapabilities clientCapabilities,
-            CancellationToken cancellationToken)
+        private DiagnosticMode GetDiagnosticMode(RequestContext context)
         {
             var diagnosticModeOption = context.ServerKind switch
             {
@@ -204,21 +198,24 @@ private async Task<TReport> ComputeAndReportCurrentDiagnosticsAsync(
             };
 
             var diagnosticMode = _globalOptions.GetDiagnosticMode(diagnosticModeOption);
-            var isPull = diagnosticMode == DiagnosticMode.Pull;
-
-            context.TraceInformation($"Getting '{(isPull ? "pull" : "push")}' diagnostics with mode '{diagnosticMode}'");
+            return diagnosticMode;
+        }
 
+        private async Task<TReport> ComputeAndReportCurrentDiagnosticsAsync(
+            RequestContext context,
+            Document document,
+            string resultId,
+            ClientCapabilities clientCapabilities,
+            DiagnosticMode diagnosticMode,
+            CancellationToken cancellationToken)
+        {
             using var _ = ArrayBuilder<LSP.Diagnostic>.GetInstance(out var result);
+            var text = await document.GetTextAsync(cancellationToken).ConfigureAwait(false);
+            var diagnostics = await GetDiagnosticsAsync(context, document, diagnosticMode, cancellationToken).ConfigureAwait(false);
+            context.TraceInformation($"Found {diagnostics.Length} diagnostics for {document.FilePath}");
 
-            if (isPull)
-            {
-                var text = await document.GetTextAsync(cancellationToken).ConfigureAwait(false);
-                var diagnostics = await GetDiagnosticsAsync(context, document, diagnosticMode, cancellationToken).ConfigureAwait(false);
-                context.TraceInformation($"Got {diagnostics.Length} diagnostics");
-
-                foreach (var diagnostic in diagnostics)
-                    result.Add(ConvertDiagnostic(document, text, diagnostic, clientCapabilities));
-            }
+            foreach (var diagnostic in diagnostics)
+                result.Add(ConvertDiagnostic(document, text, diagnostic, clientCapabilities));
 
             return CreateReport(ProtocolConversions.DocumentToTextDocumentIdentifier(document), result.ToArray(), resultId);
         }

diff --git a/src/Features/LanguageServer/Protocol/Handler/Diagnostics/DocumentPullDiagnosticHandler.cs b/src/Features/LanguageServer/Protocol/Handler/Diagnostics/DocumentPullDiagnosticHandler.cs
@@ -100,7 +100,7 @@ internal static ImmutableArray<Document> GetRequestedDocument(RequestContext con
 
             if (!context.IsTracking(context.Document.GetURI()))
             {
-                context.TraceInformation($"Ignoring diagnostics request for untracked document: {context.Document.GetURI()}");
+                context.TraceWarning($"Ignoring diagnostics request for untracked document: {context.Document.GetURI()}");
                 return ImmutableArray<Document>.Empty;
             }
 

diff --git a/src/Features/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs b/src/Features/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs
@@ -83,9 +83,7 @@ public async Task TestNoDocumentDiagnosticsForOpenFilesWithFSAOffIfInPushMode(bo
 
             await OpenDocumentAsync(testLspServer, document);
 
-            var results = await RunGetDocumentPullDiagnosticsAsync(testLspServer, document.GetURI(), useVSDiagnostics);
-
-            Assert.Empty(results.Single().Diagnostics);
+            await Assert.ThrowsAsync<StreamJsonRpc.RemoteInvocationException>(async () => await RunGetDocumentPullDiagnosticsAsync(testLspServer, document.GetURI(), useVSDiagnostics));
         }
 
         [Theory, CombinatorialData]
@@ -103,8 +101,7 @@ public async Task TestNoDocumentDiagnosticsForOpenFilesIfDefaultAndFeatureFlagOf
             // Ensure we get no diagnostics when feature flag is off.
             testLspServer.TestWorkspace.GlobalOptions.SetGlobalOption(new OptionKey(DiagnosticOptions.LspPullDiagnosticsFeatureFlag), false);
 
-            var results = await RunGetDocumentPullDiagnosticsAsync(testLspServer, document.GetURI(), useVSDiagnostics);
-            Assert.Empty(results.Single().Diagnostics);
+            await Assert.ThrowsAsync<StreamJsonRpc.RemoteInvocationException>(async () => await RunGetDocumentPullDiagnosticsAsync(testLspServer, document.GetURI(), useVSDiagnostics));
         }
 
         [Theory, CombinatorialData]
@@ -586,7 +583,7 @@ public async Task TestNoWorkspaceDiagnosticsForClosedFilesWithFSAOffWithFileInPr
 @"class A {";
             var markup2 = "";
             using var testLspServer = await CreateTestWorkspaceWithDiagnosticsAsync(
-                new[] { markup1, markup2 }, BackgroundAnalysisScope.OpenFiles, useVSDiagnostics, pullDiagnostics: false);
+                new[] { markup1, markup2 }, BackgroundAnalysisScope.OpenFiles, useVSDiagnostics, pullDiagnostics: true);
 
             var firstDocument = testLspServer.GetCurrentSolution().Projects.Single().Documents.First();
             await OpenDocumentAsync(testLspServer, firstDocument);
@@ -644,11 +641,7 @@ public async Task TestNoWorkspaceDiagnosticsForClosedFilesWithFSAOnAndInPushMode
             using var testLspServer = await CreateTestWorkspaceWithDiagnosticsAsync(
                 new[] { markup1, markup2 }, BackgroundAnalysisScope.FullSolution, useVSDiagnostics, pullDiagnostics: false);
 
-            var results = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics);
-
-            Assert.Equal(2, results.Length);
-            Assert.Empty(results[0].Diagnostics);
-            Assert.Empty(results[1].Diagnostics);
+            await Assert.ThrowsAsync<StreamJsonRpc.RemoteInvocationException>(async () => await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics));
         }
 
         [Theory, CombinatorialData]

diff --git a/src/Features/LanguageServer/ProtocolUnitTests/VSTypeScriptHandlerTests.cs b/src/Features/LanguageServer/ProtocolUnitTests/VSTypeScriptHandlerTests.cs
@@ -12,12 +12,14 @@
 using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.CodeAnalysis.Diagnostics;
 using Microsoft.CodeAnalysis.Editor.UnitTests.Workspaces;
 using Microsoft.CodeAnalysis.ExternalAccess.VSTypeScript;
 using Microsoft.CodeAnalysis.ExternalAccess.VSTypeScript.Api;
 using Microsoft.CodeAnalysis.Host.Mef;
 using Microsoft.CodeAnalysis.Options;
 using Microsoft.CodeAnalysis.Shared.TestHooks;
+using Microsoft.CodeAnalysis.SolutionCrawler;
 using Microsoft.CodeAnalysis.Test.Utilities;
 using Microsoft.VisualStudio.LanguageServer.Protocol;
 using Nerdbank.Streams;
@@ -60,6 +62,7 @@ public async Task TestRoslynTypeScriptHandlerInvoked()
 </Workspace>";
 
         using var testLspServer = await CreateTsTestLspServerAsync(workspaceXml);
+        testLspServer.TestWorkspace.GlobalOptions.SetGlobalOption(new OptionKey(InternalDiagnosticsOptions.NormalDiagnosticMode), DiagnosticMode.Pull);
 
         var document = testLspServer.GetCurrentSolution().Projects.Single().Documents.Single();
         var documentPullRequest = new VSInternalDocumentDiagnosticsParams