cezarypiatek / RoslynTestKit

A lightweight framework for writing unit tests for Roslyn diagnostic analyzers, code fixes, refactorings and completion providers.
Other
24 stars 7 forks source link

Consider testing all `Diagnostic`s that may be returned by certain diagnostic_id on CodeFix testing #19

Open ledjon-behluli opened 1 year ago

ledjon-behluli commented 1 year ago

Issue

Right now the protected methods CodeFixTestFixture.TestCodeFix accept: markupCode, expected, and the diagnosticId to locate the Diagnostics occurring in markupCode based on diagnosticId (see: GetDiagnostic(Document document, string diagnosticId, IDiagnosticLocator locator).

The underlying method GetReportedDiagnostics returns correctly all the Diagnostics for a given diagnosticId & IDiagnosticLocator but only the first available Diagnostic is being tested against, this results in failing tests

Example

Suppose we write an analyzer that checks on the ctor arguments of MyClass. We check that any types of System.Type passed as an argument is NOT a valid arg. It means the analyzer will report a Diagnostic foreach of the unsupported args.

Case 1: One offending argument type (works)

[Fact]
    public void A()
    {
        string markupCode = @"
class MyClass { }
MyClass c = new MyClass(1, [|typeof(int)|], 'a');";

        string expected = @"
class MyClass { }
MyClass c = new MyClass(1, 'a');";

        TestCodeFix(markupCode, expected, "ID");
    }

Case 2: Two or more offending argument types - Using one "locator" (doesnt work)

[Fact]
    public void A()
    {
        string markupCode = @"
class MyClass { }
MyClass c = new MyClass(1, [|typeof(int), 'a', typeof(string)|]);";   // See how [||] wraps both parts of expected diagnostics

        string expected = @"
class MyClass { }
MyClass c = new MyClass(1, 'a');
";

        TestCodeFix(markupCode, expected, "ID");  // Result is: MyClass c = new MyClass(1, 'a', [|typeof(string)|]);
    }

Case 3: Two or more offending argument types - Using two or more "locator"'s [||] (doesnt work)

[Fact]
    public void A()
    {
        string markupCode = @"
class MyClass { }
MyClass c = new MyClass(1, [|typeof(int)|], 'a', [|typeof(string)|]);   // See how two [||] wrap, each their expected diagnostic
";

        string expected = @"
class MyClass { }
MyClass c = new MyClass(1, 'a');
";

        TestCodeFix(markupCode, expected, "ID");  // Result is: MyClass c = new MyClass(1, 'a', [|typeof(string)|]);
    }

Proposal

As I see it, this can be fixed in two ways:

  1. Keep one IDiagnosticLocator but test all the reported diagnostics - Need to update documentation to tell that if you expect multiple occurrence of a Diagnostic one should wrap the whole code (including parts you don't expect, if they are in between parts you do expect diagnostic's).
  2. Support multiple IDiagnosticLocator a.k.a multiple [||] - Need to update documentation to tell that if you expect multiple occurrence of a Diagnostic one should wrap each offending part within [||]

    I think both would be fine, but if asked I think the 2nd solution would be cleaner and removes potential ambiguity as a user of the library.