Update insert optional data tests and update indices visitor. (#1212)

* Update insert optional data tests and update indices visitor.

* Delete speculatively needed file

* Remove erroneous override of base visit method.

* Rework summary comment on DeletesOutputsDirectoryOnClassInitializationFixture.

src/Sarif.Converters.UnitTests/FortifyFprConverterTests.cs

@@ -1,13 +1,16 @@
 // Copyright (c) Microsoft. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+using Microsoft.CodeAnalysis.Sarif.TestUtilities;
 using Xunit;
 using Xunit.Abstractions;
 
 namespace Microsoft.CodeAnalysis.Sarif.Converters
 {
-    public class FortifyFprConverterTests : FileDiffingTests
+    public class FortifyFprConverterTests : FileDiffingTests, IClassFixture<FortifyFprConverterTests.FortifyFprConverterTestsFixture>
     {
+        public class FortifyFprConverterTestsFixture : DeletesOutputsDirectoryOnClassInitializationFixture { }
+
         protected override string TestLogResourceNameRoot => "Microsoft.CodeAnalysis.Sarif.Converters.UnitTests.TestData." + TypeUnderTest;
 
         public FortifyFprConverterTests(ITestOutputHelper outputHelper) : base(outputHelper) { }

src/Sarif.TestUtilities/DeletesOutputsDirectoryOnClassInitializationFixture.cs

@@ -0,0 +1,42 @@
+// Copyright (c) Microsoft. All rights reserved. Licensed under the MIT        
+// license. See LICENSE file in the project root for full license information.
+
+using System.IO;
+
+namespace Microsoft.CodeAnalysis.Sarif.TestUtilities
+{
+    /// <summary>
+    /// This class is an XUnit "class fixture." If a test class is marked with the interface
+    /// IClassFixture&lt;T>, then before XUnit runs any tests from the class, it will instantiate
+    /// T (which must have a parameterless constructor). If T implements IDisposable, then after
+    /// xUnit runs the last test method from the class, it will dispose the fixture. This mechanism
+    /// allows class-level setup and teardown (although I don't know why they don't just reflect
+    /// for static methods like ClassSetup and ClassTeardown).
+    ///
+    /// See https://xunit.github.io/docs/shared-context for more information about xUnit class fixtures.
+    ///
+    /// This particular fixture deletes any existing test output files that may have been produced
+    /// by a previous run. It is designed for use on test classes that derive from FileDiffingTests.
+    /// It is required because FileDiffingTests emits the outputs from each test to a common directory,
+    /// to allow diffing an entire directory of failing tests. If each test case deleted this directory,
+    /// then at the end it would contain only the output from the last failing test.
+    ///
+    /// Each class that derives from FileDiffingTests can declare its own derived fixture class if
+    /// it wants to override the virtual TypeUnderTest or OutputFolderPath properties, but there seems
+    /// no good reason to do this.
+    /// </summary>
+    public abstract class DeletesOutputsDirectoryOnClassInitializationFixture
+    {
+        protected virtual string TypeUnderTest => this.GetType().Name.Substring(0, this.GetType().Name.Length - "TestsFixture".Length);
+
+        protected virtual string OutputFolderPath => Path.Combine(Path.GetDirectoryName(this.GetType().Assembly.Location), "UnitTestOutput." + TypeUnderTest);
+
+        public DeletesOutputsDirectoryOnClassInitializationFixture()
+        {
+            if (Directory.Exists(OutputFolderPath))
+            {
+                Directory.Delete(OutputFolderPath, recursive: true);
+            }
+        }
+    }
+}

src/Sarif.TestUtilities/FileDiffingTests.cs

@@ -13,6 +13,7 @@
 using Newtonsoft.Json;
 using Newtonsoft.Json.Linq;
 using Newtonsoft.Json.Serialization;
+using Xunit;
 using Xunit.Abstractions;
 
 namespace Microsoft.CodeAnalysis.Sarif
@@ -39,11 +40,6 @@ public FileDiffingTests(ITestOutputHelper outputHelper, bool testProducesSarifCu
             _outputHelper = outputHelper;
             _testProducesSarifCurrentVersion = testProducesSarifCurrentVersion;
 
-            if (Directory.Exists(OutputFolderPath))
-            {
-                Directory.Delete(OutputFolderPath, recursive: true);
-            }
-
             Directory.CreateDirectory(OutputFolderPath);
         }
 
@@ -57,23 +53,24 @@ public FileDiffingTests(ITestOutputHelper outputHelper, bool testProducesSarifCu
 
         protected virtual string TestLogResourceNameRoot => "Microsoft.CodeAnalysis.Sarif.UnitTests.TestData." + TypeUnderTest;
 
-        protected virtual string ConstructTestOutputFromInputResource(string inputResource) { return string.Empty; }
+        protected virtual string ConstructTestOutputFromInputResource(string inputResourceName) { return string.Empty; }
 
         protected virtual bool RebaselineExpectedResults => false;
 
-        protected virtual void RunTest(string resourceName)
+        protected virtual void RunTest(string inputResourceName, string expectedOutputResourceName = null)
         {
+            expectedOutputResourceName = expectedOutputResourceName ?? inputResourceName;
             // When retrieving constructed test content, we pass the resourceName is the test
             // specified it. When constructing actual and expected file names from this data, 
             // however, we will ensure that the name has the ".sarif" extension. We do this
             // for test classes such as the Fortify converter that operate again non-SARIF inputs.
-            string actualSarifText = ConstructTestOutputFromInputResource("Inputs." + resourceName);
+            string actualSarifText = ConstructTestOutputFromInputResource("Inputs." + inputResourceName);
 
-            resourceName = Path.GetFileNameWithoutExtension(resourceName) + ".sarif";
+            expectedOutputResourceName = Path.GetFileNameWithoutExtension(expectedOutputResourceName) + ".sarif";
 
             var sb = new StringBuilder();
 
-            string expectedSarifText = GetResourceText("ExpectedOutputs." + resourceName);
+            string expectedSarifText = GetResourceText("ExpectedOutputs." + expectedOutputResourceName);
 
             bool passed;
             if (_testProducesSarifCurrentVersion)
@@ -88,13 +85,13 @@ protected virtual void RunTest(string resourceName)
 
             if (!passed)
             {
-                string errorMessage = string.Format(@"there should be no unexpected diffs detected comparing actual results to '{0}'.", resourceName);
+                string errorMessage = string.Format(@"there should be no unexpected diffs detected comparing actual results to '{0}'.", inputResourceName);
                 sb.AppendLine(errorMessage);
 
                 if (!Utilities.RunningInAppVeyor)
                 {
-                    string expectedFilePath = GetOutputFilePath("ExpectedOutputs", resourceName);
-                    string actualFilePath = GetOutputFilePath("ActualOutputs", resourceName);
+                    string expectedFilePath = GetOutputFilePath("ExpectedOutputs", expectedOutputResourceName);
+                    string actualFilePath = GetOutputFilePath("ActualOutputs", expectedOutputResourceName);
 
                     string expectedRootDirectory = Path.GetDirectoryName(expectedFilePath);
                     string actualRootDirectory = Path.GetDirectoryName(actualFilePath);
@@ -116,7 +113,7 @@ protected virtual void RunTest(string resourceName)
                         // We retrieve all test strings from embedded resources. To rebaseline, we need to
                         // compute the enlistment location from which these resources are compiled.
 
-                        expectedFilePath = Path.Combine(testDirectory, resourceName);
+                        expectedFilePath = Path.Combine(testDirectory, expectedOutputResourceName);
                         File.WriteAllText(expectedFilePath, actualSarifText);
                     }
                 }

src/Sarif.UnitTests/Processors/Log/SarifLogExtensionTests.cs

@@ -78,7 +78,7 @@ public void AbsoluteUri_ReversesRebasedURIs()
                 logs.Add(log);
             }
             logs.RebaseUri("SRCROOT", false, new Uri(RandomSarifLogGenerator.GeneratorBaseUri)).MakeUrisAbsolute();
-
+          
             // All file URIs should be absolute.
             logs.All(
                 log =>

src/Sarif.UnitTests/Sarif.UnitTests.csproj

@@ -31,6 +31,15 @@
   </ItemGroup>
 
   <ItemGroup>
+    <None Remove="TestData\InsertOptionalDataVisitor\ExpectedOutputs\CoreTests_ComprehensiveRegionProperties.sarif" />
+    <None Remove="TestData\InsertOptionalDataVisitor\ExpectedOutputs\CoreTests_ContextRegionSnippets.sarif" />
+    <None Remove="TestData\InsertOptionalDataVisitor\ExpectedOutputs\CoreTests_FlattenedMessages.sarif" />
+    <None Remove="TestData\InsertOptionalDataVisitor\ExpectedOutputs\CoreTests_Hashes+TextFiles+ComprehensiveRegionProperties+RegionSnippets+ContextRegionSnippets+FlattenedMessages.sarif" />
+    <None Remove="TestData\InsertOptionalDataVisitor\ExpectedOutputs\CoreTests_Hashes+TextFiles.sarif" />
+    <None Remove="TestData\InsertOptionalDataVisitor\ExpectedOutputs\CoreTests_Hashes.sarif" />
+    <None Remove="TestData\InsertOptionalDataVisitor\ExpectedOutputs\CoreTests_RegionSnippets.sarif" />
+    <None Remove="TestData\InsertOptionalDataVisitor\ExpectedOutputs\CoreTests_TextFiles.sarif" />
+    <None Remove="TestData\InsertOptionalDataVisitor\Inputs\CoreTests.sarif" />
     <None Remove="TestData\PrereleaseCompatibilityTransformer\ExpectedOutputs\NestedFiles.sarif" />
     <None Remove="TestData\PrereleaseCompatibilityTransformer\Inputs\NestedFiles.sarif" />
     <None Remove="TestData\SarifCurrentToVersionOneVisitor\ExpectedOutputs\Minimum.sarif" />
@@ -88,6 +97,15 @@
   </ItemGroup>
 
   <ItemGroup>
+    <EmbeddedResource Include="TestData\InsertOptionalDataVisitor\ExpectedOutputs\CoreTests_ComprehensiveRegionProperties.sarif" />
+    <EmbeddedResource Include="TestData\InsertOptionalDataVisitor\ExpectedOutputs\CoreTests_ContextRegionSnippets.sarif" />
+    <EmbeddedResource Include="TestData\InsertOptionalDataVisitor\ExpectedOutputs\CoreTests_FlattenedMessages.sarif" />
+    <EmbeddedResource Include="TestData\InsertOptionalDataVisitor\ExpectedOutputs\CoreTests_Hashes+TextFiles+ComprehensiveRegionProperties+RegionSnippets+ContextRegionSnippets+FlattenedMessages.sarif" />
+    <EmbeddedResource Include="TestData\InsertOptionalDataVisitor\ExpectedOutputs\CoreTests_Hashes+TextFiles.sarif" />
+    <EmbeddedResource Include="TestData\InsertOptionalDataVisitor\ExpectedOutputs\CoreTests_Hashes.sarif" />
+    <EmbeddedResource Include="TestData\InsertOptionalDataVisitor\ExpectedOutputs\CoreTests_RegionSnippets.sarif" />
+    <EmbeddedResource Include="TestData\InsertOptionalDataVisitor\ExpectedOutputs\CoreTests_TextFiles.sarif" />
+    <EmbeddedResource Include="TestData\InsertOptionalDataVisitor\Inputs\CoreTests.sarif" />
     <EmbeddedResource Include="TestData\PrereleaseCompatibilityTransformer\ExpectedOutputs\NestedFiles.sarif" />
     <EmbeddedResource Include="TestData\PrereleaseCompatibilityTransformer\Inputs\NestedFiles.sarif" />
     <EmbeddedResource Include="TestData\SarifCurrentToVersionOneVisitor\ExpectedOutputs\Minimum.sarif" />