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

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

Merged
merged 6 commits into from
Jan 15, 2019
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e39fb52
Update insert optional data tests and update indices visitor.
michaelcfanning Jan 14, 2019
0e55d7c
Delete speculatively needed file
michaelcfanning Jan 14, 2019
a77802b
Remove erroneous override of base visit method.
michaelcfanning Jan 14, 2019
e8c2252
Merge branch 'files-array' into tests-fixup
Jan 15, 2019
1e0f22f
Rework summary comment on DeletesOutputsDirectoryOnClassInitializatio…
Jan 15, 2019
e6768ff
Merge branch 'files-array' into tests-fixup
michaelcfanning Jan 15, 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
5 changes: 4 additions & 1 deletion src/Sarif.Converters.UnitTests/FortifyFprConverterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 { }
Copy link
Member Author

@michaelcfanning michaelcfanning Jan 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

DeletesOutputsDirectoryOnClassInitializationFixture [](start = 55, length = 51)

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

Copy link

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)

Copy link
Member Author

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)

Copy link

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)

Copy link

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:

FortifyFprConverterTests : FileDiffingTests, IClassFixture<DeletesOutputsDirectoryOnClassInitializationFixture>
{
    ...
}

In reply to: 247662456 [](ancestors = 247662456,247661342,247657934,247381989)

Copy link

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

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 with IClassFixture<DeletesOutputsDirectoryOnClassInitializationFixture>.

In reply to: 247724801 [](ancestors = 247724801,247662456,247661342,247657934,247381989)

Copy link
Member Author

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)


protected override string TestLogResourceNameRoot => "Microsoft.CodeAnalysis.Sarif.Converters.UnitTests.TestData." + TypeUnderTest;

public FortifyFprConverterTests(ITestOutputHelper outputHelper) : base(outputHelper) { }
Expand Down
32 changes: 32 additions & 0 deletions src/Sarif.TestUtilities/DeletesOutputsDirectoryOnClassInitializationFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright (c) Microsoft. All rights reserved. Licensed under the MIT
// license. See LICENSE file in the project root for full license information.

using System;
using System.IO;

namespace Microsoft.CodeAnalysis.Sarif.TestUtilities
{
/// <summary>
/// This class is invoked a single time on initializing an xunit class for testing. It deletes
/// any existing test output files that may have been produced by the previous run. This fixture
/// is required because individual tests in test classes result in an object instantiation for
/// each test case. The FileDiffingTests pattern, by design outputs a bundle of outputs to
Copy link

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

FileDiffingTests [](start = 28, length = 16)

Since this fixture is intended to support classes that derive from FileDiffingTests, why not do this:

class FileDiffingTests: IClassFixture<DeletesOutputsDirectoryOnClassInitializationFixture>
{
    ...
}

class FortifyFprConverterTests: FileDiffingTests
{
    ...
}

... rather than marking each derived class as implementing IClassFixture. Does XUnit's reflection not find the interface if it's on a base class? #Resolved

Copy link

Choose a reason for hiding this comment

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

This might relate to the comment I made on FortifyFprConverterTests, where I asked why you bothered to derive a class from DeletesOutputsDirectoryOnClassInitializationFixture. It's true that if you want to leave the flexibility for each derived test class to have its own fixture, then you can't put the marker interface on the base class.

If that was your reasoning, it's worth a comment here.

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

Copy link

Choose a reason for hiding this comment

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

Ok, I see now that DeletesOutputsDirectoryOnClassInitializationFixture has some virtual properties that derived classes can override, so you really do intend each derived class to have a shot at it. I've been reworking this comment, so I'll mention that.

In reply to: 247725737 [](ancestors = 247725737,247725332)

Copy link

Choose a reason for hiding this comment

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

Ok, I updated this comment with my accustomed crystalline authorial voice :-)

In reply to: 247729832 [](ancestors = 247729832,247725737,247725332)

/// a common directory, to allow diffing an entire directory of failing tests. If each test
/// case deleted this directory, the results would be that only the last test failure would
/// exist in the common outputs location
/// </summary>
public abstract class DeletesOutputsDirectoryOnClassInitializationFixture
Copy link

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

DeletesOutputsDirectoryOnClassInitializationFixture [](start = 26, length = 51)

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 DeletesOutputDirectoryClassFixture (or even OutputDirectoryDeletingClassFixture). #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.

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);
}
}
}
}
26 changes: 11 additions & 15 deletions src/Sarif.TestUtilities/FileDiffingTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;
using Newtonsoft.Json.Serialization;
using Xunit;
using Xunit.Abstractions;

namespace Microsoft.CodeAnalysis.Sarif
Expand All @@ -34,12 +35,6 @@ public static string GetProductTestDataDirectory(string subdirectory = "")
public FileDiffingTests(ITestOutputHelper outputHelper)
{
_outputHelper = outputHelper;

if (Directory.Exists(OutputFolderPath))
{
Directory.Delete(OutputFolderPath, recursive: true);
}

Directory.CreateDirectory(OutputFolderPath);
}

Expand All @@ -53,37 +48,38 @@ public FileDiffingTests(ITestOutputHelper outputHelper)

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)
Copy link
Member Author

@michaelcfanning michaelcfanning Jan 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

RunTest [](start = 31, length = 7)

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

Copy link

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

glad to discuss further.

In reply to: 247659167 [](ancestors = 247659167,247382063)

{
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);
PrereleaseCompatibilityTransformer.UpdateToCurrentVersion(expectedSarifText, forceUpdate: false, Formatting.Indented, out expectedSarifText);

if (!AreEquivalentSarifLogs<SarifLog>(actualSarifText, expectedSarifText))
{
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 = null;
string actualFilePath = null;

expectedFilePath = GetOutputFilePath("ExpectedOutputs", resourceName);
actualFilePath = GetOutputFilePath("ActualOutputs", resourceName);
expectedFilePath = GetOutputFilePath("ExpectedOutputs", expectedOutputResourceName);
actualFilePath = GetOutputFilePath("ActualOutputs", expectedOutputResourceName);

string expectedRootDirectory = Path.GetDirectoryName(expectedFilePath);
string actualRootDirectory = Path.GetDirectoryName(actualFilePath);
Expand All @@ -105,7 +101,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);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Sarif.UnitTests/Processors/Log/SarifLogExtensionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =>
Expand Down
18 changes: 18 additions & 0 deletions src/Sarif.UnitTests/Sarif.UnitTests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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\v1\Minimum.sarif" />
Expand Down Expand Up @@ -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\v1\Minimum.sarif" />
Expand Down
Loading