From b5e6778ff144c81dd37c9f29cc8ab15ec18407ef Mon Sep 17 00:00:00 2001 From: Marc Paine Date: Fri, 15 Nov 2024 15:49:37 -0800 Subject: [PATCH 1/6] Switch to spliting our test work items by method instead of class. Let's see if this works. --- test/HelixTasks/AssemblyScheduler.cs | 133 +++++++++++---------------- 1 file changed, 54 insertions(+), 79 deletions(-) diff --git a/test/HelixTasks/AssemblyScheduler.cs b/test/HelixTasks/AssemblyScheduler.cs index 5a6e909f4cda..a85a3ee11aec 100644 --- a/test/HelixTasks/AssemblyScheduler.cs +++ b/test/HelixTasks/AssemblyScheduler.cs @@ -4,6 +4,7 @@ using System.Reflection; using System.Reflection.Metadata; using System.Reflection.PortableExecutable; +using System.Text; namespace Microsoft.DotNet.SdkCustomHelix.Sdk { @@ -93,13 +94,19 @@ private void Build(List typeInfoList) foreach (var typeInfo in typeInfoList) { _currentTypeInfoList.Add(typeInfo); + if (_netFramework) + { + if (_builder.Length > 0) + { + _builder.Append("|"); + } + _builder.Append($@"{typeInfo.FullName}"); - if (_builder.Length > 0) + } + else { - _builder.Append("|"); + _builder.Append($@"-class ""{typeInfo.FullName}"" "); } - _builder.Append($@"{typeInfo.FullName}"); - CheckForPartitionLimit(done: false); } @@ -211,22 +218,25 @@ private static List GetTypeInfoList(string assemblyPath) private static List GetTypeInfoList(MetadataReader reader) { var list = new List(); - foreach (var handle in reader.TypeDefinitions) + foreach (var handle in reader.MethodDefinitions) { - var type = reader.GetTypeDefinition(handle); - if (!IsValidIdentifier(reader, type.Name)) + var method = reader.GetMethodDefinition(handle); + + var name = reader.GetString(method.Name); + if (!IsValidIdentifier(reader, name)) { continue; } - var methodCount = GetMethodCount(reader, type); - if (!ShouldIncludeType(reader, type, methodCount)) + if (!ShouldIncludeMethod(reader, method)) { continue; } - var fullName = GetFullName(reader, type); - list.Add(new TypeInfo(fullName, methodCount)); + var type = reader.GetTypeDefinition(method.GetDeclaringType()); + var fullName = GetFullName(reader, type) + "." + name; + list.Add(new TypeInfo(fullName, 1)); + } // Ensure we get classes back in a deterministic order. @@ -235,69 +245,47 @@ private static List GetTypeInfoList(MetadataReader reader) } /// - /// Determine if this type should be one of the class values passed to xunit. This - /// code doesn't actually resolve base types or trace through inherrited Fact attributes - /// hence we have to error on the side of including types with no tests vs. excluding them. + /// Determine if this method method values passed to xunit. This doesn't check for skipping + /// Include any tests with the known theory and fact attributes /// - private static bool ShouldIncludeType(MetadataReader reader, TypeDefinition type, int testMethodCount) + private static bool ShouldIncludeMethod(MetadataReader reader, MethodDefinition method) { - // xunit only handles public, non-abstract, non-generic classes - var isPublic = - TypeAttributes.Public == (type.Attributes & TypeAttributes.VisibilityMask) || - TypeAttributes.NestedPublic == (type.Attributes & TypeAttributes.VisibilityMask); - if (!isPublic || - TypeAttributes.Abstract == (type.Attributes & TypeAttributes.Abstract) || - type.GetGenericParameters().Count != 0 || - TypeAttributes.Class != (type.Attributes & TypeAttributes.ClassSemanticsMask)) - { - return false; - } + var methodAttributes = method.GetCustomAttributes(); + bool isTestMethod = false; - // Compiler generated types / methods have the shape of the heuristic that we are looking - // at here. Filter them out as well. - if (!IsValidIdentifier(reader, type.Name)) - { - return false; - } - - if (testMethodCount > 0) - { - return true; - } - - // The case we still have to consider at this point is a class with 0 defined methods, - // inheritting from a class with > 0 defined test methods. That is a completely valid - // xunit scenario. For now we're just going to exclude types that inherit from object - // because they clearly don't fit that category. - return !(InheritsFromObject(reader, type) ?? false); - } - - private static int GetMethodCount(MetadataReader reader, TypeDefinition type) - { - var count = 0; - foreach (var handle in type.GetMethods()) - { - var methodDefinition = reader.GetMethodDefinition(handle); - if (methodDefinition.GetCustomAttributes().Count == 0 || - !IsValidIdentifier(reader, methodDefinition.Name)) + foreach (var attributeHandle in methodAttributes) { - continue; - } - - if (MethodAttributes.Public != (methodDefinition.Attributes & MethodAttributes.Public)) - { - continue; + var attribute = reader.GetCustomAttribute(attributeHandle); + var attributeConstructor = reader.GetMemberReference((MemberReferenceHandle)attribute.Constructor); + var attributeType = reader.GetTypeReference((TypeReferenceHandle)attributeConstructor.Parent); + var attributeTypeName = reader.GetString(attributeType.Name); + + if (attributeTypeName == "FactAttribute" || + attributeTypeName == "TheoryAttribute" || + attributeTypeName == "CoreMSBuildAndWindowsOnlyFactAttribute" || + attributeTypeName == "CoreMSBuildAndWindowsOnlyTheoryAttribute" || + attributeTypeName == "CoreMSBuildOnlyFactAttribute" || + attributeTypeName == "CoreMSBuildOnlyTheoryAttribute" || + attributeTypeName == "FullMSBuildOnlyFactAttribute" || + attributeTypeName == "FullMSBuildOnlyTheoryAttribute" || + attributeTypeName == "PlatformSpecificFact" || + attributeTypeName == "PlatformSpecificTheory" || + attributeTypeName == "RequiresMSBuildVersionFactAttribute" || + attributeTypeName == "RequiresMSBuildVersionTheoryAttribute" || + attributeTypeName == "RequiresSpecificFrameworkFactAttribute" || + attributeTypeName == "RequiresSpecificFrameworkTheoryAttribute" || + attributeTypeName == "WindowsOnlyRequiresMSBuildVersionFactAttribute" || + attributeTypeName == "WindowsOnlyRequiresMSBuildVersionTheoryAttribute") + { + isTestMethod = true; + break; + } } - - count++; - } - - return count; + return isTestMethod; } - private static bool IsValidIdentifier(MetadataReader reader, StringHandle handle) + private static bool IsValidIdentifier(MetadataReader reader, String name) { - var name = reader.GetString(handle); for (int i = 0; i < name.Length; i++) { switch (name[i]) @@ -312,19 +300,6 @@ private static bool IsValidIdentifier(MetadataReader reader, StringHandle handle return true; } - private static bool? InheritsFromObject(MetadataReader reader, TypeDefinition type) - { - if (type.BaseType.Kind != HandleKind.TypeReference) - { - return null; - } - - var typeRef = reader.GetTypeReference((TypeReferenceHandle)type.BaseType); - return - reader.GetString(typeRef.Namespace) == "System" && - reader.GetString(typeRef.Name) == "Object"; - } - private static string GetFullName(MetadataReader reader, TypeDefinition type) { var typeName = reader.GetString(type.Name); From 26963c0969a8778c59b4c500c4669df3a8bbafad Mon Sep 17 00:00:00 2001 From: Marc Paine Date: Mon, 16 Dec 2024 16:21:18 -0800 Subject: [PATCH 2/6] Fix the cast exception --- test/HelixTasks/AssemblyScheduler.cs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/test/HelixTasks/AssemblyScheduler.cs b/test/HelixTasks/AssemblyScheduler.cs index a85a3ee11aec..b30360ae1ad6 100644 --- a/test/HelixTasks/AssemblyScheduler.cs +++ b/test/HelixTasks/AssemblyScheduler.cs @@ -256,7 +256,16 @@ private static bool ShouldIncludeMethod(MetadataReader reader, MethodDefinition foreach (var attributeHandle in methodAttributes) { var attribute = reader.GetCustomAttribute(attributeHandle); - var attributeConstructor = reader.GetMemberReference((MemberReferenceHandle)attribute.Constructor); + var attributeConstructorHandle = attribute.Constructor; + MemberReference attributeConstructor; + if (attributeConstructorHandle.Kind == HandleKind.MemberReference) + { + attributeConstructor = reader.GetMemberReference((MemberReferenceHandle)attributeConstructorHandle); + } + else + { + continue; + } var attributeType = reader.GetTypeReference((TypeReferenceHandle)attributeConstructor.Parent); var attributeTypeName = reader.GetString(attributeType.Name); From b5b8a6cdf50f937de68316d728e7d3d3013f4759 Mon Sep 17 00:00:00 2001 From: Marc Paine Date: Wed, 18 Dec 2024 09:47:38 -0800 Subject: [PATCH 3/6] Fix merge issues --- test/HelixTasks/AssemblyScheduler.cs | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/test/HelixTasks/AssemblyScheduler.cs b/test/HelixTasks/AssemblyScheduler.cs index b30360ae1ad6..adba62610a04 100644 --- a/test/HelixTasks/AssemblyScheduler.cs +++ b/test/HelixTasks/AssemblyScheduler.cs @@ -79,7 +79,7 @@ private AssemblyInfoBuilder(string assemblyPath, int methodLimit) _methodLimit = methodLimit; } - internal static void Build(string assemblyPath, int methodLimit, List typeInfoList, out List partitionList, out List assemblyInfoList, bool netFramework = false) + internal static void Build(string assemblyPath, int methodLimit, List typeInfoList, out List partitionList, out List assemblyInfoList) { var builder = new AssemblyInfoBuilder(assemblyPath, methodLimit); builder.Build(typeInfoList); @@ -94,19 +94,7 @@ private void Build(List typeInfoList) foreach (var typeInfo in typeInfoList) { _currentTypeInfoList.Add(typeInfo); - if (_netFramework) - { - if (_builder.Length > 0) - { - _builder.Append("|"); - } - _builder.Append($@"{typeInfo.FullName}"); - - } - else - { - _builder.Append($@"-class ""{typeInfo.FullName}"" "); - } + _builder.Append($@"-class ""{typeInfo.FullName}"" "); CheckForPartitionLimit(done: false); } @@ -182,12 +170,12 @@ internal IEnumerable Schedule(IEnumerable assembl return list; } - public IEnumerable Schedule(string assemblyPath, bool force = false, bool netFramework = false) + public IEnumerable Schedule(string assemblyPath, bool force = false) { var typeInfoList = GetTypeInfoList(assemblyPath); var assemblyInfoList = new List(); var partitionList = new List(); - AssemblyInfoBuilder.Build(assemblyPath, _methodLimit, typeInfoList, out partitionList, out assemblyInfoList, netFramework); + AssemblyInfoBuilder.Build(assemblyPath, _methodLimit, typeInfoList, out partitionList, out assemblyInfoList; // If the scheduling didn't actually produce multiple partition then send back an unpartitioned // representation. From f68de148b222288cb85a147ae916481d57d19f86 Mon Sep 17 00:00:00 2001 From: Marc Paine Date: Wed, 18 Dec 2024 16:16:32 -0800 Subject: [PATCH 4/6] Fix typo --- test/HelixTasks/AssemblyScheduler.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/HelixTasks/AssemblyScheduler.cs b/test/HelixTasks/AssemblyScheduler.cs index adba62610a04..cdcd3f463727 100644 --- a/test/HelixTasks/AssemblyScheduler.cs +++ b/test/HelixTasks/AssemblyScheduler.cs @@ -1,4 +1,4 @@ -// Licensed to the .NET Foundation under one or more agreements. +// Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. using System.Reflection; @@ -175,7 +175,7 @@ public IEnumerable Schedule(string assemblyPath, bool for var typeInfoList = GetTypeInfoList(assemblyPath); var assemblyInfoList = new List(); var partitionList = new List(); - AssemblyInfoBuilder.Build(assemblyPath, _methodLimit, typeInfoList, out partitionList, out assemblyInfoList; + AssemblyInfoBuilder.Build(assemblyPath, _methodLimit, typeInfoList, out partitionList, out assemblyInfoList); // If the scheduling didn't actually produce multiple partition then send back an unpartitioned // representation. From 16e2eb84c02298a22c25f51e5814bcb18aae25a4 Mon Sep 17 00:00:00 2001 From: Marc Paine Date: Fri, 10 Jan 2025 16:43:42 -0800 Subject: [PATCH 5/6] Add correct dotnet test splitter --- test/HelixTasks/AssemblyScheduler.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/HelixTasks/AssemblyScheduler.cs b/test/HelixTasks/AssemblyScheduler.cs index cdcd3f463727..77122bd71118 100644 --- a/test/HelixTasks/AssemblyScheduler.cs +++ b/test/HelixTasks/AssemblyScheduler.cs @@ -94,7 +94,11 @@ private void Build(List typeInfoList) foreach (var typeInfo in typeInfoList) { _currentTypeInfoList.Add(typeInfo); - _builder.Append($@"-class ""{typeInfo.FullName}"" "); + if(_builder.Length > 0) + { + var separator = "|"; + } + _builder.Append($@"{separator}{typeInfo.FullName}"); CheckForPartitionLimit(done: false); } From 50baef45475064835a155dbc5768358aaf801eb0 Mon Sep 17 00:00:00 2001 From: Marc Paine Date: Mon, 13 Jan 2025 09:29:00 -0800 Subject: [PATCH 6/6] Fix the separator logic --- test/HelixTasks/AssemblyScheduler.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/HelixTasks/AssemblyScheduler.cs b/test/HelixTasks/AssemblyScheduler.cs index 77122bd71118..cf1ceacabb95 100644 --- a/test/HelixTasks/AssemblyScheduler.cs +++ b/test/HelixTasks/AssemblyScheduler.cs @@ -94,9 +94,10 @@ private void Build(List typeInfoList) foreach (var typeInfo in typeInfoList) { _currentTypeInfoList.Add(typeInfo); + var separator = ""; if(_builder.Length > 0) { - var separator = "|"; + separator = "|"; } _builder.Append($@"{separator}{typeInfo.FullName}"); CheckForPartitionLimit(done: false);