Test restructure #2113

tlakollo · 2021-06-25T01:40:33Z

Add ProducedBy enum to define where the diagnostic can be generated
ExpectedWarningAttribute and LogContainsAttribute now have a property to define where the diagnostic can be generated using the ProducedBy enum
Modified the different tests environments to support the new property, also test will only warn if the id is supported by the current running analyzer
RunTest function now accepts additional references to be inserted before compilation
RequiresUnreferencedCodeCapability.cs file is now only RequiresCapability.cs since is now agnostic of which kind of RequiresAttribute is testing
Fixes #2077
Fixes #2107

Add ProducedBy enum to define where the diagnostic can be generated ExpectedWarningAttribute and LogContainsAttribute noew have a property to define where the diagnostic can be generated using the ProducedBy enum Modified the differet tests environments to support the new property, also test will only warn if the id is supported by the current running analyzer RunTest function now accepts additional references to be inserted before compilation RequiresUnreferencedCodeCapability.cs file is now only RequiresCapability.cs since is now agnostic of which kind of RequiresAttribute is testing

test file and the linkertestcases file Add function to search for dependency files and transform them into metadatareferences

…cture

…cture Add missing attribute in compiler generated type and method Uncomment the expected warnings

Change names to be more generic

Analyzer test infra now understand attributes in accessors and in properties

the compilergenerated property Fix CompilerGenerated tests

