-
Notifications
You must be signed in to change notification settings - Fork 126
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
Analyzer testing restructure #2077
Comments
I'm not sure I follow what the goal is. Are we trying to move the RequiresAssemblyFiles tests into the I expected we would have three sets of tests:
That seems like it would produce a lot of sharing, but also a lot of flexibility. What else would this proposal solve? |
So in the case of the test suite mentioned in number 2, at this moment it can only run with the RequiresUnreferencedCode analyzer since the linker tests don't have any [RequiresAssemblyFiles]. Moreover, if the linker tests had RequiresAssemblyFiles attribute on them, there is no mechanism to use ExpectedWarning in such a way that Analyzer warns but linker doesn't. |
Sure, the functionality actually makes complete sense, I just don't know which tests we would want to move over. Do you have an example? |
I think the main goal here is to stop writing analyzer-specific tests and use as many linker tests as we can to validate the analyzer diagnostics; basically, stop having all those
For instance, we could get rid of all tests here and, if we extend public enum ProducedBy {
LinkerOnly,
AnalyzerOnly,
LinkerAndAnalyzer
}
public ExpectedWarningAttribute (string warningCode,
params string[] messageContains, ProducedBy p = ProducedBy.LinkerAndAnalyzer) // linker/test/Mono.Linker.Tests.Cases/ATestThatIsHappilyLivingHere.cs
// This would cause the linker result checker to ignore this.
[ExpectedWarning("IL3002", "", ProucedBy.AnalyzerOnly)]
[RequiresAssemblyFiles]
void SomeMethodTestedByLinker()
{
} Furthermore, we should also look at what can be simplified in the linker test infra. For instance, we could move from |
Got it, the hope is to move over most of the tests to the linker format. Seems reasonable to me. |
Extending what Mateo said if we look at the RequieresUnreferencedCodeAnalyzerTests file at this moment contains basically the following:
Ideally, we would like to have a test in the linker for even these non-covered scenarios and maybe have a comment next to the [ExpectedWarning] explaining why the diagnostic is only produced by the analyzer, or the issue tracking the fix. Which would leave us with a test file only for codefixers. In the case of RAF it becomes more complicated because we would have tests in the linker that appear not to do anything, but then when running the analyzer test suite they will generate a diagnostic (whatever uses properties/events). At this moment the RequiresAssemblyFilesAnalyzerTests file does not include all the RequiresUnreferencedCodeCapability tests, it would be a duplicate if we did, but at the same time we are missing some of the scenarios like TypeIsBeforeFieldInit |
Interestingly I recently added basically one half of this: [ExpectedWarning("IL2026", GlobalAnalysisOnly = true)] // This will validate in linker tests, but will be skipped by analyzer tests
void TestMethod () {} I don't see a problem expanding this to support all combinations (both, global only, local only, ...) Trim analysis warnings (RUC + DAM):
Single-file analysis warnings (RAF):
I'm not 100% sold on the idea of having Single-file tests in the linker test suites, but it does make sense to share some of the tests around RUC and RAF as those two attribute should have very similar behavior. I would like this to be localized to just a couple of places and clearly documented (with comments). |
discussed in dotnet#2077 Now interfaces matching are lookup on types instead of asking per member Added support for specifying in Expected Warning and LogContains where the diagnostic is spected to be produced by default is LinkerAndAnalyzer Renaming methods and classes to specify Requires instead of RequiresUnreferencedCode Merge branch 'main' of https://github.com/mono/linker into MatchOverrideAttributes
Add Explicit Interface testing
Closed via #2336 |
Given the increase in the number of tests for the different analyzers, we have now repeated several of the tests to cover different attribute usages. In general, we have two approaches to reduce the number of repeated tests:
The proposal is an idea originally by @mateoatr to modify the ExpectedWarning attribute to be able to differentiate between a warning that is generated by the linker, the analyzer, or both. This way we could have analyzer validation inside linker tests and cover analyzer scenarios by reusing tests in the linker. Consider the following example:
The result checker in the linker will check if the ExpectedTypes.Linker is active, otherwise; it would disregard the warning. We could do something similar for the analyzer testing infrastructure to test only what is marked with ExpectedTypes.Analyzer. Explaining the code scenario the warning IL2026 would be expected for both the analyzer and the linker, meaning that both testing infrastructures would check for the warning being generated. In the case of IL3002 the result checker in linker will bypass the diagnostic but the test infrastructure of the analyzer will check for the diagnostic.
This would simplify the testing since there would be only 1 testing file in the linker for requirescapability that tests RAF and RUC on both linker and analyzer.
There would be still analyzer testing since the CodeFixers cannot be tested in the linker infrastructure.
The text was updated successfully, but these errors were encountered: