Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve coreclr test infra #77929

Merged
merged 3 commits into from
Nov 8, 2022
Merged

Improve coreclr test infra #77929

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
869211f
Improve coreclr test infra
davidwrighton Nov 4, 2022
c49ef39
Add some documentation about project files for coreclr tests, as well…
davidwrighton Nov 7, 2022
064ca9b
Fix type
davidwrighton Nov 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/tests/Common/CoreCLRTestLibrary/PlatformDetection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,11 @@ public static class PlatformDetection
&& (AppContext.TryGetSwitch("System.Runtime.InteropServices.BuiltInComInterop.IsSupported", out bool isEnabled)
? isEnabled
: true);

static string _variant = Environment.GetEnvironmentVariable("DOTNET_RUNTIME_VARIANT");

public static bool IsMonoLLVMAOT => _variant == "llvmaot";
public static bool IsMonoLLVMFULLAOT => _variant == "llvmfullaot";
public static bool IsMonoInterpreter => _variant == "monointerpreter";
}
}
75 changes: 66 additions & 9 deletions src/tests/Common/XUnitWrapperGenerator/XUnitWrapperGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,22 +154,51 @@ private static string GenerateFullTestRunner(ImmutableArray<ITestInfo> testInfos
builder.AppendLine(string.Join("\n", aliasMap.Values.Where(alias => alias != "global").Select(alias => $"extern alias {alias};")));
builder.AppendLine("System.Collections.Generic.HashSet<string> testExclusionList = XUnitWrapperLibrary.TestFilter.LoadTestExclusionList();");

builder.AppendLine("XUnitWrapperLibrary.TestFilter filter = new (args.Length != 0 ? args[0] : null, testExclusionList);");
builder.AppendLine("XUnitWrapperLibrary.TestFilter filter = new (args, testExclusionList);");
builder.AppendLine("XUnitWrapperLibrary.TestSummary summary = new();");
builder.AppendLine("System.Diagnostics.Stopwatch stopwatch = System.Diagnostics.Stopwatch.StartNew();");
builder.AppendLine("XUnitWrapperLibrary.TestOutputRecorder outputRecorder = new(System.Console.Out);");
builder.AppendLine("System.Console.SetOut(outputRecorder);");

ITestReporterWrapper reporter = new WrapperLibraryTestSummaryReporting("summary", "filter", "outputRecorder");

foreach (ITestInfo test in testInfos)
StringBuilder testExecutorBuilder = new();
int testsLeftInCurrentTestExecutor = 0;
int currentTestExecutor = 0;
int totalTestsEmitted = 0;

if (testInfos.Length > 0)
{
builder.AppendLine(test.GenerateTestExecution(reporter));
// Break tests into groups of 50 so that we don't create an unreasonably large main method
// Excessively large methods are known to take a long time to compile, and use excessive stack
// leading to test failures.
foreach (ITestInfo test in testInfos)
{
if (testsLeftInCurrentTestExecutor == 0)
{
if (currentTestExecutor != 0)
testExecutorBuilder.AppendLine("}");
currentTestExecutor++;
testExecutorBuilder.AppendLine($"void TestExecutor{currentTestExecutor}(){{");
builder.AppendLine($"TestExecutor{currentTestExecutor}();");
testsLeftInCurrentTestExecutor = 50; // Break test executors into groups of 50, which empircally seems to work well
}
testExecutorBuilder.AppendLine(test.GenerateTestExecution(reporter));
totalTestsEmitted++;
testsLeftInCurrentTestExecutor--;
}
testExecutorBuilder.AppendLine("}");
}

builder.AppendLine($@"System.IO.File.WriteAllText(""{assemblyName}.testResults.xml"", summary.GetTestResultOutput(""{assemblyName}""));");
builder.AppendLine($@"string testResults = summary.GetTestResultOutput(""{assemblyName}"");");
builder.AppendLine($@"string workitemUploadRoot = System.Environment.GetEnvironmentVariable(""HELIX_WORKITEM_UPLOAD_ROOT"");");
builder.AppendLine($@"if (workitemUploadRoot != null) System.IO.File.WriteAllText(System.IO.Path.Combine(workitemUploadRoot, ""{assemblyName}.testResults.xml.txt""), testResults);");
builder.AppendLine($@"System.IO.File.WriteAllText(""{assemblyName}.testResults.xml"", testResults);");
builder.AppendLine("return 100;");

builder.Append(testExecutorBuilder);

builder.AppendLine("public static class TestCount { public const int Count = " + totalTestsEmitted.ToString() + "; }");
return builder.ToString();
}

