dotnet / roslyn-sdk

Roslyn-SDK templates and Syntax Visualizer
MIT License
512 stars 257 forks source link

New testing framework does not deal with 1 analyzer: n codefix scenario as expected #424

Open taori opened 4 years ago

taori commented 4 years ago

@sharwell

In my github repo in this branch and the method FixCommentsOnClassTests.SimpleRemoval i ran into an issue with my test case. The ordinary short syntax does not work and it appears like the inherited CodeFixTest FixedState.ExpectedDiagnostics is not being used.

I suppose this might be related to the scenario that i have one analyzer which spawns multiple diagnostics, while one codefix will remove the occurence of all reported diagnostics.

When debugging the testing framework i can see, that it expects just one of the diagnostics to be gone, instead of both like specified in the unit test.

See:

            var codeFixTest = new CodeFixTest<CommentAnalyzer, FixByRemovingClassComments>()
            {
                TestBehaviors = TestBehaviors.SkipGeneratedCodeCheck,
                CompilerDiagnostics = CompilerDiagnostics.Errors,
                TestState =
                {
                    Sources = {test},
                    ExpectedDiagnostics =
                    {
                        // Test0.cs(9,11): info ACA0005: Comments can be removed from this namespace.
                        Verifier.Diagnostic(CommentAnalyzer.NamespaceRule).WithSpan(9, 11, 9, 30),
// Test0.cs(11,11): info ACA0002: Comments can be removed from this class.
                        Verifier.Diagnostic(CommentAnalyzer.ClassRule).WithSpan(11, 11, 11, 19)
                    },
                },
                FixedState =
                {
                    ExpectedDiagnostics =
                    {
                    },
                    Sources = {fixtest}
                },
            };
            await codeFixTest.RunAsync();

After a brief debugging session it appears that the issue might be that at the following place fixedState is being used instead of the FixedState property.

https://github.com/dotnet/roslyn-sdk/blob/869a894010b0d41b7175a13c6758e65ee3d79f6c/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.CodeFix.Testing/CodeFixTest%601.cs#L211

sharwell commented 4 years ago

If FixedState.ExpectedDiagnostics is empty (like you have in your test), the default behavior is to derive the expected diagnostics from the analyzer and/or code fix. In cases where you want to explicitly indicate that the expected diagnostics is empty (inheritance does not work), you can set FixedState.InheritanceMode to StateInheritanceMode.Explicit.

taori commented 4 years ago

What do you think about adding some sort of detection to the code to point this out to the developer? I know this is an early stage of the component and i might have overlooked some docs, but some as mentioned in another issue, pointers help a lot + less questions here 😄

Setting it to Explicit fixes my issue though! so thanks for that!

sharwell commented 4 years ago

No objection to anything that can make it easier for developers to figure things out

taori commented 4 years ago

Is that something you would want to do yourself once you find the time or is that a "help wanted" kind of thing? :)

sharwell commented 4 years ago

If you want to, please feel free 😄

If it's trivial to implement you can just submit a PR and we can discuss it there. If it's something more complicated and you want to confirm the design before finishing the implementation, you can propose the specifics here.