Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement MSML_ExtendBaseTestClass (Test classes should be derived from BaseTestClass) #4746

Merged
merged 1 commit into from Jan 31, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 49 additions & 0 deletions .editorconfig
Expand Up @@ -26,3 +26,52 @@ dotnet_diagnostic.MSML_ParameterLocalVarName.severity = none

# MSML_TypeParamName: Type parameter name not standard
dotnet_diagnostic.MSML_TypeParamName.severity = none

[test/Microsoft.ML.CodeAnalyzer.Tests/**.cs]
# BaseTestClass does not apply for analyzer testing.
# MSML_ExtendBaseTestClass: Test classes should be derived from BaseTestClass
dotnet_diagnostic.MSML_ExtendBaseTestClass.severity = none
Comment on lines +30 to +33
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never want to report BaseTestClass violations in the code analyzer tests. These are "meta" tests for the analyzer itself, and thus follows the patterns for analyzer testing instead of the patterns for ML.NET testing.


[test/Microsoft.Extensions.ML.Tests/**.cs]
# MSML_ExtendBaseTestClass: Test classes should be derived from BaseTestClass
dotnet_diagnostic.MSML_ExtendBaseTestClass.severity = none
Comment on lines +35 to +37
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block disables the MSML_ExtendBaseTestClass rule for source files under test/Microsoft.Extensions.ML.Tests. This is required because one or more classes in this folder is considered a test class by this analyzer but does not extend BaseTestClass. However, this pull request only implements the analyzer; a future pull request will remove these lines from the configuration file and update the test classes to correct the violations.


[test/Microsoft.ML.Tests/**.cs]
# MSML_ExtendBaseTestClass: Test classes should be derived from BaseTestClass
dotnet_diagnostic.MSML_ExtendBaseTestClass.severity = none

[test/Microsoft.ML.AutoML.Tests/**.cs]
# MSML_ExtendBaseTestClass: Test classes should be derived from BaseTestClass
dotnet_diagnostic.MSML_ExtendBaseTestClass.severity = none

[test/Microsoft.ML.Benchmarks.Tests/**.cs]
# MSML_ExtendBaseTestClass: Test classes should be derived from BaseTestClass
dotnet_diagnostic.MSML_ExtendBaseTestClass.severity = none

[test/Microsoft.ML.CodeGenerator.Tests/**.cs]
# MSML_ExtendBaseTestClass: Test classes should be derived from BaseTestClass
dotnet_diagnostic.MSML_ExtendBaseTestClass.severity = none

[test/Microsoft.ML.Core.Tests/**.cs]
# MSML_ExtendBaseTestClass: Test classes should be derived from BaseTestClass
dotnet_diagnostic.MSML_ExtendBaseTestClass.severity = none

[test/Microsoft.ML.CpuMath.UnitTests/**.cs]
# MSML_ExtendBaseTestClass: Test classes should be derived from BaseTestClass
dotnet_diagnostic.MSML_ExtendBaseTestClass.severity = none

[test/Microsoft.ML.Functional.Tests/**.cs]
# MSML_ExtendBaseTestClass: Test classes should be derived from BaseTestClass
dotnet_diagnostic.MSML_ExtendBaseTestClass.severity = none

[test/Microsoft.ML.Predictor.Tests/**.cs]
# MSML_ExtendBaseTestClass: Test classes should be derived from BaseTestClass
dotnet_diagnostic.MSML_ExtendBaseTestClass.severity = none

[test/Microsoft.ML.Sweeper.Tests/**.cs]
# MSML_ExtendBaseTestClass: Test classes should be derived from BaseTestClass
dotnet_diagnostic.MSML_ExtendBaseTestClass.severity = none

[test/Microsoft.ML.TimeSeries.Tests/**.cs]
# MSML_ExtendBaseTestClass: Test classes should be derived from BaseTestClass
dotnet_diagnostic.MSML_ExtendBaseTestClass.severity = none
108 changes: 108 additions & 0 deletions test/Microsoft.ML.CodeAnalyzer.Tests/Code/BaseTestClassTest.cs
@@ -0,0 +1,108 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Immutable;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Testing;
using Xunit;
using VerifyCS = Microsoft.ML.CodeAnalyzer.Tests.Helpers.CSharpCodeFixVerifier<
Microsoft.ML.InternalCodeAnalyzer.BaseTestClassAnalyzer,
Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>;

namespace Microsoft.ML.CodeAnalyzer.Tests.Code
{
public class BaseTestClassTest
{
private static readonly ReferenceAssemblies ReferenceAssemblies = ReferenceAssemblies.Default
.AddPackages(ImmutableArray.Create(new PackageIdentity("xunit", "2.4.0")));

[Fact]
public async Task TestClassWithFact()
{
var code = @"
using Xunit;

public class [|SomeClass|] {
[Fact]
public void TestMethod() { }
}
";

await new VerifyCS.Test
{
ReferenceAssemblies = ReferenceAssemblies,
TestState = { Sources = { code } },
}.RunAsync();
}

[Fact]
public async Task TestClassWithTheory()
{
var code = @"
using Xunit;

public class [|SomeClass|] {
[Theory, InlineData(0)]
public void TestMethod(int arg) { }
}
";

await new VerifyCS.Test
{
ReferenceAssemblies = ReferenceAssemblies,
TestState = { Sources = { code } },
}.RunAsync();
}

[Fact]
public async Task TestDirectlyExtendsBaseTestClass()
{
var code = @"
using Microsoft.ML.TestFramework;
using Xunit;

public class SomeClass : BaseTestClass {
[Fact]
public void TestMethod() { }
}

namespace Microsoft.ML.TestFramework {
public class BaseTestClass { }
}
";

await new VerifyCS.Test
{
ReferenceAssemblies = ReferenceAssemblies,
TestState = { Sources = { code } },
}.RunAsync();
}

[Fact]
public async Task TestIndirectlyExtendsBaseTestClass()
{
var code = @"
using Microsoft.ML.TestFramework;
using Xunit;

public class SomeClass : IntermediateClass {
[Fact]
public void TestMethod() { }
}

public abstract class IntermediateClass : BaseTestClass { }

namespace Microsoft.ML.TestFramework {
public class BaseTestClass { }
}
";

await new VerifyCS.Test
{
ReferenceAssemblies = ReferenceAssemblies,
TestState = { Sources = { code } },
}.RunAsync();
}
}
}
134 changes: 134 additions & 0 deletions tools-local/Microsoft.ML.InternalCodeAnalyzer/BaseTestClassAnalyzer.cs
@@ -0,0 +1,134 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Concurrent;
using System.Collections.Immutable;
using System.Diagnostics;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;

namespace Microsoft.ML.InternalCodeAnalyzer
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class BaseTestClassAnalyzer : DiagnosticAnalyzer
{
private const string Category = "Test";
internal const string DiagnosticId = "MSML_ExtendBaseTestClass";

private const string Title = "Test classes should be derived from BaseTestClass";
private const string Format = "Test class '{0}' should extend BaseTestClass.";
private const string Description =
"Test classes should be derived from BaseTestClass.";

private static DiagnosticDescriptor Rule =
new DiagnosticDescriptor(DiagnosticId, Title, Format, Category,
DiagnosticSeverity.Warning, isEnabledByDefault: true, description: Description);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);

public override void Initialize(AnalysisContext context)
{
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None is the normal behavior for analyzers, but it is not the default behavior. It is a combination of two things:

  • This analyzer does not need to analyze generated code (i.e. it will not receive Named Type symbol callbacks for types defined in generated code files, such as *.Designer.cs files). Some analyzers (e.g. analysis of unused code) will not be accurate if generated code is skipped, but this analyzer is not impacted.
  • This analyzer does not report diagnostics in generated code.

context.EnableConcurrentExecution();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This analyzer is safe for concurrent execution. In particular, AnalyzerImpl.AnalyzeNamedType can be called concurrently for different types.


context.RegisterCompilationStartAction(AnalyzeCompilation);
}

private void AnalyzeCompilation(CompilationStartAnalysisContext context)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method sets up the analysis for a compilation, which is the compilation of one assembly (normally one project, but can occur more than once for a project if you multi-target).

{
if (!(context.Compilation.GetTypeByMetadataName("Xunit.FactAttribute") is { } factAttribute))
{
return;
}
Comment on lines +40 to +43
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the project doesn't reference Xunit.FactAttribute, then return early because there are no test classes of interest.


var analyzerImpl = new AnalyzerImpl(context.Compilation, factAttribute);
context.RegisterSymbolAction(analyzerImpl.AnalyzeNamedType, SymbolKind.NamedType);
Comment on lines +45 to +46
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

analyzerImpl is a state object in the context of one Compilation. Analyzers which require state (in this case to cache the symbol lookups of FactAttribute and BaseTestClass) can do so in one of the *StartAnalysisContext callbacks.

}

private sealed class AnalyzerImpl
{
private readonly Compilation _compilation;
private readonly INamedTypeSymbol _factAttribute;
private readonly INamedTypeSymbol _baseTestClass;
private readonly ConcurrentDictionary<INamedTypeSymbol, bool> _knownTestAttributes = new ConcurrentDictionary<INamedTypeSymbol, bool>();

public AnalyzerImpl(Compilation compilation, INamedTypeSymbol factAttribute)
{
_compilation = compilation;
_factAttribute = factAttribute;
_baseTestClass = _compilation.GetTypeByMetadataName("Microsoft.ML.TestFramework.BaseTestClass");
}

public void AnalyzeNamedType(SymbolAnalysisContext context)
{
var namedType = (INamedTypeSymbol)context.Symbol;
if (namedType.TypeKind != TypeKind.Class)
return;
Comment on lines +65 to +67
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is called for all named types. We know the symbol is a INamedTypeSymbol, but we don't know what kind of named type it is. Since we only want to analyze classes, we check that and return immediately for any other type kind.


if (ExtendsBaseTestClass(namedType))
return;
Comment on lines +69 to +70
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid looking at all the members if the class already extends BaseTestClass.


var hasTestMethod = false;
foreach (var member in namedType.GetMembers())
{
if (member is IMethodSymbol method && IsTestMethod(method))
{
hasTestMethod = true;
break;
}
}

if (!hasTestMethod)
return;
Comment on lines +72 to +83
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the class does not have any test methods, it does not need to extend BaseTestClass.


context.ReportDiagnostic(Diagnostic.Create(Rule, namedType.Locations[0], namedType));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, we have a class with test methods, but the class did not extend BaseTestClass. We report a diagnostic instance, using the descriptor Rule defined above. The location is the identifier for the class, and the third parameter (namedType) is the message argument for the {0} in the message format.

}

private bool IsTestMethod(IMethodSymbol method)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test method is a method with a test attribute

{
foreach (var attribute in method.GetAttributes())
{
if (IsTestAttribute(attribute.AttributeClass))
return true;
}

return false;
}

private bool IsTestAttribute(INamedTypeSymbol attributeClass)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test attribute is any attribute that extends Xunit.FactAttribute

{
if (_knownTestAttributes.TryGetValue(attributeClass, out var isTest))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cache the "is test attribute" result for each attribute we encounter, since a few types of attributes tend to be used in many locations.

return isTest;

return _knownTestAttributes.GetOrAdd(attributeClass, ExtendsFactAttribute(attributeClass));
}

private bool ExtendsBaseTestClass(INamedTypeSymbol namedType)
{
if (_baseTestClass is null)
return false;

for (var current = namedType; current is object; current = current.BaseType)
{
if (Equals(current, _baseTestClass))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Symbols need to be compared with Equals (as opposed to ==). We have an analyzer for this to help developers avoid mistakes.

return true;
}

return false;
}

private bool ExtendsFactAttribute(INamedTypeSymbol namedType)
{
Debug.Assert(_factAttribute is object);
for (var current = namedType; current is object; current = current.BaseType)
{
if (Equals(current, _factAttribute))
return true;
}

return false;
}
}
}
}