Update insert optional data tests and update indices visitor. #1212
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<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 | ||
|
There was a problem hiding this comment.
This is an xUnit class fixture (which, as we now both know, does work on class initialization (and sometimes on class disposal). I suggest the name There was a problem hiding this comment. Our fixture only deletes on class initialization, so would prefer to retain the name. In any case, this fixture is going to be changed, I'm switching to a different pattern as this one is non-obvious and cumbersome to implement. In reply to: 247737869 [](ancestors = 247737869) |
||
| { | ||
| 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); | ||
| } | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
||
| 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 inputResourceName) { return string.Empty; } | ||
|
|
||
| protected virtual bool RebaselineExpectedResults => false; | ||
|
|
||
| protected virtual void RunTest(string inputResourceName, string expectedOutputResourceName = null) | ||
|
There was a problem hiding this comment.
Here's how we handle one-to-many tests. Create an individual test referencing the common input, but generate an alternate output name. See the optional data visitor tests for a specific example. #Closed There was a problem hiding this comment. So if an operation (such as a JSchema code gen from a schema) generates multiple outputs, we actually have to run the operation as many times as there are output files, even though all the outputs are generated each time? I'm not wild about that. In reply to: 247382063 [](ancestors = 247382063) There was a problem hiding this comment. |
||
| { | ||
| 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." + inputResourceName); | ||
|
|
||
| expectedOutputResourceName = Path.GetFileNameWithoutExtension(expectedOutputResourceName) + ".sarif"; | ||
|
|
||
| var sb = new StringBuilder(); | ||
|
|
||
| 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}'.", inputResourceName); | ||
| sb.AppendLine(errorMessage); | ||
|
|
||
| if (!Utilities.RunningInAppVeyor) | ||
| { | ||
| 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, expectedOutputResourceName); | ||
| File.WriteAllText(expectedFilePath, actualSarifText); | ||
| } | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This fixture deletes the outputs directory once per test session. Previously, we deleted this directory on executing every test case. Oops. This means that only the very last test failure was persisted to the directory. #Pending
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.
Oh, nice, I will add that to SarifCurrentToVersionOneVisitorTests.
In reply to: 247381989 [](ancestors = 247381989)
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.
Just want to be clear you understand this pattern. A fixture is called on a class with a public parameterless constructor. it is instantiated a single time on xunit identifying a class with one or more [Fact] or other attribute-derived tests. The test class itself cannot be the fixture class (as xUnit supports only one public ctor on a test class, due to the diversity of parameterizations of these things, I guess. Our fixture derives some computed data (i.e., the test output directory) from our test organization scheme. From the base class. I have recapped this minimal data computation and chosen to embed these fixtures as nested types with the actual test class (since they are not generally repurposable. The fixture must be named identically to the test class with the term 'Fixture' appended to it.
If you can think of anything cleaner/more elegant, let me know. The fixture itself can be provided, btw, to the test class ctor, with the result that it can be additionally parameterized when it is passed to each test case. This parameterization can be used most easily for test cleanup. You could imagine us simply placing a bool property on this thing, though ('HaveIDeletedTheOutputDirectoryYet' and having the first test case note that it's false, perform the delete itself, and set that bool to true. The test fixture is effectively just a bundle of static data with this approach. It would eliminate the requirement to define a nested type for each test (because we could put this standard operation into the file diffing base type). Thoughts?
In reply to: 247657934 [](ancestors = 247657934,247381989)
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.
I'll read up on XUnit's "fixture" support and get back to you.
In reply to: 247661342 [](ancestors = 247661342,247657934,247381989)
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.
I read the XUnit documentation of shared context. Now I know enough to ask:
Why do you derive a class from
DeletesOutputsDirectoryOnClassInitializationFixture? I could understand it if the derived class added some functionality, but it doesn't. Why not just:In reply to: 247662456 [](ancestors = 247662456,247661342,247657934,247381989)
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.
Ok, I'm bouncing back and forth between commenting on the fixture and commenting on this class :-). I now see that the fixture has virtual properties that a
FileDiffingTests-derived class could override in a bespoke derived fixture class. But in cases like this class, that do not override those properties or do anything else, I still think you should just mark them withIClassFixture<DeletesOutputsDirectoryOnClassInitializationFixture>.In reply to: 247724801 [](ancestors = 247724801,247662456,247661342,247657934,247381989)
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.
No, this won't work. These properties are initialized based on class naming convention. A single class can only represent the intended output location of a single suite. I'm just going to change the pattern entirely, stay tuned. Not this change.
In reply to: 247732570 [](ancestors = 247732570,247724801,247662456,247661342,247657934,247381989)