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

SLVS-1679 Enable issue streaming for SLCore analysis #5876

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/CFamily.UnitTests/Subprocess/MessageHandlerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ public void HandleMessage_IssuesForAnalyzedFileAreNotIgnored(string fileNameInMe
context.IssueConverter.Invocations.Count.Should().Be(1);
context.IssueConsumer.Invocations.Count.Should().Be(1);

context.IssueConsumer.Verify(x => x.Accept(analyzedFile, It.IsAny<IEnumerable<IAnalysisIssue>>()));
context.IssueConsumer.Verify(x => x.Set(analyzedFile, It.IsAny<IEnumerable<IAnalysisIssue>>()));
}

[TestMethod]
Expand Down
7 changes: 4 additions & 3 deletions src/CFamily/Subprocess/MessageHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ public void HandleMessage(Message message)

// Rules that start with internal shouldn't be treated as issues.
// Some of them should be handled like `internal.fileDependency`. See: https://github.com/SonarSource/sonarlint-visualstudio/issues/2611
// Others should can simply ignored like `internal.z3RefutationRate`, which is used to log in the scanner how many issues are rejected by the Z3 solver
// Others should can simply ignored like `internal.z3RefutationRate`, which is used to log in the scanner how many issues are rejected by the Z3 solver
case string s when s.StartsWith("internal."):
break;

Expand Down Expand Up @@ -137,11 +137,12 @@ private void HandleAnalysisIssue(Message message)
IssueCount++;
var issue = issueConverter.Convert(message, request.Context.CFamilyLanguage, request.Context.RulesConfiguration);

// Note: the file being analyzed might have been closed by the time the analysis results are
// Note: the file being analyzed might have been closed by the time the analysis results are
// returned. This doesn't cause a crash; all active taggers will have been detached from the
// TextBufferIssueTracker when the file was closed, but the TextBufferIssueTracker will
// still exist and handle the call.
issueConsumer.Accept(request.Context.File, new[] { issue });
// todo https://sonarsource.atlassian.net/browse/SLVS-1661

Choose a reason for hiding this comment

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

Please remove the TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? This links to the cleanup task

Choose a reason for hiding this comment

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

You added it after you commented out the line 145 in this commit: 30e2dd5

Then you added back the line, so I assumed the TODO also has to be removed.

If that is not the case, then consider explaining what has to be done in the line and update the task SLVS-1661 to make sure the TODO will be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The whole class needs to be removed, I reverted the line change because I didn't want to comment out the tests

Choose a reason for hiding this comment

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

Then, just update the corresponding Jira task, and everything should be fine. 👍

issueConsumer.Set(request.Context.File, new[] { issue });
}

internal /* for testing */ static bool IsIssueForActiveRule(Message message, ICFamilyRulesConfig rulesConfiguration)
Expand Down
54 changes: 27 additions & 27 deletions src/Core.UnitTests/Analysis/AnalysisServiceTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public void MefCtor_CheckIsSingleton()
{
MefTestHelpers.CheckIsSingletonMefComponent<AnalysisService>();
}

[TestMethod]
public void ScheduleAnalysis_AnalysisScheduler_CachesIssueConsumer_And_RunsAnalyzerController()
{
Expand All @@ -54,15 +54,15 @@ public void ScheduleAnalysis_AnalysisScheduler_CachesIssueConsumer_And_RunsAnaly
var testSubject = CreateTestSubject(analyzerController, issueConsumerStorage, scheduler);

testSubject.ScheduleAnalysis("file/path", analysisId, detectedLanguages, issueConsumer, analyzerOptions);

Received.InOrder(() =>
{
scheduler.Schedule("file/path", Arg.Any<Action<CancellationToken>>(), Arg.Any<int>());
issueConsumerStorage.Set("file/path", analysisId, issueConsumer);
analyzerController.ExecuteAnalysis("file/path", analysisId, detectedLanguages, issueConsumer, analyzerOptions, Arg.Any<CancellationToken>());
});
}

