Use Microsoft.CodeAnalysis.Testing for analyzer testing

[sharwell]

This test library provides a more comprehensive set of test features for analyzer authors. The primary barrier to adoption is the initial conversion, which I've done here for the full xunit.analyzers suite.

[sharwell]

I will reopen this pull request after the prerequisite is merged.

[sharwell]

📝 This version has a direct impact on downstream consumers. It should only be updated when necessary to use a new API, and even then alternatives should be used when possible.

[sharwell]

📝 This tool uses a newer version of the compiler to avoid warnings, but it's only a local tool so it doesn't have any impact on downstream consumers.

[bradwilson]

We appear to be losing a significant part of the testing here; namely, that we are validating the code, message, and severity of the check.

[sharwell]

These are unlikely to change in a way that leads to a regression. With that said, there are a few things to keep in mind:

1. Both the ID and the severity are getting tested (it asserts that they match the values provided in the diagnostic descriptor, which is not always a given).
2. It is possible to write a more verbose test that covers any of these explicitly, and/or other situations that matter more for the library such as an assertion that no two analyzer types use the same ID.
3. The new tests add dozens of new assertions covering cases that were not covered at all previously, including but not limited to the spans where the diagnostics are reported.

I'd recommend keeping most tests in this simplified form, and adding a minimal set of new tests to cover cases that might be prone to regression.

[bradwilson]

At the end of the day, I'm not sure I understand what this buys me. Can you tell me what's better about this?

[sharwell]

The new testing library automates the testing of a wide variety of tests relevant to analyzer authors. It's nothing that can't be done in other ways, but it plugs a bunch of test gaps compared to all the other approaches seen to date (by combining their various approaches). The tests themselves are deceptively simple, while providing high confidence in overall behavior. The shared library also helps analyzer authors contribute to each other's projects by offering a unified approach to testing.

[bradwilson]

All development is moving to Linux and VS Code. The C# extension for VS Code only runs .NET Core unit tests. Since this PR disables .NET Core, I think I'm willing to wait until the library catches up to support .NET Core.

[bradwilson]

As I was trying to clean up the source code, I ran across issues like this.

This test:

    [Theory]
    [MemberData(nameof(TypesAndValues))]
    public async void FindsWarningForExpectedConstantOrLiteralValueAsNamedExpectedArgument(string type, string value)
    {
        var source =
@"class TestClass {
    void TestMethod() {
        var v = default(" + type + @");
        Xunit.Assert.Equal(actual: " + value + @", expected: v);
    }
}";
        var expected = Verify.Diagnostic().WithLocation(3, 5).WithArguments(value, "Assert.Equal(expected, actual)", "TestMethod", "TestClass");

        await Verify.VerifyAnalyzerAsync(source, expected);
    }

Failed with this message:

[xUnit.net 00:00:02.22]       Assert.Equal() Failure
[xUnit.net 00:00:02.22]       Expected: 2
[xUnit.net 00:00:02.22]       Actual:   3
[xUnit.net 00:00:02.22]       Stack Trace:
[xUnit.net 00:00:02.22]         /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/XUnitVerifier.cs(48,0): at Microsoft.CodeAnalysis.Testing.Verifiers.XUnitVerifier.Equal[T](T expected, T actual, String message)
[xUnit.net 00:00:02.22]         /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs(346,0): at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.VerifyLinePosition(ImmutableArray`1 analyzers, Diagnostic diagnostic, LinePosition actualLinePosition, LinePosition expectedLinePosition, String positionText, IVerifier verifier)
[xUnit.net 00:00:02.22]         /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs(337,0): at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.VerifyDiagnosticLocation(ImmutableArray`1 analyzers, Diagnostic diagnostic, Location actual, DiagnosticLocation expected, IVerifier verifier)
[xUnit.net 00:00:02.22]         /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs(285,0): at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.VerifyDiagnosticResults(IEnumerable`1 actualResults, ImmutableArray`1 analyzers, DiagnosticResult[] expectedResults, IVerifier verifier)
[xUnit.net 00:00:02.22]         /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs(195,0): at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.VerifyDiagnosticsAsync(ValueTuple`2[] sources, ValueTuple`2[] additionalFiles, ProjectState[] additionalProjects, MetadataReference[] additionalMetadataReferences, DiagnosticResult[] expected, IVerifier verifier, CancellationToken cancellationToken)
[xUnit.net 00:00:02.22]         /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.CodeFix.Testing/CodeFixTest`1.cs(189,0): at Microsoft.CodeAnalysis.Testing.CodeFixTest`1.RunAsync(CancellationToken cancellationToken)
[xUnit.net 00:00:02.22]         c:\Dev\xunit\xunit.analyzers\test\xunit.analyzers.tests\AssertEqualLiteralValueShouldBeFirstTests.cs(92,0): at AssertEqualLiteralValueShouldBeFirstTests.FindsWarningForExpectedConstantOrLiteralValueAsNamedExpectedArgument(String type, String value)

What is the purpose of including this in the assertion message? It doesn't seem to have any useful information in it, save perhaps the final line of the stack trace.

[sharwell]

@bradwilson I'm not sure I understand the question. There are some cases remaining where the internal assertions of the testing library might not make sense to users. We've been fixing those cases as they are found, with the goal of making the failures messages lead directly to the expected solutions. I'm not seeing that test fail locally so I'm not sure what the specific situation here is.

[bradwilson]