Expand All @@ -193,14 +222,37 @@ private static string GenerateXHarnessTestRunner(ImmutableArray<ITestInfo> testI

ITestReporterWrapper reporter = new WrapperLibraryTestSummaryReporting("summary", "filter", "outputRecorder");

foreach (ITestInfo test in testInfos)
StringBuilder testExecutorBuilder = new();
int testsLeftInCurrentTestExecutor = 0;
int currentTestExecutor = 0;

if (testInfos.Length > 0)
{
builder.AppendLine(test.GenerateTestExecution(reporter));
// Break tests into groups of 50 so that we don't create an unreasonably large main method
// Excessively large methods are known to take a long time to compile, and use excessive stack
// leading to test failures.
foreach (ITestInfo test in testInfos)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit challenging as we long-term envision we should emit something like a watchdog app that would run itself as a child process and execute the tests starting at a given index; this is needed especially for GCStress tests where a hard crash or timeout in a particular test basically drops all coverage on the floor; it would be great to collect the XUnit summaries and allow restarting the run at the next test by the parent process. I think this is good for now, I can imagine how to adapt this to the described scenario.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm, yes. We should possibly consider adding yet another parameter with the test index to start with, and we can tie that into the test striping logic. That should be workable, but we have enough changes put together here in this PR that I don't want to add that right now.

{
if (testsLeftInCurrentTestExecutor == 0)
{
if (currentTestExecutor != 0)
testExecutorBuilder.AppendLine("}");
currentTestExecutor++;
testExecutorBuilder.AppendLine($"static void TestExecutor{currentTestExecutor}(XUnitWrapperLibrary.TestSummary summary, XUnitWrapperLibrary.TestFilter filter, XUnitWrapperLibrary.TestOutputRecorder outputRecorder, System.Diagnostics.Stopwatch stopwatch){{");
builder.AppendLine($"TestExecutor{currentTestExecutor}(summary, filter, outputRecorder, stopwatch);");
testsLeftInCurrentTestExecutor = 50; // Break test executors into groups of 50, which empirically seems to work well
}
testExecutorBuilder.AppendLine(test.GenerateTestExecution(reporter));
testsLeftInCurrentTestExecutor--;
}
testExecutorBuilder.AppendLine("}");
}

builder.AppendLine("return summary;");
builder.AppendLine("}");

builder.Append(testExecutorBuilder);

return builder.ToString();
}

Expand Down Expand Up @@ -354,7 +406,8 @@ private static IEnumerable<ITestInfo> GetTestMethodInfosForMethod(IMethodSymbol
testInfos,
conditionType,
conditionMembers,
aliasMap[conditionType.ContainingAssembly.MetadataName]);
aliasMap[conditionType.ContainingAssembly.MetadataName],
false /* do not negate the condition, as this attribute indicates that a test will be run */);
break;
}
case "Xunit.OuterloopAttribute":
Expand All @@ -373,7 +426,8 @@ private static IEnumerable<ITestInfo> GetTestMethodInfosForMethod(IMethodSymbol
testInfos,
conditionType,
filterAttribute.ConstructorArguments[2].Values,
aliasMap[conditionType.ContainingAssembly.MetadataName]);
aliasMap[conditionType.ContainingAssembly.MetadataName],
true /* negate the condition, as this attribute indicates that a test will NOT be run */);
break;
}
else if (filterAttribute.AttributeConstructor.Parameters.Length == 4)
Expand Down Expand Up @@ -667,9 +721,12 @@ private static ImmutableArray<ITestInfo> DecorateWithUserDefinedCondition(
ImmutableArray<ITestInfo> testInfos,
ITypeSymbol conditionType,
ImmutableArray<TypedConstant> values,
string externAlias)
string externAlias,
bool negate)
{
string condition = string.Join("&&", values.Select(v => $"{externAlias}::{conditionType.ToDisplayString(FullyQualifiedWithoutGlobalNamespace)}.{v.Value}"));
if (negate)
condition = $"!({condition})";
return ImmutableArray.CreateRange<ITestInfo>(testInfos.Select(m => new ConditionalTest(m, condition)));
}

Expand Down
59 changes: 53 additions & 6 deletions src/tests/Common/XUnitWrapperLibrary/TestFilter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,14 +118,50 @@ public override string ToString()
// issues.targets file as a split model would be very confusing for developers
// and test monitors.
private readonly HashSet<string>? _testExclusionList;
private readonly int _stripe = 0;
private readonly int _stripeCount = 1;
private int _shouldRunQuery = -1;

public TestFilter(string? filterString, HashSet<string>? testExclusionList)
public TestFilter(string? filterString, HashSet<string>? testExclusionList) :
this(filterString == null ? Array.Empty<string>() : new string[]{filterString}, testExclusionList)
{
}