[TestMethod]
public void ScheduleAnalysis_JobCancelledBeforeStarting_DoesNotExecute()
{
Expand All @@ -72,7 +72,7 @@ public void ScheduleAnalysis_JobCancelledBeforeStarting_DoesNotExecute()
var testSubject = CreateTestSubject(analyzerController, issueConsumerStorage, scheduler);

testSubject.ScheduleAnalysis("file/path", default, default, default, default);

scheduler.Received().Schedule("file/path", Arg.Any<Action<CancellationToken>>(), Arg.Any<int>());
issueConsumerStorage.DidNotReceiveWithAnyArgs().Set(default, default, default);
analyzerController.DidNotReceiveWithAnyArgs().ExecuteAnalysis(default, default, default, default, default, default);
Expand All @@ -90,26 +90,26 @@ public void ScheduleAnalysis_ProvidesCorrectTimeout(int envSettingsResponse, int
Environment.SetEnvironmentVariable(EnvironmentSettings.AnalysisTimeoutEnvVar, envSettingsResponse.ToString());
var scheduler = Substitute.For<IScheduler>();
var testSubject = CreateTestSubject(scheduler: scheduler);

testSubject.ScheduleAnalysis("file/path", default, default, default, default);

scheduler.Received().Schedule("file/path", Arg.Any<Action<CancellationToken>>(), expectedTimeout);
}
finally
{
Environment.SetEnvironmentVariable(EnvironmentSettings.AnalysisTimeoutEnvVar, null);
}

}

[TestMethod]
public void ScheduleAnalysis_NoEnvironmentSettings_DefaultTimeout()
{
var scheduler = Substitute.For<IScheduler>();
var testSubject = CreateTestSubject(scheduler: scheduler);

testSubject.ScheduleAnalysis("file/path", default, default, default, default);

scheduler.Received().Schedule("file/path", Arg.Any<Action<CancellationToken>>(), AnalysisService.DefaultAnalysisTimeoutMs);
}

Expand All @@ -118,25 +118,25 @@ public void PublishIssues_NoConsumerInStorage_DoesNothing()
{
var issueConsumerStorage = CreateIssueConsumerStorageWithStoredItem(Guid.NewGuid(), null, false);
var testSubject = CreateTestSubject(issueConsumerStorage:issueConsumerStorage);

var act = () => testSubject.PublishIssues("file/path", Guid.NewGuid(), Substitute.For<IEnumerable<IAnalysisIssue>>());

act.Should().NotThrow();
}

[TestMethod]
public void PublishIssues_DifferentAnalysisId_DoesNothing()
{
var analysisId = Guid.NewGuid();
var issueConsumer = Substitute.For<IIssueConsumer>();
var issueConsumerStorage = CreateIssueConsumerStorageWithStoredItem(Guid.NewGuid(), issueConsumer, true);
var testSubject = CreateTestSubject(issueConsumerStorage:issueConsumerStorage);

testSubject.PublishIssues("file/path", analysisId, Substitute.For<IEnumerable<IAnalysisIssue>>());
issueConsumer.DidNotReceiveWithAnyArgs().Accept(default, default);

issueConsumer.DidNotReceiveWithAnyArgs().Set(default, default);
}

[TestMethod]
public void PublishIssues_MatchingConsumer_PublishesIssues()
{
Expand All @@ -147,10 +147,10 @@ public void PublishIssues_MatchingConsumer_PublishesIssues()
var analysisIssues = Substitute.For<IEnumerable<IAnalysisIssue>>();

testSubject.PublishIssues("file/path", analysisId, analysisIssues);
issueConsumer.Received().Accept("file/path", analysisIssues);

issueConsumer.Received().Set("file/path", analysisIssues);
}

[DataRow(true)]
[DataRow(false)]
[DataTestMethod]
Expand All @@ -170,22 +170,22 @@ public void CancelForFile_JobCancelledBeforeStarting_DoesNotExecute()
var issueConsumerStorage = Substitute.For<IIssueConsumerStorage>();
var scheduler = CreateDefaultScheduler(true);
var testSubject = CreateTestSubject(issueConsumerStorage: issueConsumerStorage, scheduler: scheduler);

