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
Conversation
…om BaseTestClass)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding some comments for @harishsk since this is the first analyzer review
context.RegisterCompilationStartAction(AnalyzeCompilation); | ||
} | ||
|
||
private void AnalyzeCompilation(CompilationStartAnalysisContext context) |
There was a problem hiding this comment.
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; | ||
} |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
var namedType = (INamedTypeSymbol)context.Symbol; | ||
if (namedType.TypeKind != TypeKind.Class) | ||
return; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
.
|
||
for (var current = namedType; current is object; current = current.BaseType) | ||
{ | ||
if (Equals(current, _baseTestClass)) |
There was a problem hiding this comment.
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.
|
||
public override void Initialize(AnalysisContext context) | ||
{ | ||
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); |
There was a problem hiding this comment.
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.
public override void Initialize(AnalysisContext context) | ||
{ | ||
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); | ||
context.EnableConcurrentExecution(); |
There was a problem hiding this comment.
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.
[test/Microsoft.Extensions.ML.Tests/**.cs] | ||
# MSML_ExtendBaseTestClass: Test classes should be derived from BaseTestClass | ||
dotnet_diagnostic.MSML_ExtendBaseTestClass.severity = none |
There was a problem hiding this comment.
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.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 |
There was a problem hiding this comment.
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.
All current violations are excluded from analysis via .editorconfig. After this is merged, someone can enable the diagnostic in one project at a time and ensure there are no violations.