Update to Microsoft.CodeAnalysis.Testing #2562

sharwell · 2019-02-14T23:57:55Z

codecov · 2019-02-15T00:36:14Z

Codecov Report

❗ No coverage uploaded for pull request base (master@b2587fa). Click here to learn what that means.
The diff coverage is 94.28%.

@@            Coverage Diff            @@
##             master    #2562   +/-   ##
=========================================
  Coverage          ?   71.45%           
=========================================
  Files             ?      800           
  Lines             ?   141566           
  Branches          ?    16103           
=========================================
  Hits              ?   101155           
  Misses            ?    35967           
  Partials          ?     4444

Flag	Coverage Δ
#Debug	`71.45% <94.28%> (?)`
#production	`67.73% <ø> (?)`
#test	`85.73% <94.28%> (?)`

Ivanidzo4ka · 2019-02-15T00:42:45Z

build/Dependencies.props

@@ -42,6 +42,7 @@
  <!-- Test-only Dependencies -->
  <PropertyGroup>
    <BenchmarkDotNetVersion>0.11.3</BenchmarkDotNetVersion>
+    <MicrosoftCodeAnalysisTestingVersion>1.0.0-beta1-63812-02</MicrosoftCodeAnalysisTestingVersion>


I don't know anything regarding analyzers. Is it normal to use beta package? Is where are stable version available?

This is a dependency we control. We're still stabilizing the API for the testing package (and determining whether breaking changes are allowed in updates after stable) so this is what's currently available. The package performs much more rigorous testing than previous harnesses, which will show up in some of the comments. Several other analyzers internally (dotnet/roslyn-analyzers, Microsoft/vs-threading, Microsoft/VSSDK-Analyzers) and externally (DotNetAnalyzers/StyleCopAnalyzers) are already using this.

src/Microsoft.ML.Analyzer/Properties/AssemblyInfo.cs

sharwell · 2019-02-15T02:59:56Z