testSubject.CancelForFile("file/path");

scheduler.Received().Schedule("file/path", Arg.Any<Action<CancellationToken>>(), -1);
issueConsumerStorage.DidNotReceiveWithAnyArgs().Remove(default);
}

[TestMethod]
public void CancelForFile_RunsConsumerStorageClearAsScheduledJob()
{
var issueConsumerStorage = Substitute.For<IIssueConsumerStorage>();
var scheduler = CreateDefaultScheduler();
var testSubject = CreateTestSubject(issueConsumerStorage: issueConsumerStorage, scheduler: scheduler);

testSubject.CancelForFile("file/path");

Received.InOrder(() =>
{
scheduler.Schedule("file/path", Arg.Any<Action<CancellationToken>>(), -1);
Expand All @@ -202,7 +202,7 @@ private static IAnalysisService CreateTestSubject(IAnalyzerController analyzerCo
scheduler ??= Substitute.For<IScheduler>();
return new AnalysisService(analyzerController, issueConsumerStorage, scheduler);
}

private static IIssueConsumerStorage CreateIssueConsumerStorageWithStoredItem(Guid analysisId, IIssueConsumer issueConsumer, bool result)
{
var issueConsumerStorage = Substitute.For<IIssueConsumerStorage>();
Expand All @@ -214,7 +214,7 @@ private static IIssueConsumerStorage CreateIssueConsumerStorageWithStoredItem(Gu
});
return issueConsumerStorage;
}

private static IScheduler CreateDefaultScheduler(bool createCancelled = false)
{
var cancellationTokenSource = new CancellationTokenSource();
Expand Down
4 changes: 2 additions & 2 deletions src/Core/Analysis/AnalysisService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public void PublishIssues(string filePath, Guid analysisId, IEnumerable<IAnalysi
if (issueConsumerStorage.TryGet(filePath, out var currentAnalysisId, out var issueConsumer)
&& analysisId == currentAnalysisId)
{
issueConsumer.Accept(filePath, issues);
issueConsumer.Set(filePath, issues);
}
}

Expand Down Expand Up @@ -84,7 +84,7 @@ public void CancelForFile(string filePath)
},
-1);
}

