Analyzer testing restructure

[tlakollo]

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:

• Share tests between the linker and the analyzer. Given that ideally the linker and the analyzer should produce the same diagnostics sharing the tests between the linker and analyzer makes sense. The problem is that we can only share tests for things that are tested in the linker repo, this means that we cannot test RAF as part of the linker tests since the linker doesn't conduct a global analysis of this attribute.
• Share tests between analyzers. The RAF and RUC analyzer tests are similar and therefore we can use a base class to share the test between them.

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:

[ExpectedWarning ("IL2026", "Message for --RequiresWithMessageOnly--.")]
// ExpectedTypes includes something like Analyzer, Linker, and All values, All is the default
[ExpectedWarning ("IL3002", ExpectedBy = ExpectedTypes.Analyzer]
static void TestRequiresWithMessageOnlyOnMethod ()
{
	RequiresWithMessageOnly ();
}

[RequiresUnreferencedCode ("Message for --RequiresWithMessageOnly--")]
[RequiresAssemblyFiles]
static void RequiresWithMessageOnly () {}

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.

[tlakollo]

cc @mateoatr @sbomer @agocke @vitek-karas @MichalStrehovsky

[agocke]

I'm not sure I follow what the goal is. Are we trying to move the RequiresAssemblyFiles tests into the RequiresCapability analyzer tests? I don't see why.

I expected we would have three sets of tests:

1. The existing linker tests. We would run the linker and the RequiresUnreferencedCode analyzer over these tests, so the tests would be shared.
2. A RequiresCapability analyzer test suite that is run for both RequiresUnreferencedCode and RequiresAssemblyFiles
3. Two specific analyzer test files for RUC and RAF. This would test things that are specific to each analyzer and shouldn't be shared with anything else.

That seems like it would produce a lot of sharing, but also a lot of flexibility. What else would this proposal solve?

[tlakollo]

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.
If we move the linker test to cover different requires attributes, we would have cases like in the example (one method with two attributes) that test RUC and RAF functionality, but we could also have tests that target only RAF or only RUC. Making kind of unnecessary to have number 3, with the exception of CodeFixers which cannot be tested in the linker.
I think there is already something similar in the recently merged PR #2057, in which we have an attribute that will warn in the linker but not on the analyzer, we would just need the opposite behavior something that can warn on the analyzer but not on the linker.

[agocke]

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?

[mateoatr]

I'm not sure I follow what the goal is.

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 string thisIsMyTestSource = @"... and just use the test that already exist inside linker. This is not so different from what we already do with RequiresCapability where we basically pick up any method definition declared under Mono.Linker.Tests.Cases.RequiresCapability and pass it to the RUC analyzer.

I just don't know which tests we would want to move over. Do you have an example?

For instance, we could get rid of all tests here and, if we extend ExpectedWarning with a property allowing us to specify that a warning is to be expected only from the analyzer/linker, we could also get rid of all tests here and simply use RAF on some linker tests and have them marked with ExpectedWarning[("IL3001", "Oh no", ExpectedByAnalyzerOnly)].

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 UnrecognizedReflectionAccessPattern/RecognizedReflectionAccessPattern and use ExpectedWarning everywhere instead (related: #2066 (comment)).

[agocke]

Got it, the hope is to move over most of the tests to the linker format. Seems reasonable to me.

[tlakollo]

Extending what Mateo said if we look at the RequieresUnreferencedCodeAnalyzerTests file at this moment contains basically the following:

• CodeFixer tests
• Repeated tests from the linker that could get deleted
• Tests not covered by linker (test for which analyzer generates a warning but linker doesn't)

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

[vitek-karas]

Interestingly I recently added basically one half of this:
https://github.com/mono/linker/blob/0f3ea71923fdc93800103ec4d8f59bbe15e2813b/test/Mono.Linker.Tests.Cases.Expectations/Assertions/ExpectedWarningAttribute.cs#L20

[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):

• Both - obviously this is the ideal place to be in - so it should be the default. The warning is produced by both global and local analysis.
• Linker only - this is expected in some cases - specifically tests which target cross-assembly behaviors in the linker. The analyzer will simply not be able to report all warnings since it only sees local view of the program.
• Analyzer only - ideally this should not exist for trim analysis warnings. It would mean that you get a warning during build, but the warning magically goes away when you publish - not a good experience (and hard to explain).

Single-file analysis warnings (RAF):

• Analyzer only - this is the only one which makes sense currently

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).

[tlakollo]

Closed via #2336