Fix TODOs and update unit tests with diagnostics #5287

pgovind · 2021-07-23T23:32:35Z

Addresses 3 of 4 points in #5227.

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DetectPreviewFeatureAnalyzer.cs

buyaa-n · 2021-07-26T18:04:00Z

src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/DetectPreviewFeatureUnitTests.cs

@@ -59,7 +56,7 @@ public class Program
        static void Main(string[] args)
        {
            var a = new Fraction();
-            var b = [|+a|];
+            var b = {|CA2252:+a|};


Are the different rules ids for distinguishing diagnostics? You can verify with the diagnostic descriptor instead

roslyn-analyzers/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/AvoidZeroLengthArrayAllocationsTests.cs

Line 118 in 682876f

VerifyCS.Diagnostic(AvoidZeroLengthArrayAllocationsAnalyzer.UseArrayEmptyDescriptor).WithLocation(8, 22).WithArguments("Array.Empty<int>()"),

Yup I added the new ruleIDs so I could use the easy markup syntax for the unit tests. Multiple diagnostics defined in the same rule ID didn't seem to work well with the markup syntax.

Do you feel strongly about having only 1 diagnostic ID here? Is it wrong to use multiple diagnostic IDs in 1 analyzer? If I have to change to using VerifyCS.Diagnostic, I have to modify every unit test with the row and column numbers. Seems like a waste of time?

Is it wrong to use multiple diagnostic IDs in 1 analyzer?

Is there a reason a developer would want to configure them independently, e.g. different warning levels or some enabled and some not? If not, we should have only one. We shouldn't add additional IDs just for testing purposes.

Is there a reason a developer would want to configure them independently

I can't think of a natural reason for this. I'll change to using VerifyCS.Diagnostic

buyaa-n

I don't see any reason using different diagnostics id for this analyzer

pgovind · 2021-07-26T22:33:56Z

I don't see any reason using different diagnostics id for this analyzer

Fixed now, this can be reviewed again. Hopefully this fixed the weird CI issue too

codecov · 2021-07-26T23:05:20Z

Codecov Report

Merging #5287 (5a245e5) into release/6.0.1xx (3897eb2) will increase coverage by 0.01%.
The diff coverage is 99.08%.

@@                 Coverage Diff                 @@
##           release/6.0.1xx    #5287      +/-   ##
===================================================
+ Coverage            95.60%   95.62%   +0.01%     
===================================================
  Files                 1233     1233              
  Lines               283395   283449      +54     
  Branches             16971    16968       -3     
===================================================
+ Hits                270939   271047     +108     
+ Misses               10162    10108      -54     
  Partials              2294     2294

buyaa-n

Left some NIT comments for styling, overall LGTM

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DetectPreviewFeatureAnalyzer.cs

src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/DetectPreviewFeatureUnitTests.cs

pgovind · 2021-07-27T22:35:04Z

Thanks for the review @buyaa-n

Fix TODOs and update unit tests with diagnostics

682876f

pgovind requested a review from a team as a code owner July 23, 2021 23:32

pgovind requested a review from buyaa-n July 23, 2021 23:32