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.

Sign up for GitHub

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

Jump to bottom

Fixing AnalyzeCommandBase and MultithreadedAnalyzeCommandBase artifacts generation #2433

Merged
merged 16 commits into from
Jan 31, 2022
Merged

Fixing AnalyzeCommandBase and MultithreadedAnalyzeCommandBase artifacts generation #2433

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
2247cbe
Fixing multithreaded artifacts generation
eddynaka Jan 28, 2022
6972472
Fixing tests and flags
eddynaka Jan 28, 2022
5d87498
Loosing precision.
eddynaka Jan 28, 2022
af33d03
Applying fix for AnalyzeCommandBase
eddynaka Jan 28, 2022
3869b86
Enabling tests
eddynaka Jan 28, 2022
70dffb5
Updating test case and release history
eddynaka Jan 28, 2022
171133f
Creating const to prevent magical numbers everywhere
eddynaka Jan 28, 2022
d322ee4
Rebaselining tests
eddynaka Jan 28, 2022
e800ead
Merge branch 'users/ednakamu/rebaselining-files' into users/ednakamu/…
eddynaka Jan 28, 2022
fd9bd16
Creating Artifacts flag to keep previous behavior
eddynaka Jan 28, 2022
6996260
Addressing PR feedback.
eddynaka Jan 28, 2022
8a78dd8
Rollback changes
eddynaka Jan 28, 2022
44eb8d2
Update SARIF2012.ProvideRuleProperties_Invalid.sarif
eddynaka Jan 30, 2022
cabbd5f
updating back
eddynaka Jan 30, 2022
a3408b3
Ordering deprecated names
eddynaka Jan 31, 2022
dbaae40
Merge branch 'users/ednakamu/fixing-multithreaded-artifacts' of https…
eddynaka Jan 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 11 additions & 4 deletions src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public abstract class MultithreadedAnalyzeCommandBase<TContext, TOptions> : Plug
private Run _run;
private Tool _tool;
private bool _computeHashes;
private bool _persistArtifacts;
internal TContext _rootContext;
private int _fileContextsCount;
private Channel<int> _hashChannel;
Expand Down Expand Up @@ -307,6 +308,11 @@ private void LogCachingLogger(TContext rootContext, IAnalysisLogger logger, TCon

if (results?.Count > 0)
{
_run?.GetFileIndex(new ArtifactLocation { Uri = context.TargetUri },
addToFilesTableIfNotPresent: _persistArtifacts,
dataToInsert: _dataToInsert,
hashData: context.Hashes);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we should only persist if:

  1. we have results
  2. if we have hashes/textfiles/binaryfiles flag enabled


foreach (KeyValuePair<ReportingDescriptor, IList<Result>> kv in results)
{
foreach (Result result in kv.Value)
Expand Down Expand Up @@ -485,10 +491,6 @@ private async Task<bool> HashAsync()
_hashToFilesMap[hashData.Sha256] = paths;
}

_run?.GetFileIndex(new ArtifactLocation { Uri = context.TargetUri },
dataToInsert: _dataToInsert,
hashData: hashData);
Copy link
Collaborator Author

@eddynaka eddynaka Jan 28, 2022 •
edited

Choose a reason for hiding this comment

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

removing this to prevent us from generating artifacts for all the files #ByDesign


paths.Add(localPath);
context.Hashes = hashData;
}
Copy link
Collaborator Author

@eddynaka eddynaka Jan 28, 2022 •
edited

Choose a reason for hiding this comment

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

Updating this will enable us to use the code from SarifLogger to create the artifacts #ByDesign

Expand Down Expand Up @@ -539,6 +541,11 @@ protected virtual void ValidateOptions(TOptions options, TContext context)
_dataToInsert = options.DataToInsert.ToFlags();
_computeHashes = (_dataToInsert & OptionallyEmittedData.Hashes) != 0;

_persistArtifacts =
(_dataToInsert & OptionallyEmittedData.Hashes) != 0 ||
(_dataToInsert & OptionallyEmittedData.TextFiles) != 0 ||
(_dataToInsert & OptionallyEmittedData.BinaryFiles) != 0;

bool succeeded = true;

succeeded &= ValidateFile(context, options.OutputFilePath, DefaultPolicyName, shouldExist: null);
Expand Down
57 changes: 55 additions & 2 deletions src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1165,7 +1165,7 @@ public void AnalyzeCommandBase_AutomationDetailsTests()
}
}