@sharwell I'm questioning the value of your assertion messages in your test library when there is a failure. Your assertion message starts out with useful information (like 'expected diagnostic to start on line 3, but started on line 4'), but then some internal xUnit.net assertion is appended to this message (like the one shown above) which is both unnecessary and confusing. Furthermore, the stack trace includes a bunch of unnecessary lines from your testing framework.

This:

Microsoft.CodeAnalysis.Testing.Verifiers.EqualWithMessageException : Context: Diagnostics of test state
Expected diagnostic to start on line "2" was actually on line "3"

Diagnostic:
    // Test0.cs(3,9): warning xUnit2007: Do not use typeof(int) expression to check the type.
GetCSharpResultAt(3, 9, AssertIsTypeShouldUseGenericOverloadType.xUnit2007)


Assert.Equal() Failure
Expected: 1
Actual:   2
    Stack Trace:
       at Microsoft.CodeAnalysis.Testing.Verifiers.XUnitVerifier.Equal[T](T expected, T actual, String message) in /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/XUnitVerifier.cs:line 48
   at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.VerifyLinePosition(ImmutableArray`1 analyzers, Diagnostic diagnostic, LinePosition actualLinePosition, LinePosition expectedLinePosition, String positionText, IVerifier verifier) in /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs:line 346
   at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.VerifyDiagnosticLocation(ImmutableArray`1 analyzers, Diagnostic diagnostic, Location actual, DiagnosticLocation expected, IVerifier verifier) in /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs:line 337
   at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.VerifyDiagnosticResults(IEnumerable`1 actualResults, ImmutableArray`1 analyzers, DiagnosticResult[] expectedResults, IVerifier verifier) in /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs:line 285
   at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.VerifyDiagnosticsAsync(ValueTuple`2[] sources, ValueTuple`2[] additionalFiles, ProjectState[] additionalProjects, MetadataReference[] additionalMetadataReferences, DiagnosticResult[] expected, IVerifier verifier, CancellationToken cancellationToken) in /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs:line 195
   at Microsoft.CodeAnalysis.Testing.CodeFixTest`1.RunAsync(CancellationToken cancellationToken) in /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.CodeFix.Testing/CodeFixTest`1.cs:line 189
   at AssertIsTypeShouldUseGenericOverloadTests.FindsWarning_ForNonGenericCall(String method) in c:\Dev\xunit\xunit.analyzers\test\xunit.analyzers.tests\AssertIsTypeShouldUseGenericOverloadTests.cs:line 21

Would be more useful/less noisy if it looked like this:

Microsoft.CodeAnalysis.Testing.Verifiers.EqualWithMessageException : Context: Diagnostics of test state
Expected diagnostic to start on line "2" was actually on line "3"

Diagnostic:
    // Test0.cs(3,9): warning xUnit2007: Do not use typeof(int) expression to check the type.
GetCSharpResultAt(3, 9, AssertIsTypeShouldUseGenericOverloadType.xUnit2007)

Stack Trace:
   at AssertIsTypeShouldUseGenericOverloadTests.FindsWarning_ForNonGenericCall(String method) in c:\Dev\xunit\xunit.analyzers\test\xunit.analyzers.tests\AssertIsTypeShouldUseGenericOverloadTests.cs:line 21

[sharwell]

#122 (comment)

I would not be opposed to improving the presentation following a failure in this group of assertions. It's one of the more clumsy sets of output. For example, instead of comparing "2" and "3", it might be better to present the comparison as:

 Diagnostic was not at the correct location. Showing expected as baseline:
-Test0.cs(3,9)
+Test0.cs(2,9)

This could use some more brainstorming. Please consider filing an issue for this in dotnet/roslyn-sdk.

[bradwilson]

There are some line-ending issues still remaining with non-Windows (Roslyn code fixers will unilaterally use \r\n when creating line endings, regardless of the value of Environment.NewLine), which I will fix up after this is merged.

[bradwilson]

As for the issue with .NET Framework and Visual Studio Test Explorer, I'm comfortable moving forward without a resolution because (a) netcoreapp2.2 is listed first, so it'll be what VS2017 uses; and (b) .NET Core versions of tests run inside Visual Studio, which should be sufficient for inner loop development work.

I did not open an issue with the VSTest team. I suggest that you do so, since it appears to be at least somewhat related to dependency resolution with your test framework rather than xUnit.net itself that is at issue here.

[bradwilson]

I'm trying to fix up the Linux issues, but non-AppDomains now appears to be broken:

System.MissingMethodException : Method not found: System.Collections.Immutable.ImmutableArray`1<Microsoft.CodeAnalysis.DiagnosticDescriptor> Microsoft.CodeAnalysis.Diagnostics.DiagnosticAnalyzer.get_SupportedDiagnostics()

As does AppDomains with shadow copy turned off:

   System.TypeInitializationException : The type initializer for 'Microsoft.CodeAnalysis.Testing.MetadataReferences' threw an exception.
---- System.IO.FileNotFoundException : Could not load file or assembly 'Microsoft.VisualBasic, Version=10.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies.

I think I'm going to have to revert this for now until these issues can be resolved.

[bradwilson]

I'm planning to split the fixer tests into their own assembly, so I'll apply your rollbacks to the .csproj files at the same time.

[bradwilson]

(This should be a simple rename in git, so your changes to those files should be able to track across the rename.)

[sharwell]

@bradwilson Can you send me a message on Teams?