-
Notifications
You must be signed in to change notification settings - Fork 91
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
Restore MakeUrisAbsoluteVisitor tests #1210
Changes from 1 commit
a914371
97838b9
ff057db
1479658
beaa698
5dd3a40
f6e65da
f1b3734
f424593
338e2fb
f4c73cb
adbc4ea
28261ec
549d666
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,26 +11,26 @@ 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); | ||
} | ||
|
||
run.OriginalUriBaseIds = uriBaseIdMapping; | ||
return run; | ||
} | ||
|
||
|
@@ -44,7 +44,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); | ||
|
@@ -103,79 +103,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\") }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
{ | ||
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); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(() => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There's a more compact syntax for this. #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
{ | ||
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>(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,13 @@ public partial class Run | |
|
||
public Uri GetExpandedUriBaseIdValue(string key, string currentValue = null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested method name:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
{ | ||
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( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
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); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops :-) #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
Uri result = new Uri(absoluteBaseUri, relativeUri); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Temporary not needed #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
return result; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary, remove. #Resolved
There was a problem hiding this comment.
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)