test/Microsoft.ML.CodeAnalyzer.Tests/Code/BestFriendOnPublicDeclarationTest.cs

    {
        private readonly Lazy<string> SourceAttribute = TestUtils.LazySource("BestFriendAttribute.cs");
        private readonly Lazy<string> SourceDeclaration = TestUtils.LazySource("BestFriendOnPublicDeclaration.cs");

        [Fact]
-        public void BestFriendOnPublicDeclaration()
+        public async Task BestFriendOnPublicDeclaration()


📝 I kept the test names the same, which means you'll keep your pass/fail history on ADO. If you prefer the Async suffix convention even in test code, I'm happy to add it.

sharwell · 2019-02-15T03:01:12Z

test/Microsoft.ML.CodeAnalyzer.Tests/Code/ContractsCheckTest.cs

-                diagMsg.CreateDiagnosticResult(basis + 32, 35, "Check", "\"Less fine: \" + env.GetType().Name"),
-                diagName.CreateDiagnosticResult(basis + 34, 17, "CheckUserArg", "name", "\"p\""),
-                diagDecode.CreateDiagnosticResult(basis + 39, 41, "CheckDecode", "\"This message is suspicious\""),
+                new DiagnosticResult("CS0051", DiagnosticSeverity.Error).WithLocation(14, 16).WithMessage("Inconsistent accessibility: parameter type 'IHostEnvironment' is less accessible than method 'TypeName.TypeName(IHostEnvironment, float, int)'"),


📝 It was not clear to me if this was a desired error in the test code. It could indicate that the test input has become stale relative to some API and needs to be updated.

Very interesting. Absolutely not intended, these may have crept in as the input source files changed. Anyway it should be fixed, though perhaps not part of this PR, since this is about using the proper test harness, and we can hardly expect you to fix problems with our faulty tests. Anyway, I have filed a separate issue #2588 to capture this work. My comment would be the same for all these "compilation error" additions you needed to add, absolutely not the intent of the class. But thank you for finding this, this is definitely something we must do!

sharwell · 2019-02-15T03:01:46Z

test/Microsoft.ML.CodeAnalyzer.Tests/Code/ContractsCheckTest.cs

+                VerifyCS.Diagnostic(ContractsCheckAnalyzer.NameofDiagnostic.Rule).WithLocation(basis + 34, 17).WithArguments("CheckUserArg", "name", "\"p\""),
+                VerifyCS.Diagnostic(ContractsCheckAnalyzer.DecodeMessageWithLoadContextDiagnostic.Rule).WithLocation(basis + 39, 41).WithArguments("CheckDecode", "\"This message is suspicious\""),
+                new DiagnosticResult("CS0117", DiagnosticSeverity.Error).WithLocation("Test1.cs", 220, 70).WithMessage("'MessageSensitivity' does not contain a definition for 'UserData'"),
+                new DiagnosticResult("CS0117", DiagnosticSeverity.Error).WithLocation("Test1.cs", 231, 70).WithMessage("'MessageSensitivity' does not contain a definition for 'Schema'"),


📝 It's not clear these two errors were intended to be part of the test input. If the test input needs to be updated please let me know.

sharwell · 2019-02-15T03:02:31Z

test/Microsoft.ML.CodeAnalyzer.Tests/Code/InstanceInitializerTest.cs

@@ -23,22 +27,19 @@ class TypeName
        private static int _muck = 4;
        private readonly float _blorg = 3.0f;
        private string _fooBacking;
-        public string Foo { get; set => _fooBacking = value; }
+        public string Foo { get => _fooBacking; set => _fooBacking = value; }


📝 I went ahead and fixed this compilation error in a test input. It seemed unintentional.

sharwell · 2019-02-15T03:04:14Z

test/Microsoft.ML.CodeAnalyzer.Tests/Code/NameTest.cs

+            {
+                TestCode = TestSource,
+                FixedCode = FixedTestSource,
+                NumberOfFixAllIterations = 2,


📝 This means the application of a Fix All did not actually fix all instances at once. The expected FixedTestSource was reached, but it took two Fix All iterations to reach. The test harness forces you to call out these cases because they indicate places where a code fix works but could have improved usability.

sharwell · 2019-02-15T03:06:04Z

test/Microsoft.ML.CodeAnalyzer.Tests/TypeIsSchemaShapeTest.cs

-            VerifyCSharpDiagnostic(Source, expected);
+            var test = new VerifyCS.Test { TestCode = Source };
+            test.ExpectedDiagnostics.AddRange(expected);
+            test.Exclusions &= ~AnalysisExclusions.GeneratedCode;


📝 This means the analyzer reports diagnostics even in code marked as generated (most commonly due to // <auto-generated/> in the file header, but also handles certain designer files, etc.). If this was not intentional, we should update the analyzer to indicate these files should be excluded from diagnostic reporting.

Some other analyzers in this test suite require this as well. Please review all of them to ensure exclusions are applied/not applied where you want.

sharwell · 2019-02-15T03:06:44Z

test/Microsoft.ML.CodeAnalyzer.Tests/TypeIsSchemaShapeTest.cs

-                diagTp.CreateDiagnosticResult(41, 24, "T"),
-                diag.CreateDiagnosticResult(55, 26, p("text")),
+                VerifyCS.Diagnostic(TypeIsSchemaShapeAnalyzer.ShapeParameterDiagnostic.Rule).WithLocation(18, 24).WithArguments("T"),
+                new DiagnosticResult("CS8205", DiagnosticSeverity.Error).WithLocation(21, 52).WithMessage("Attributes are not allowed on local function parameters or type parameters"),


📝 This seems like a bug in the test input, but I wasn't sure if you wanted me to go ahead and fix it.

sharwell · 2019-02-15T03:08:12Z

test/Microsoft.ML.CodeAnalyzer.Tests/TypeIsSchemaShapeTest.cs

-                diagPg.CreateDiagnosticResult(38, 13, "Class9", "F2"),
-                diagPc.CreateDiagnosticResult(39, 13, "Class10"),
-                diagPc.CreateDiagnosticResult(40, 13, "Class11"),
+                new DiagnosticResult("CS0246", DiagnosticSeverity.Error).WithLocation(44, 71).WithMessage("The type or namespace name 'MissingClass' could not be found (are you missing a using directive or an assembly reference?)"),


📝 Based on the name, it seems this could be intentional. If it's not, let me know.

sharwell · 2019-02-15T03:10:42Z

I'll fix the build error in the morning. Looks like dependency version on CI is slightly different from local but shouldn't be bad to fix.

Fixes dotnet#2480

TomFinley

Thank you @sharwell for lending your expertise!! I'm quite glad to have an expert in this area improving this part of our infrastructure.

sharwell · 2019-02-16T01:35:33Z

@TomFinley no problem! I helped write this test library following experiences with a few other analyzers. It tests a lot of different aspects, and by default it tries to test many scenarios relevant to the most common analyzer designs (things like verifying that generated code is excluded by default). I'm sure questions will come up as you continue to use it, and I'm happy to help figure things out and/or make improvements to the library so it's easier to work with.

shauheen

Ivanidzo4ka reviewed Feb 15, 2019

View reviewed changes

sharwell commented Feb 15, 2019

View reviewed changes

Update to Microsoft.CodeAnalysis.Testing

b97ac32

Fixes dotnet#2480

sharwell force-pushed the use-testing-library branch from 837b077 to b97ac32 Compare February 15, 2019 18:10

TomFinley mentioned this pull request Feb 16, 2019

Some Roslyn analyzer tests have unexpected compilation errors #2588

Open

TomFinley approved these changes Feb 16, 2019

View reviewed changes

shauheen approved these changes Feb 16, 2019

View reviewed changes

TomFinley merged commit 369a9b6 into dotnet:master Feb 19, 2019

sharwell deleted the use-testing-library branch February 19, 2019 07:19

ghost locked as resolved and limited conversation to collaborators Mar 24, 2022

Update to Microsoft.CodeAnalysis.Testing #2562

Update to Microsoft.CodeAnalysis.Testing #2562

Uh oh!

Conversation

sharwell commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell commented Feb 15, 2019

Uh oh!

TomFinley left a comment

Choose a reason for hiding this comment

Uh oh!

sharwell commented Feb 16, 2019

Uh oh!

shauheen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sharwell commented Feb 14, 2019 •

edited

Loading

codecov bot commented Feb 15, 2019 •

edited

Loading

sharwell Feb 15, 2019 •

edited

Loading