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

The multiool, core sarif, and validation test binaries now all pass #1215

Merged
merged 6 commits into from
Jan 16, 2019
Merged

The multiool, core sarif, and validation test binaries now all pass #1215

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bd80b54
The multiool, core sarif, and validation test binaries now all pass c…
michaelcfanning Jan 16, 2019
0e99aee
Remove unwanted assert that may fire during unit testing.
michaelcfanning Jan 16, 2019
66ce107
Merge branch 'files-array' into multitool-core-and-validation-succeed
michaelcfanning Jan 16, 2019
008b49f
Merge from files-array
michaelcfanning Jan 16, 2019
5077d2c
PR feedback.
michaelcfanning Jan 16, 2019
c278310
PR feedback tweaks
michaelcfanning Jan 16, 2019
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
23 changes: 7 additions & 16 deletions src/Sarif.Driver/Sdk/SkimmerBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,12 @@

namespace Microsoft.CodeAnalysis.Sarif.Driver
{
public abstract class SkimmerBase<TContext> : PropertyBagHolder, ISkimmer<TContext>
public abstract class SkimmerBase<TContext> : Rule, ISkimmer<TContext>
Copy link
Member Author

@michaelcfanning michaelcfanning Jan 16, 2019 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

Rule [](start = 51, length = 4)

Rule derives from PropertyBag. The big problem with not using interfaces, of course, is that you need a single, sensible base class chain. We barely are able to accomplish this to eliminate IRule (thanks to the SARIF design change to make everything have a property bag). #ByDesign

{
public SkimmerBase()
{
this.Options = new Dictionary<string, string>();
}

abstract public Uri HelpUri { get; }

abstract public Message Help { get; }

private IDictionary<string, string> messageStrings;
private IDictionary<string, string> richMessageStrings;

Expand All @@ -27,11 +22,10 @@ public SkimmerBase()

virtual protected IEnumerable<string> RichMessageResourceNames => new List<string>();

virtual public RuleConfiguration Configuration { get; }

virtual public ResultLevel DefaultLevel { get { return ResultLevel.Warning; } }

virtual public IDictionary<string, string> MessageStrings
override public IDictionary<string, string> MessageStrings
{
get
{
Expand All @@ -43,7 +37,7 @@ virtual public IDictionary<string, string> MessageStrings
}
}

virtual public IDictionary<string, string> RichMessageStrings
override public IDictionary<string, string> RichMessageStrings
{
get
{
Expand All @@ -65,14 +59,11 @@ private Dictionary<string, string> InitializeRichMessageStrings()
return RuleUtilities.BuildDictionary(ResourceManager, RichMessageResourceNames,ruleId: Id, prefix: "Rich");
}

abstract public string Id { get; }

// This one isn't abstract because there's no point in forcing every skimmer to implement it.
public virtual IList<string> DeprecatedIds => null;
public override string Id => throw new InvalidOperationException($"The {nameof(Id)} property must be overridden in the SkimmerBase-derived class.");

abstract public Message FullDescription { get; }
public override Message FullDescription => throw new InvalidOperationException($"The {nameof(FullDescription)} property must be overridden in the SkimmerBase-derived class.");

public virtual Message ShortDescription
public override Message ShortDescription
{
get { return new Message { Text = FirstSentence(FullDescription.Text) }; }
}
Expand Down Expand Up @@ -105,7 +96,7 @@ internal static string FirstSentence(string fullDescription)
return fullDescription.Substring(0, length) + (truncated ? "..." : "");
}

public virtual Message Name { get { return new Message { Text = this.GetType().Name }; } }
public override Message Name { get { return new Message { Text = this.GetType().Name }; } }

public IDictionary<string, string> Options { get; }

Expand Down
31 changes: 24 additions & 7 deletions src/Sarif.Multitool.FunctionalTests/Rules/ValidationSkimmerTestsBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using FluentAssertions;
Expand Down Expand Up @@ -87,15 +88,31 @@ protected void Verify(string testFileName, bool disablePrereleaseCompatibilityTr
// Verify that those detected result locations match the expected locations.
private void Verify(Run run, ExpectedValidationResults expectedResults)
Copy link
Member Author

@michaelcfanning michaelcfanning Jan 16, 2019 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

Verify [](start = 21, length = 6)

I couldn't debug this test easily. So made a slight improvement where the relevant locations of unexpected conditions were isolated/more obvious. #ByDesign

{
string[] detectedResultLocations = run.Results.Select(r => r.Message.Arguments[0]).OrderBy(loc => loc).ToArray();
string[] expectedResultLocations = expectedResults.ResultLocationPointers.OrderBy(loc => loc).ToArray();
HashSet<string> actualResultLocations = new HashSet<string>(run.Results.Select(r => r.Message.Arguments[0]).OrderBy(loc => loc));
Copy link

@ghost ghost Jan 16, 2019 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

.OrderBy(loc => loc) [](start = 119, length = 20)

Since you're making a HashSet, there's no reason to order the list. Same thing on the next line. #Closed

Copy link
Member Author

@michaelcfanning michaelcfanning Jan 16, 2019 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

Got it. FYI, i have updated this PR to merge files-array, will push an update in a moment. #Closed

HashSet<string> unexpectedlyAbsentResultLocations = new HashSet<string>(expectedResults.ResultLocationPointers.OrderBy(loc => loc));

// We could make this assertion at the start of the method. We delay it until here
// so that, during debugging, you can set a breakpoint here, and you'll have the
// detected and expected result location arrays available to compare.
run.Results.Count.Should().Be(expectedResults.ResultCount);
List<string> unexpectedNewIssues = new List<string>();

detectedResultLocations.Should().ContainInOrder(expectedResultLocations);
foreach(string location in actualResultLocations)
{
if (unexpectedlyAbsentResultLocations.Contains(location))
{
// Happy path. Actual location found in expected
unexpectedlyAbsentResultLocations.Remove(location);
}
else
{
// Bad new unexpected issue
unexpectedNewIssues.Add(location);
}
}

// No new issues
unexpectedNewIssues.Count.Should().Be(0);

// No unexpectedly absent issues. If we found everything we expected,
// then our expected results should be empty
unexpectedlyAbsentResultLocations.Count.Should().Be(0);
Copy link

@ghost ghost Jan 16, 2019 •
edited by michaelcfanning
Loading

Choose a reason for hiding this comment

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

unexpectedlyAbsentResultLocations [](start = 12, length = 33)

This whole thing can be done with a couple of LINQ calls. Also, a suggested variable rename for consistency:

IEnumerable<string> unexpectedNewResultLocations = actualResultLocations.Except(expectedResults.ResultLocationPointers);
unexpectedNewResultLocations.Count().Should().Be(0);

IEnumerable<string> unexpectedlyAbsentResultLocations = expectedResults.ResultLocationPointers.Except(actualResultLocations);
unexpectedlyAbsentResultLocations.Count().Should().Be(0);
``` #WontFix

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered this but I don't like leaving O(N^2) comparisons hanging around. The code as authored is much less compact/elegant but is clear and performant.

In reply to: 248362142 [](ancestors = 248362142)

Copy link

Choose a reason for hiding this comment

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

Would you at least do the variable rename?

In reply to: 248384220 [](ancestors = 248384220,248362142)

}
}
}
Loading