private static int GetAnalysisTimeoutInMilliseconds()
{
var environmentSettings = new EnvironmentSettings();
Expand Down
2 changes: 1 addition & 1 deletion src/Core/Analysis/IIssueConsumer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,5 @@ namespace SonarLint.VisualStudio.Core.Analysis;
/// </summary>
public interface IIssueConsumer
{
void Accept(string path, IEnumerable<IAnalysisIssue> issues);
void Set(string path, IEnumerable<IAnalysisIssue> issues);
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public void Create_InitializedIssueConsumerReturned()
/* The empty issues list is passed as an argument here because
it's impossible to verify the actual pipeline due to the fact
that mocking ITextSnapshot in a way that then can be used by a SnapshotSpan takes a lot of effort */
consumer.Accept("analysisfile.txt", []);
consumer.Set("analysisfile.txt", []);

publishedIssuesSnapshot.Should().NotBeNull();
publishedIssuesSnapshot.AnalyzedFilePath.Should().Be("updatedfile.txt"); // filename should be updted by this point
Expand Down
2 changes: 1 addition & 1 deletion src/Integration.Vsix.UnitTests/ReferencesTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public class ReferencesTests
public void MicrosoftVisualStudioVCProjectEngine_EnsureCorrectVersion()
{
var codeAnalysisAssemblyVersion = AssemblyHelper.GetVersionOfReferencedAssembly(
typeof(SonarLint.VisualStudio.Integration.Vsix.AccumulatingIssueConsumer), // any type in the VSIX assembly will do
typeof(SonarLint.VisualStudio.Integration.Vsix.IssueConsumer), // any type in the VSIX assembly will do
"Microsoft.VisualStudio.VCProjectEngine");

AssertIsCorrectMajorVersion(codeAnalysisAssemblyVersion.Major);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,15 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using System;
using System.Collections.Generic;
using System.Linq;
using FluentAssertions;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Microsoft.VisualStudio.Text;
using Moq;
using SonarLint.VisualStudio.Core.Analysis;
using SonarLint.VisualStudio.Integration.Vsix;
using SonarLint.VisualStudio.IssueVisualization.Models;
using SonarLint.VisualStudio.TestInfrastructure;

namespace SonarLint.VisualStudio.Integration.UnitTests.SonarLintTagger
{
[TestClass]
public class AccumulatingIssueConsumerTests
public class IssueConsumerTests
{
private static readonly IAnalysisIssue ValidIssue = CreateIssue(startLine: 1, endLine: 1);
private static readonly ITextSnapshot ValidTextSnapshot = CreateSnapshot(lineCount: 10);
Expand All @@ -44,18 +37,18 @@ public class AccumulatingIssueConsumerTests
[TestMethod]
public void Ctor_InvalidArgs_Throws()
{
AccumulatingIssueConsumer.OnIssuesChanged validCallback = _ => { };
IssueConsumer.OnIssuesChanged validCallback = _ => { };

Action act = () => new AccumulatingIssueConsumer(null, ValidFilePath, validCallback, ValidConverter);
Action act = () => new IssueConsumer(null, ValidFilePath, validCallback, ValidConverter);
act.Should().ThrowExactly<ArgumentNullException>().And.ParamName.Should().Be("analysisSnapshot");

act = () => new AccumulatingIssueConsumer(ValidTextSnapshot, null, validCallback, ValidConverter);
act = () => new IssueConsumer(ValidTextSnapshot, null, validCallback, ValidConverter);
act.Should().ThrowExactly<ArgumentNullException>().And.ParamName.Should().Be("analysisFilePath");

act = () => new AccumulatingIssueConsumer(ValidTextSnapshot, ValidFilePath, null, ValidConverter);
act = () => new IssueConsumer(ValidTextSnapshot, ValidFilePath, null, ValidConverter);
act.Should().ThrowExactly<ArgumentNullException>().And.ParamName.Should().Be("onIssuesChangedCallback");

act = () => new AccumulatingIssueConsumer(ValidTextSnapshot, ValidFilePath, validCallback, null);
act = () => new IssueConsumer(ValidTextSnapshot, ValidFilePath, validCallback, null);
act.Should().ThrowExactly<ArgumentNullException>().And.ParamName.Should().Be("issueToIssueVisualizationConverter");
}

Expand All @@ -65,11 +58,11 @@ public void Accept_WrongFile_CallbackIsNotCalled()
var callbackSpy = new OnIssuesChangedCallbackSpy();
var issues = new IAnalysisIssue[] { ValidIssue };

var testSubject = new AccumulatingIssueConsumer(ValidTextSnapshot, "c:\\file1.txt", callbackSpy.Callback, ValidConverter);
var testSubject = new IssueConsumer(ValidTextSnapshot, "c:\\file1.txt", callbackSpy.Callback, ValidConverter);

using (new AssertIgnoreScope())
{
testSubject.Accept("wrong file", issues);
testSubject.Set("wrong file", issues);
}

callbackSpy.CallCount.Should().Be(0);
Expand All @@ -95,11 +88,11 @@ public void Accept_IssuesNotInSnapshotAreIgnored_CallbackIsCalledWithExpectedIss
var callbackSpy = new OnIssuesChangedCallbackSpy();
var converter = CreatePassthroughConverter();

var testSubject = new AccumulatingIssueConsumer(snapshot, ValidFilePath, callbackSpy.Callback, converter);
var testSubject = new IssueConsumer(snapshot, ValidFilePath, callbackSpy.Callback, converter);

using (new AssertIgnoreScope())
{
testSubject.Accept(ValidFilePath, issues);
testSubject.Set(ValidFilePath, issues);
}

callbackSpy.CallCount.Should().Be(1);
Expand All @@ -122,16 +115,16 @@ public void Accept_HasFileLevelIssues_NotIgnored()
var callbackSpy = new OnIssuesChangedCallbackSpy();
var converter = CreatePassthroughConverter();

var testSubject = new AccumulatingIssueConsumer(snapshot, ValidFilePath, callbackSpy.Callback, converter);
var testSubject = new IssueConsumer(snapshot, ValidFilePath, callbackSpy.Callback, converter);

testSubject.Accept(ValidFilePath, issues);
testSubject.Set(ValidFilePath, issues);

callbackSpy.CallCount.Should().Be(1);
callbackSpy.LastSuppliedIssues.Should().BeEquivalentTo(issues);
}

[TestMethod]
public void Accept_MultipleCallsToAccept_IssuesAreAccumulated()
public void Accept_MultipleCallsToAccept_IssuesAreReplaced()
{
var callbackSpy = new OnIssuesChangedCallbackSpy();
var firstSetOfIssues = new[]
Expand All @@ -147,19 +140,19 @@ public void Accept_MultipleCallsToAccept_IssuesAreAccumulated()
var snapshot = CreateSnapshot(lineCount: 10);
var converter = CreatePassthroughConverter();

var testSubject = new AccumulatingIssueConsumer(snapshot, ValidFilePath, callbackSpy.Callback, converter);
var testSubject = new IssueConsumer(snapshot, ValidFilePath, callbackSpy.Callback, converter);

// 1. First call
testSubject.Accept(ValidFilePath, firstSetOfIssues);
testSubject.Set(ValidFilePath, firstSetOfIssues);

callbackSpy.CallCount.Should().Be(1);
callbackSpy.LastSuppliedIssues.Should().BeEquivalentTo(firstSetOfIssues);

// 2. Second call
testSubject.Accept(ValidFilePath, secondSetOfIssues);
testSubject.Set(ValidFilePath, secondSetOfIssues);

callbackSpy.CallCount.Should().Be(2);
callbackSpy.LastSuppliedIssues.Should().BeEquivalentTo(firstSetOfIssues.Union(secondSetOfIssues));
callbackSpy.LastSuppliedIssues.Should().BeEquivalentTo(secondSetOfIssues);
}

private class OnIssuesChangedCallbackSpy
Expand Down Expand Up @@ -217,7 +210,7 @@ private static IAnalysisIssueVisualizationConverter CreatePassthroughConverter()
mockIssueConverter
.Setup(x => x.Convert(It.IsAny<IAnalysisIssue>(), It.IsAny<ITextSnapshot>()))
.Returns<IAnalysisIssue, ITextSnapshot>((issue, snapshot) => CreateIssueViz(issue, new SnapshotSpan()));

return mockIssueConverter.Object;

IAnalysisIssueVisualization CreateIssueViz(IAnalysisIssue issue, SnapshotSpan snapshotSpan)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ public void RequestAnalysis_ClearsErrorListAndSchedulesAnalysisOnBackgroundThrea
Received.InOrder(() =>
{
threadHandling.RunOnBackgroundThread(Arg.Any<Func<Task<int>>>());
issueConsumer.Accept(analysisFilePath, []);
issueConsumer.Set(analysisFilePath, []);
analysisService.ScheduleAnalysis(analysisFilePath,
Arg.Any<Guid>(),
Arg.Any<IEnumerable<AnalysisLanguage>>(),
Expand Down
2 changes: 1 addition & 1 deletion src/Integration.Vsix/Analysis/IssueConsumerFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public IIssueConsumer Create(ITextDocument textDocument,
SnapshotChangedHandler onSnapshotChanged)
{
var issueHandler = new IssueHandler(textDocument, projectName, projectGuid, suppressedIssueMatcher, onSnapshotChanged, localHotspotsStore);
var issueConsumer = new AccumulatingIssueConsumer(analysisSnapshot, analysisFilePath, issueHandler.HandleNewIssues, converter);
var issueConsumer = new IssueConsumer(analysisSnapshot, analysisFilePath, issueHandler.HandleNewIssues, converter);

return issueConsumer;
}
Expand Down
Loading
Loading