test/ILLink.RoslynAnalyzer.Tests/Verifiers/CSharpAnalyzerVerifier`1.cs

test/ILLink.RoslynAnalyzer.Tests/TestChecker.cs

Update static constructor Compiler Generated conditions to only check for fields and properties

vitek-karas · 2021-07-07T15:17:19Z

test/ILLink.RoslynAnalyzer.Tests/TestCaseUtils.cs

+				case "Dependencies":
+					foreach (var file in Directory.EnumerateFiles (subDir, "*.cs"))
+						yield return file;
+					break;


I guess for now this is OK, but ideally we would react to the same attributes linker tests do. I guess that would require us to parse the test file twice - once to just go over the attributes to detect additional dependencies. And then second time to do the actual test compilation. But it would more precisely match what the linker tests do.

test/ILLink.RoslynAnalyzer.Tests/TestChecker.cs

vitek-karas · 2021-07-07T16:08:45Z

test/Mono.Linker.Tests.Cases/RequiresCapability/Dependencies/RequiresAttributeInCopyAssembly.cs

 		[RequiresUnreferencedCode ("Message for --Method--")]
+		[RequiresAssemblyFiles (Message = "Message for --Method--")]


The fact that the two attributes can't look the same is really not ideal. I filed dotnet/runtime#55274 to track changing it.

I opened #2156 to update linker sdk along with analyzer Microsoft.NetCore.App.Ref package to get the changes made in RAF

vitek-karas · 2021-07-07T16:10:48Z

test/Mono.Linker.Tests.Cases/RequiresCapability/Dependencies/RequiresAttributeInCopyAssembly.cs

 		[RequiresUnreferencedCode ("Message for --UncalledMethod--")]
+		[RequiresAssemblyFiles (Message = "Message for --UncalledMethod--")]


This looks fragile - in that if somebody adds a new test case and forgets to add both attributes we're loosing coverage (and it's really hard to find). Maybe we can "fix" this by:

Adding a unit test which goes over the test classes in the right namespace and validates that if one attribute is there, there's also the second one

Would need some way to tell this test that there are cases where it's intentional that there's a difference.

test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs

test/ILLink.RoslynAnalyzer.Tests/TestChecker.cs

…cture

mateoatr · 2021-07-21T16:06:20Z

test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs

+		bool IsProducedByLinker (CustomAttribute attr)
+		{
+			var propertyObject = attr.GetPropertyValue ("ProducedBy");
+			ProducedBy diagnosticProducedBy = propertyObject is null ? ProducedBy.LinkerAndAnalyzer : (ProducedBy) propertyObject;


Nit:

Suggested change

ProducedBy diagnosticProducedBy = propertyObject is null ? ProducedBy.LinkerAndAnalyzer : (ProducedBy) propertyObject;

return propertyObject is null ? true : ((ProducedBy) propertyObject).HasFlag (ProducedBy.Linker);

mateoatr · 2021-07-21T16:06:34Z

test/ILLink.RoslynAnalyzer.Tests/LinkerTestCases.cs

-		public void RequiresCapability (MethodDeclarationSyntax m, List<AttributeSyntax> attrs)
+		private static Lazy<IEnumerable<MetadataReference>> _additionalReferences = new Lazy<IEnumerable<MetadataReference>> (LoadDependencies);
+
+		private static IEnumerable<MetadataReference> LoadDependencies ()


Why not put this in LinkerTestCases.cs? It feels like GetReferenceFilesByDirName should only be used there and have the dependencies resolved before compilation (ideally through looking at the linker attributes that we use to add assemblies to the compilation).

mateoatr · 2021-07-21T16:06:38Z

test/ILLink.RoslynAnalyzer.Tests/LinkerTestCases.cs

+			List<MetadataReference> additionalReferences = new List<MetadataReference> ();
+			var s_refFiles = GetReferenceFilesByDirName ();
+			foreach (var refFile in s_refFiles["Dependencies"]) {
+				if (refFile.Contains ("RequiresCapability"))


It feels somewhat fragile looking into the string of a filename here.

mateoatr · 2021-07-21T16:06:44Z

test/ILLink.RoslynAnalyzer.Tests/TestCaseUtils.cs

+						var ancestor = attr.Ancestors ().OfType<MemberDeclarationSyntax> ().First ();
+						if (ancestor.IsKind (SyntaxKind.MethodDeclaration) || ancestor.IsKind (SyntaxKind.PropertyDeclaration) || ancestor.IsKind (SyntaxKind.GetAccessorDeclaration) || ancestor.IsKind (SyntaxKind.SetAccessorDeclaration))
+							return true;
+						return false;


Nit: I think it helps readability to have one line between the if () <exp>; <exp>;. I've noticed this pattern in the codebase, although I didn't find any coding style doc (searched in runtime repo) that clearly states this.

mateoatr · 2021-07-21T16:06:49Z

test/ILLink.RoslynAnalyzer.Tests/TestCaseUtils.cs

+		public static async Task<Compilation> GetCompilation (string src)
+		{
+			var srctree = CSharpSyntaxTree.ParseText (src);
+			var mdRef = MetadataReference.CreateFromFile (typeof (Mono.Linker.Tests.Cases.Expectations.Metadata.BaseMetadataAttribute).Assembly.Location);


Nit: Why not have a using and put typeof (BaseMetadataAttribute) here?

mateoatr · 2021-07-21T16:06:51Z

test/ILLink.RoslynAnalyzer.Tests/TestCaseUtils.cs

+		{
+			var srctree = CSharpSyntaxTree.ParseText (src);
+			var mdRef = MetadataReference.CreateFromFile (typeof (Mono.Linker.Tests.Cases.Expectations.Metadata.BaseMetadataAttribute).Assembly.Location);
+			var comp = CSharpCompilation.Create (


This could probably be a good place to call a method that pulls in dependencies. Probably by understanding the SetupCompileBefore/SetupCompileAfter attributes.

mateoatr · 2021-07-21T16:06:55Z

test/ILLink.RoslynAnalyzer.Tests/TestCaseUtils.cs

+			return builder.ToImmutable ();
+		}
+
+		public static IEnumerable<string> GetReferenceFiles ()


This is only called by GetReferenceFilesByDirName, should we collapse these two methods into just one?

mateoatr · 2021-07-21T16:06:57Z

test/ILLink.RoslynAnalyzer.Tests/TestCaseUtils.cs

+		{
+			GetDirectoryPaths (out var rootSourceDir, out _);
+
+			foreach (var subDir in Directory.EnumerateDirectories (rootSourceDir, "*", SearchOption.AllDirectories)) {


Why not use Directory.GetDirectories(rootSourceDir) instead?

mateoatr · 2021-07-21T16:07:02Z

test/ILLink.RoslynAnalyzer.Tests/TestChecker.cs

@@ -56,16 +57,35 @@ public void ValidateAttributes (List<AttributeSyntax> attributes)
 			}
 		}

+		private bool IsProducedByAnalyzer (Dictionary<string, ExpressionSyntax> args)
+		{
+			return !args.TryGetValue ("ProducedBy", out var diagnosticProducedBy) || (


Could we do something similar to what we have in IsProducedByLinker instead of having this very long expression?

…cture

tlakollo · 2021-11-02T20:50:53Z

Closing in favor of #2336

tlakollo added 7 commits June 24, 2021 18:34

Add raf source to testcaseutils so it can be shared between the analyzer

5151793

test file and the linkertestcases file Add function to search for dependency files and transform them into metadatareferences

Merge branch 'main' of https://github.com/mono/linker into TestRestru…

fec8f41

…cture

Add more attributes to RequiresCapability

04b85b5

Merge branch 'main' of https://github.com/mono/linker into TestRestru…

b7c79c9

…cture

Add more RequiresAssemblyFiles tests

6cceb41

Merge branch 'main' of https://github.com/mono/linker into TestRestru…

d35a8e4

…cture Add missing attribute in compiler generated type and method Uncomment the expected warnings

tlakollo mentioned this pull request Jul 1, 2021

RUC analyzer does not warn about delegate conversion #1912

Closed

tlakollo added 2 commits July 1, 2021 13:05

Add lazy delegate tests

cf9c63a

Change names to be more generic

Move mismatching attributes tests to linker

fdaf485

Analyzer test infra now understand attributes in accessors and in properties

tlakollo self-assigned this Jul 2, 2021

tlakollo added 2 commits July 1, 2021 18:42

Create tests that mimic having a reference and source files in linker

d1cac9f

Handle the case in which compiler generates a static constructor with

957777a

the compilergenerated property Fix CompilerGenerated tests

tlakollo marked this pull request as ready for review July 2, 2021 07:22

tlakollo requested a review from marek-safar as a code owner July 2, 2021 07:22

tlakollo requested review from mateoatr and agocke July 2, 2021 07:22

agocke reviewed Jul 6, 2021

View reviewed changes

test/ILLink.RoslynAnalyzer.Tests/Verifiers/CSharpAnalyzerVerifier`1.cs Show resolved Hide resolved

