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

Restore MakeUrisAbsoluteVisitor tests #1210

Merged
14 commits merged into from
Jan 13, 2019
Merged

Restore MakeUrisAbsoluteVisitor tests #1210

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a914371
Restore MakeUrisAbsoluteVisitor tests
michaelcfanning Jan 12, 2019
97838b9
Remove old test knobs, now extraneous.
michaelcfanning Jan 12, 2019
ff057db
DRY out RebaseUriVisitor ctors
Jan 13, 2019
1479658
Remove unused code from RebaseUriVisitor.
Jan 13, 2019
beaa698
More code cleanup on RebaseUriVisitor.
Jan 13, 2019
5dd3a40
RebaseUriVisitor: Use null coalescing operator
Jan 13, 2019
f6e65da
Clean up comments in RebaseUriVisitor.
Jan 13, 2019
f1b3734
Build files-array branch in AppVeyor, temporarily.
Jan 13, 2019
f424593
Fix typo.
Jan 13, 2019
338e2fb
Undo change to appveyor.yml -- can't do it while tests fail.
Jan 13, 2019
f4c73cb
Rename Run.GetExpandedUriBaseIdValue to ExpandUrisWithUriBaseId.
Jan 13, 2019
adbc4ea
MakeUriAbsoluteVisitorTests: Remove unused code, clean up helper method.
Jan 13, 2019
28261ec
Initialize Action variable more compactly.
Jan 13, 2019
549d666
Minor cleanups in MakeUrisAbsoluteVisitor and SarifLogStageFactory.
Jan 13, 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
123 changes: 80 additions & 43 deletions src/Sarif.UnitTests/Visitors/MakeUriAbsoluteVisitorTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,26 +11,25 @@ namespace Microsoft.CodeAnalysis.Sarif.Visitors
{
public class MakeUriAbsoluteVisitorTest
{
private Run GenerateRunForTest(Dictionary<string, Uri> uriBaseIdMapping)
private Run GenerateRunForTest(Dictionary<string, FileLocation> uriBaseIdMapping)
{
Run run = new Run();
run.Files = new List<FileData>(new[]
{
new FileData() { FileLocation=new FileLocation{ Uri=new Uri("src/file1.cs", UriKind.Relative), UriBaseId="%TEST1%", FileIndex = 0 } },
new FileData() { FileLocation=new FileLocation{ Uri=new Uri("src/file2.dll", UriKind.Relative), UriBaseId="%TEST2%", FileIndex = 1 } },
new FileData() { FileLocation=new FileLocation{ Uri=new Uri("src/archive.zip", UriKind.Relative), UriBaseId="%TEST1%", FileIndex = 2 } },
new FileData() { FileLocation=new FileLocation{ Uri=new Uri("src/archive.zip#file3.cs", UriKind.Relative), UriBaseId="%TEST1%", FileIndex = 3 } },
new FileData() { FileLocation=new FileLocation{ Uri=new Uri("src/archive.zip#archive2.gz", UriKind.Relative), UriBaseId="%TEST1%", FileIndex = 4 } },
new FileData() { FileLocation=new FileLocation{ Uri=new Uri("src/archive.zip#archive2.gz/file4.cs", UriKind.Relative), UriBaseId="%TEST1%", FileIndex = 5 }, },
new FileData() { FileLocation=new FileLocation{ Uri=new Uri("src/archive.zip#file5.cs", UriKind.Relative), UriBaseId="%TEST1%", FileIndex = 6 }, }
new FileData() { FileLocation=new FileLocation{ Uri=new Uri("file3.cs", UriKind.Relative), FileIndex = 3 }, ParentIndex = 2 },
new FileData() { FileLocation=new FileLocation{ Uri=new Uri("archive2.gz", UriKind.Relative), FileIndex = 4 }, ParentIndex = 2 },
new FileData() { FileLocation=new FileLocation{ Uri=new Uri("file4.cs", UriKind.Relative), FileIndex = 5 }, ParentIndex = 4 },
});

if (uriBaseIdMapping != null)
{
run.Properties = new Dictionary<string, SerializedPropertyInfo>();

run.Properties[RebaseUriVisitor.BaseUriDictionaryName] = RebaseUriVisitor.ReserializePropertyDictionary(uriBaseIdMapping);
}

Copy link

@ghost ghost Jan 13, 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.

Not necessary, remove. #Resolved

Copy link

Choose a reason for hiding this comment

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

Fixed.

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

run.OriginalUriBaseIds = uriBaseIdMapping;
return run;
}

Expand All @@ -44,7 +43,7 @@ public void MakeUriAbsoluteVisitor_VisitPhysicalLocation_SetsAbsoluteURI()
["%TEST%"] = new FileLocation { Uri = new Uri("C:/github/sarif/") }
}
};