public TestFilter(string[] filterArgs, HashSet<string>? testExclusionList)
{
string? filterString = null;

for (int i = 0; i < filterArgs.Length; i++)
{
if (filterArgs[i].StartsWith("-stripe"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth starting a markdown file or something for us to start documenting the command line options the merged runners support, especially as we continue to add more options.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, actually it would be probably also useful to have an md on the overall concepts of merged tests like the stuff we presented a couple of times in the past; I can try to put some basics together but I'll probably need your additional commits for more detailed explanation of stuff I'm not that familiar with; I'd expect the documentation of command-line options to the merged runners can be easily included in the doc.

{
_stripe = Int32.Parse(filterArgs[++i]);
_stripeCount = Int32.Parse(filterArgs[++i]);
}
else
{
if (filterString == null)
filterString = filterArgs[0];
}
}

var stripeEnvironment = Environment.GetEnvironmentVariable("TEST_HARNESS_STRIPE_TO_EXECUTE");
if (!String.IsNullOrEmpty(stripeEnvironment) && stripeEnvironment != ".0.1")
{
var stripes = stripeEnvironment.Split('.');
if (stripes.Length == 3)
{
Console.WriteLine($"Test striping enabled via TEST_HARNESS_STRIPE_TO_EXECUTE environment variable set to '{stripeEnvironment}'");
_stripe = Int32.Parse(stripes[1]);
_stripeCount = Int32.Parse(stripes[2]);
}
}

if (filterString is not null)
{
if (filterString.IndexOfAny(new[] { '!', '(', ')', '~', '=' }) != -1)
{
throw new ArgumentException("Complex test filter expressions are not supported today. The only filters supported today are the simple form supported in 'dotnet test --filter' (substrings of the test's fully qualified name). If further filtering options are desired, file an issue on dotnet/runtime for support.", nameof(filterString));
throw new ArgumentException("Complex test filter expressions ar e not supported today. The only filters supported today are the simple form supported in 'dotnet test --filter' (substrings of the test's fully qualified name). If further filtering options are desired, file an issue on dotnet/runtime for support.", nameof(filterString));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - this looks like a typo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new ArgumentException("Complex test filter expressions ar e not supported today. The only filters supported today are the simple form supported in 'dotnet test --filter' (substrings of the test's fully qualified name). If further filtering options are desired, file an issue on dotnet/runtime for support.", nameof(filterString));
throw new ArgumentException("Complex test filter expressions are not supported today. The only filters supported today are the simple form supported in 'dotnet test --filter' (substrings of the test's fully qualified name). If further filtering options are desired, file an issue on dotnet/runtime for support.", nameof(filterString));

}
_filter = new NameClause(TermKind.FullyQualifiedName, filterString, substring: true);
}
Expand All @@ -140,15 +176,26 @@ public TestFilter(ISearchClause? filter, HashSet<string>? testExclusionList)

public bool ShouldRunTest(string fullyQualifiedName, string displayName, string[]? traits = null)
{
bool shouldRun = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - it might be a bit cleaner to define the local variable as uninitialized, if would let the Roslyn compiler verify that it does get initialized in each of the code branches.

if (_testExclusionList is not null && _testExclusionList.Contains(displayName.Replace("\\", "/")))
{
return false;
shouldRun = false;
}
if (_filter is null)
else if (_filter is null)
{
shouldRun = true;
}
else
{
shouldRun = _filter.IsMatch(fullyQualifiedName, displayName, traits ?? Array.Empty<string>());
}

if (shouldRun)
{
return true;
// Test stripe, if true, then report success
return ((System.Threading.Interlocked.Increment(ref _shouldRunQuery)) % _stripeCount) == _stripe;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very clever and cheap way to handle striping. I like it!

}
return _filter.IsMatch(fullyQualifiedName, displayName, traits ?? Array.Empty<string>());
return false;
}

public static HashSet<string> LoadTestExclusionList()
Expand Down
22 changes: 20 additions & 2 deletions src/tests/Common/XUnitWrapperLibrary/TestSummary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,29 @@ public string GetTestResultOutput(string assemblyName)
string outputElement = !string.IsNullOrWhiteSpace(test.Output) ? $"<output><![CDATA[{test.Output}]]></output>" : string.Empty;
if (test.Exception is not null)
{
resultsFile.AppendLine($@"result=""Fail""><failure exception-type=""{test.Exception.GetType()}""><message><![CDATA[{test.Exception.Message}]]></message><stack-trace><![CDATA[{test.Exception.StackTrace}]]></stack-trace></failure>{outputElement}</test>");
string exceptionMessage = test.Exception.Message;
if (test.Exception is System.Reflection.TargetInvocationException tie)
{
if (tie.InnerException != null)
{
exceptionMessage = $"{exceptionMessage} \n INNER EXCEPTION--\n {tie.InnerException.GetType()}--\n{tie.InnerException.Message}--\n{tie.InnerException.StackTrace}";
}
}
if (string.IsNullOrWhiteSpace(exceptionMessage))
{
exceptionMessage = "NoExceptionMessage";
}

string? stackTrace = test.Exception.StackTrace;
if (string.IsNullOrWhiteSpace(stackTrace))
{
stackTrace = "NoStackTrace";
}
resultsFile.AppendLine($@"result=""Fail""><failure exception-type=""{test.Exception.GetType()}""><message><![CDATA[{exceptionMessage}]]></message><stack-trace><![CDATA[{stackTrace}]]></stack-trace></failure>{outputElement}</test>");
}
else if (test.SkipReason is not null)
{
resultsFile.AppendLine($@"result=""Skip""><reason><![CDATA[{test.SkipReason}]]></reason></test>");
resultsFile.AppendLine($@"result=""Skip""><reason><![CDATA[{(!string.IsNullOrWhiteSpace(test.SkipReason) ? test.SkipReason : "No Known Skip Reason")}]]></reason></test>");
}
else
{
Expand Down
Loading