test/ILLink.RoslynAnalyzer.Tests/TestChecker.cs Outdated Show resolved Hide resolved

PR comments

0877a61

Update static constructor Compiler Generated conditions to only check for fields and properties

tlakollo requested a review from vitek-karas July 7, 2021 00:20

vitek-karas reviewed Jul 7, 2021

View reviewed changes

mateoatr reviewed Jul 8, 2021

View reviewed changes

test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs Outdated Show resolved Hide resolved

mateoatr reviewed Jul 8, 2021

View reviewed changes

test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs Outdated Show resolved Hide resolved

mateoatr reviewed Jul 8, 2021

View reviewed changes

test/ILLink.RoslynAnalyzer.Tests/TestChecker.cs Show resolved Hide resolved

mateoatr reviewed Jul 8, 2021

View reviewed changes

test/ILLink.RoslynAnalyzer.Tests/TestChecker.cs Outdated Show resolved Hide resolved

tlakollo added 2 commits July 19, 2021 12:30

Merge branch 'main' of https://github.com/mono/linker into TestRestru…

6d8511e

…cture

PR comments

f1e2409

mateoatr reviewed Jul 21, 2021

View reviewed changes

Merge branch 'main' of https://github.com/mono/linker into TestRestru…

5d52eaf

…cture

mateoatr mentioned this pull request Aug 17, 2021

Warn on RUC annotated attribute ctors (analyzer) #2201

Merged

tlakollo mentioned this pull request Nov 2, 2021

Test Restructure #2336

Merged

tlakollo closed this Nov 2, 2021

tlakollo deleted the TestRestructure branch February 7, 2023 21:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test restructure #2113

Test restructure #2113

tlakollo commented Jun 25, 2021 •

edited

Loading

vitek-karas Jul 7, 2021

vitek-karas Jul 7, 2021

tlakollo Jul 21, 2021

vitek-karas Jul 7, 2021

mateoatr Jul 21, 2021

mateoatr Jul 21, 2021

mateoatr Jul 21, 2021

mateoatr Jul 21, 2021

mateoatr Jul 21, 2021

mateoatr Jul 21, 2021

mateoatr Jul 21, 2021

mateoatr Jul 21, 2021

mateoatr Jul 21, 2021

tlakollo commented Nov 2, 2021

		[RequiresUnreferencedCode ("Message for --Method--")]
		[RequiresAssemblyFiles (Message = "Message for --Method--")]

	ProducedBy diagnosticProducedBy = propertyObject is null ? ProducedBy.LinkerAndAnalyzer : (ProducedBy) propertyObject;
	return propertyObject is null ? true : ((ProducedBy) propertyObject).HasFlag (ProducedBy.Linker);

Test restructure #2113

Test restructure #2113

Conversation

tlakollo commented Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlakollo commented Nov 2, 2021

tlakollo commented Jun 25, 2021 •

edited

Loading