[Fact(Timeout = 5000)]
[Fact(Timeout = 5000, Skip = "Artifacts will be different while we don't fix SarifLogger and AnalyzeCommandBase.")]
public void AnalyzeCommandBase_ShouldGenerateSameResultsWhenRunningSingleAndMultiThread_CoyoteTest()
Copy link
Collaborator Author

@eddynaka eddynaka Jan 28, 2022 •
edited

Choose a reason for hiding this comment

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

AnalyzeCommandBase_ShouldGenerateSameResultsWhenRunningSingleAndMultiThread_CoyoteTest

This test is comparing the output of a single thread vs multithreaded command base.

With this change, both will have different outputs which will be fixed in the future. #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

already fixed in this pr

Copy link
Member

@michaelcfanning michaelcfanning Jan 28, 2022 •
edited by eddynaka

Choose a reason for hiding this comment

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

AnalyzeCommandBase_ShouldGenerateSameResultsWhenRunningSingleAndMultiThread_CoyoteTest

SingleAndMultithreaded #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done!

{
Configuration config = Configuration.Create().WithTestingIterations(100).WithConcurrencyFuzzingEnabled();
Expand All @@ -1184,13 +1184,66 @@ public void AnalyzeCommandBase_ShouldGenerateSameResultsWhenRunningSingleAndMult
Assert.True(report.NumOfFoundBugs == 0, $"Coyote found {report.NumOfFoundBugs} bug(s).");
}

[Fact]
[Fact(Skip = "Artifacts will be different while we don't fix SarifLogger and AnalyzeCommandBase.")]
public void AnalyzeCommandBase_ShouldGenerateSameResultsWhenRunningSingleAndMultiThread()
{
int[] scenarios = SetupScenarios();
AnalyzeScenarios(scenarios);
}

[Fact]
public void AnalyzeCommandBase_Multithreaded_ShouldOnlyLogArtifactsWhenResultsAreFound()
{
const int expectedNumberOfArtifacts = 2;
const int expectedNumberOfResultsWithErrors = 1;
const int expectedNumberOfResultsWithWarnings = 1;
var files = new List<string>
{
$@"{Environment.CurrentDirectory}\Error.dll",
$@"{Environment.CurrentDirectory}\Warning.dll",
$@"{Environment.CurrentDirectory}\Note.dll",
$@"{Environment.CurrentDirectory}\Pass.dll",
$@"{Environment.CurrentDirectory}\NotApplicable.exe",
$@"{Environment.CurrentDirectory}\Informational.sys",
$@"{Environment.CurrentDirectory}\Open.cab",
$@"{Environment.CurrentDirectory}\Review.dll",
$@"{Environment.CurrentDirectory}\NoIssues.dll",
};

var testCase = new ResultsCachingTestCase
{
Files = files,
PersistLogFileToDisk = true,
FileSystem = CreateDefaultFileSystemForResultsCaching(files, generateSameInput: false)
};

var options = new TestAnalyzeOptions
{
TestRuleBehaviors = testCase.TestRuleBehaviors,
OutputFilePath = testCase.PersistLogFileToDisk ? Guid.NewGuid().ToString() : null,
TargetFileSpecifiers = new string[] { Guid.NewGuid().ToString() },
Kind = new List<ResultKind> { ResultKind.Fail },
Level = new List<FailureLevel> { FailureLevel.Warning, FailureLevel.Error },
DataToInsert = new OptionallyEmittedData[] { OptionallyEmittedData.Hashes },
};

Run run = RunAnalyzeCommand(options, testCase, multithreaded: true);

// Hashes is enabled and we should expect to see two artifacts because we have:
// one result with Error level and one result with Warning level.
run.Artifacts.Should().HaveCount(expectedNumberOfArtifacts);
run.Results.Count(r => r.Level == FailureLevel.Error).Should().Be(expectedNumberOfResultsWithErrors);
run.Results.Count(r => r.Level == FailureLevel.Warning).Should().Be(expectedNumberOfResultsWithWarnings);

options.DataToInsert = new List<OptionallyEmittedData>();
run = RunAnalyzeCommand(options, testCase, multithreaded: true);

// Hashes is not enabled, so no artifacts are expected.
run.Artifacts.Should().BeNull();
run.Results.Count(r => r.Level == FailureLevel.Error).Should().Be(expectedNumberOfResultsWithErrors);
run.Results.Count(r => r.Level == FailureLevel.Warning).Should().Be(expectedNumberOfResultsWithWarnings);
}

private void AnalyzeCommandBase_ShouldGenerateSameResultsWhenRunningSingleAndMultiThread_CoyoteHelper()
{
int[] scenarios = SetupScenarios(true);
Expand Down