// Initializes visitor with run in order to retrieve uri base id mappings
MakeUrisAbsoluteVisitor visitor = new MakeUrisAbsoluteVisitor();
visitor.VisitRun(run);
Expand Down Expand Up @@ -103,79 +102,117 @@ public void MakeUriAbsoluteVisitor_VisitPhysicalLocation_DoesNotSetUriIfBaseIsNo
[Fact]
public void MakeUriAbsoluteVisitor_VisitRun_SetsAbsoluteUriForAllApplicableFiles()
{
Run run = GenerateRunForTest(new Dictionary<string, Uri>()
Run run = GenerateRunForTest(new Dictionary<string, FileLocation>()
{
{ "%TEST1%", new Uri(@"C:\srcroot\") },
{ "%TEST2%", new Uri(@"D:\bld\out\") }
["%TEST1%"] = new FileLocation { Uri = new Uri(@"C:\srcroot\") },
Copy link

@ghost ghost Jan 13, 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.

[ [](start = 16, length = 1)

I do like this syntax so much better! #ByDesign

["%TEST2%"] = new FileLocation { Uri = new Uri(@"D:\bld\out\") }
});

MakeUrisAbsoluteVisitor visitor = new MakeUrisAbsoluteVisitor();

var newRun = visitor.VisitRun(run);

// Validate.
newRun.Files[0].FileLocation.Uri.ToString().Should().Be(@"file://C:/srcroot/src/file1.cs");
newRun.Files[1].FileLocation.Uri.ToString().Should().Be(@"file://D:/bld/out/src/file2.dll");
newRun.Files[2].FileLocation.Uri.ToString().Should().Be(@"file://C:/srcroot/src/archive.zip");
newRun.Files[3].FileLocation.Uri.ToString().Should().Be(@"file://C:/srcroot/src/archive.zip#file3.cs");
newRun.Files[4].FileLocation.Uri.ToString().Should().Be(@"file://C:/srcroot/src/archive.zip#archive2.gz");
newRun.Files[5].FileLocation.Uri.ToString().Should().Be(@"file://C:/srcroot/src/archive.zip#archive2.gz/file4.cs");
newRun.Files[6].FileLocation.Uri.ToString().Should().Be(@"file://C:/srcroot/src/archive.zip#file5.cs");
newRun.Files[0].FileLocation.Uri.ToString().Should().Be(@"file:///C:/srcroot/src/file1.cs");
newRun.Files[1].FileLocation.Uri.ToString().Should().Be(@"file:///D:/bld/out/src/file2.dll");
newRun.Files[2].FileLocation.Uri.ToString().Should().Be(@"file:///C:/srcroot/src/archive.zip");
newRun.Files[3].FileLocation.Uri.ToString().Should().Be(@"file3.cs");
newRun.Files[4].FileLocation.Uri.ToString().Should().Be(@"archive2.gz");
newRun.Files[5].FileLocation.Uri.ToString().Should().Be(@"file4.cs");

// Operation should zap all uri base ids
newRun.Files.Select(f => f.FileLocation.UriBaseId != null).Any().Should().BeFalse();
Copy link
Member Author

@michaelcfanning michaelcfanning Jan 12, 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.

newRun.Files.Select [](start = 12, length = 19)

I messed up using Select here rather than Where on initial update of this test, to elide checks of rewritten files table keys. I was originally confused, thinking Select had been long-lived in our code base. :P mystery solved. #ByDesign

Copy link

Choose a reason for hiding this comment

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

Got it.

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

newRun.Files.Where(f => f.FileLocation.UriBaseId != null).Any().Should().BeFalse();
}

[Fact]
public void MakeUriAbsoluteVisitor_VisitRun_DoesNotSetAbsoluteUriIfNotApplicable()
{
Dictionary<string, Uri> uriMapping = new Dictionary<string, Uri>()
Dictionary<string, FileLocation> uriMapping = new Dictionary<string, FileLocation>()
{
{ "%TEST3%", new Uri(@"C:\srcroot\") },
{ "%TEST4%", new Uri(@"D:\bld\out\") },
["%TEST3%"] = new FileLocation { Uri = new Uri(@"C:\srcroot\") },
["%TEST4%"] = new FileLocation { Uri = new Uri(@"D:\bld\out\") }
};

Run expectedRun = GenerateRunForTest(uriMapping);
Run actualRun = expectedRun.DeepClone();


MakeUrisAbsoluteVisitor visitor = new MakeUrisAbsoluteVisitor();
var newRun = visitor.VisitRun(actualRun);

expectedRun.Should().Be(actualRun);
}

[Fact]
public void MakeUriAbsoluteVisitor_VisitRun_ThrowsIfPropertiesAreWrong()
Copy link
Member Author

@michaelcfanning michaelcfanning Jan 12, 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.

MakeUriAbsoluteVisitor_VisitRun_ThrowsIfPropertiesAreWrong [](start = 20, length = 58)

no longer necessary. used to enforce that we formed the speculative originalUriBaseId dictionary properly. OM now enforces this, now that run.originalUriBaseIds is a thin. #ByDesign

Copy link

Choose a reason for hiding this comment

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

Got it.

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

{
Run run = GenerateRunForTest(null);
run.Properties = new Dictionary<string, SerializedPropertyInfo>();
run.Properties[RebaseUriVisitor.BaseUriDictionaryName] = new SerializedPropertyInfo("\"this is a string\"", true);

MakeUrisAbsoluteVisitor visitor = new MakeUrisAbsoluteVisitor();
Assert.Throws<InvalidOperationException>(()=> visitor.VisitRun(run));
expectedRun.ValueEquals(actualRun).Should().BeTrue();
}

[Fact]
public void MakeUriAbsoluteVisitor_VisitSarifLog_MultipleRunsWithDifferentProperties_RebasesProperly()
{
Run runA = GenerateRunForTest(new Dictionary<string, Uri>()
Run runA = GenerateRunForTest(new Dictionary<string, FileLocation>()
{
{ "%TEST1%", new Uri(@"C:\srcroot\") },
{ "%TEST2%", new Uri(@"D:\bld\out\") },
["%TEST1%"] = new FileLocation { Uri = new Uri(@"C:\srcroot\") },
["%TEST2%"] = new FileLocation { Uri = new Uri(@"D:\bld\out\") }
});
Run runB = GenerateRunForTest(new Dictionary<string, Uri>()
Run runB = GenerateRunForTest(new Dictionary<string, FileLocation>()
{
{ "%TEST1%", new Uri(@"C:\src\abc") },
{ "%TEST2%", new Uri(@"D:\bld\123\") },
["%TEST1%"] = new FileLocation { Uri = new Uri(@"C:\src\abc\") },
["%TEST2%"] = new FileLocation { Uri = new Uri(@"D:\bld\123\") }
});
MakeUrisAbsoluteVisitor visitor = new MakeUrisAbsoluteVisitor();

SarifLog log = new SarifLog() { Runs=new Run[] { runA, runB } };
SarifLog log = new SarifLog() { Runs = new Run[] { runA, runB } };
SarifLog newLog = visitor.VisitSarifLog(log);

// Validate
newLog.Runs.Should().HaveCount(2);
newLog.Runs[0].Files.Should().NotIntersectWith(newLog.Runs[1].Files);
}

[Fact]
public void MakeUriAbsoluteVisitor_CombineUriFunctionsProperly()
{
var testCases = new Tuple<string, string, string>[]
{
new Tuple<string, string, string>
(@"https://base/", @"relative/file.cpp", "https://base/relative/file.cpp")
};

foreach (Tuple<string, string, string> testCase in testCases)
{
MakeUrisAbsoluteVisitor.CombineUris(
absoluteBaseUri: new Uri(testCase.Item1, UriKind.Absolute),
relativeUri: new Uri(testCase.Item2, UriKind.Relative))
.Should().Be(testCase.Item3);
}
}

[Fact]
public void MakeUriAbsoluteVisitor_CombineUriValidatesArgumentsProperly()
{
Uri absoluteUri = new Uri("https://absolute.example.com", UriKind.Absolute);
Uri relativeUri = new Uri("relative/someResource", UriKind.Relative);

// First, ensure that our test data succeeds when used properly
MakeUrisAbsoluteVisitor.CombineUris(
absoluteBaseUri: absoluteUri,
relativeUri: relativeUri);

Copy link

@ghost ghost Jan 13, 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.

Not needed. The previous test covers the happy path. But ok, maybe you want this to be a self-contained test of all argument validation scenarios. #ByDesign

// Pass relative uri where absolute expected
var action = new Action(() =>
Copy link

@ghost ghost Jan 13, 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.

Action [](start = 29, length = 6)

There's a more compact syntax for this. #Resolved

Copy link

Choose a reason for hiding this comment

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

Fixed.

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

{
MakeUrisAbsoluteVisitor.CombineUris(
absoluteBaseUri: relativeUri,
relativeUri: relativeUri);
});

action.Should().Throw<ArgumentException>();

// Pass absolute uri where relative expected
action = new Action(() =>
{
MakeUrisAbsoluteVisitor.CombineUris(
absoluteBaseUri: absoluteUri,
relativeUri: absoluteUri);
});

action.Should().Throw<ArgumentException>();
}
}
}
8 changes: 7 additions & 1 deletion src/Sarif/Core/Run.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,13 @@ public partial class Run

public Uri GetExpandedUriBaseIdValue(string key, string currentValue = null)
Copy link

@ghost ghost Jan 13, 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.

GetExpandedUriBaseIdValue [](start = 19, length = 25)

Suggested method name: ExpandUriFromUriBaseId. Because:

  1. The operation you're doing is "expanding".
  2. The thing you're expanding is a uri, not a uriBaseId.
  3. The word "value" at the end adds nothing. #Resolved

Copy link

Choose a reason for hiding this comment

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

Renamed to ExpandUrisWithUriBaseId.

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

{
throw new InvalidOperationException("Author this code along with tests");
FileLocation fileLocation = this.OriginalUriBaseIds[key];

if (fileLocation.UriBaseId == null)
{
return fileLocation.Uri;
}
throw new InvalidOperationException("Author this code along with tests for originalUriBaseIds that are nested");
}

public int GetFileIndex(
Expand Down
20 changes: 17 additions & 3 deletions src/Sarif/Visitors/MakeUrisAbsoluteVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ public override FileLocation VisitFileLocation(FileLocation node)
{
if ( _run.OriginalUriBaseIds!= null &&
!string.IsNullOrEmpty(node?.UriBaseId) &&
_run.OriginalUriBaseIds.ContainsKey(node.UriBaseId))
_run.OriginalUriBaseIds.ContainsKey(node.UriBaseId) &&
!_run.OriginalUriBaseIds.Values.Contains(node))
{
Uri baseUri = _run.GetExpandedUriBaseIdValue(node.UriBaseId);
node.Uri = CombineUris(baseUri, node.Uri);
Expand All @@ -29,9 +30,22 @@ public override FileLocation VisitFileLocation(FileLocation node)
return node;
}

private Uri CombineUris(Uri baseUri, Uri uri)
internal static Uri CombineUris(Uri absoluteBaseUri, Uri relativeUri)
{
throw new NotImplementedException();
if (!absoluteBaseUri.IsAbsoluteUri)
{
throw new ArgumentException("absoluteBaseUri is not an absolute Uri", nameof(absoluteBaseUri));
Copy link

@ghost ghost Jan 13, 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.

absoluteBaseUri [](start = 45, length = 15)

nameof #Resolved

Copy link

Choose a reason for hiding this comment

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

Fixed.

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

}

if (relativeUri.IsAbsoluteUri)
{
throw new ArgumentException("relativeUri is not a relative Uri", nameof(relativeUri));
}

Uri test = new Uri("src/archive.zip#archive2.gz", UriKind.Relative);

Copy link

@ghost ghost Jan 13, 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.

Oops :-) #Resolved

Copy link

Choose a reason for hiding this comment

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

Fixed.

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

Uri result = new Uri(absoluteBaseUri, relativeUri);
Copy link

@ghost ghost Jan 13, 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.

result [](start = 16, length = 6)

Temporary not needed #Resolved

Copy link

Choose a reason for hiding this comment

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

Fixed.

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

return result;
}
}
}
16 changes: 0 additions & 16 deletions src/Sarif/Visitors/RebaseUriVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,21 +100,5 @@ internal static bool TryDeserializePropertyDictionary(SerializedPropertyInfo ser
throw;
}
}

/// <summary>
/// Internal as used in testing as a helper.
/// </summary>
internal static Dictionary<string, Uri> DeserializePropertyDictionary(SerializedPropertyInfo info)
{
return JsonConvert.DeserializeObject<Dictionary<string, Uri>>(info.SerializedValue);
}

/// <summary>
/// Internal as used in testing as a helper.
/// </summary>
internal static SerializedPropertyInfo ReserializePropertyDictionary(Dictionary<string, Uri> dictionary)
{
return new SerializedPropertyInfo(JsonConvert.SerializeObject(dictionary), false);
}
}
}