Improve coreclr test infra #77929

davidwrighton · 2022-11-04T23:00:09Z

These fixes were built for PR #74886; however, as that PR is so utterly large and unreviewable, I've pulled out the test infra changes for separate review

Changes

Increase the number of trampolines in the llvm aot compilation process to 20,000 from 10,000 (This avoids running out of them in some of the hardware intrinsics tests
Add a concept of striping tests when running under GC stress
- To use this new feature, specify N within the merged test assembly. If this value is set, then the tests within that merged test assembly will be run across N different work items instead of 1 when running under any form of GC stress based scenario. At this moment the largest supported value of N is 99
Emit the testresults.xml file as a file which is exported from the tests. This is useful for debugging testresult.xml parsing failures
Fix the testresults summary generator to never emit an empty CDATA string. If one is present the parser may fail the parse.
In the XUnitWrapperGenerator fix the implementation of the Outerloop and ActiveIssue when used with a conditional member.
Add PlatformDetection.IsMonoLLVMAOT, PlatformDetection.IsMonoLLVMFULLAOT, and PlatformDetection.IsMonoInterpreter boolean properties to the PlatformDetection type for use with the ActiveIssue attribute

These fixes were built for PR dotnet#74886; however, as that PR is so utterly large an unreviewable, I've pulled out the test infra changes for separate review Changes - Increase the number of trampolines in the llvm aot compilation process to 20,000 from 10,000 (This avoids running out of them in some of the hardware intrinsics tests - Add a concept of striping tests when running under GC stress - To use this new feature, specify <NumberOfStripesToUseInStress>N</NumberOfStripesToUseInStress> within the merged test assembly. If this value is set, then the tests within that merged test assembly will be run across N different work items instead of 1 when running under any form of GC stress based scenario. At this moment the largest supported value of N is 99 - Emit the testresults.xml file as a file which is exported from the tests. This is useful for debugging testresult.xml parsing failures - Fix the testresults summary generator to never emit an empty CDATA string. If one is present the parser may fail the parse. - In the XUnitWrapperGenerator fix the implementation of the Outerloop and ActiveIssue when used with a conditional member. - Add PlatformDetection.IsMonoLLVMAOT, PlatformDetection.IsMonoLLVMFULLAOT, and PlatformDetection.IsMonoInterpreter boolean properties to the PlatformDetection type for use with the ActiveIssue attribute

ghost · 2022-11-04T23:00:39Z

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

These fixes were built for PR #74886; however, as that PR is so utterly large and unreviewable, I've pulled out the test infra changes for separate review

Changes

Increase the number of trampolines in the llvm aot compilation process to 20,000 from 10,000 (This avoids running out of them in some of the hardware intrinsics tests
Add a concept of striping tests when running under GC stress
- To use this new feature, specify N within the merged test assembly. If this value is set, then the tests within that merged test assembly will be run across N different work items instead of 1 when running under any form of GC stress based scenario. At this moment the largest supported value of N is 99
Emit the testresults.xml file as a file which is exported from the tests. This is useful for debugging testresult.xml parsing failures
Fix the testresults summary generator to never emit an empty CDATA string. If one is present the parser may fail the parse.
In the XUnitWrapperGenerator fix the implementation of the Outerloop and ActiveIssue when used with a conditional member.
Add PlatformDetection.IsMonoLLVMAOT, PlatformDetection.IsMonoLLVMFULLAOT, and PlatformDetection.IsMonoInterpreter boolean properties to the PlatformDetection type for use with the ActiveIssue attribute

Author:	davidwrighton
Assignees:	-
Labels:	`area-Infrastructure-coreclr`
Milestone:	-

trylek · 2022-11-04T23:09:18Z

src/tests/Common/XUnitWrapperLibrary/TestFilter.cs

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


Nit - this looks like a typo

jkoritzinsky · 2022-11-04T23:10:39Z

src/tests/Common/XUnitWrapperLibrary/TestFilter.cs

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


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.

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.

jkoritzinsky · 2022-11-04T23:10:50Z

src/tests/Common/XUnitWrapperLibrary/TestFilter.cs

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


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

jkoritzinsky · 2022-11-04T23:11:29Z

src/tests/Common/XUnitWrapperLibrary/TestFilter.cs

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


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

trylek · 2022-11-04T23:12:27Z

src/tests/Common/XUnitWrapperGenerator/XUnitWrapperGenerator.cs

+            // 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)


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.

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.

jkoritzinsky · 2022-11-04T23:13:49Z

src/tests/Directory.Build.targets

+    </PropertyGroup>
+
+    <!-- Generate a set of stress marker files to be used in stress scenarios in CI runs -->
+    <ItemGroup Condition="'$(NumberOfStripesToUseInStress)' != '' and '$(IsMergedTestRunnerAssembly)' == 'true'">


We should add a backlog item to introduce an MSBuild task for this instead of hard-coding a list of 0-99.

trylek · 2022-11-04T23:15:22Z

src/tests/Common/XUnitWrapperLibrary/TestFilter.cs

@@ -140,15 +176,26 @@ public TestFilter(ISearchClause? filter, HashSet<string>? testExclusionList)

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


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.

trylek · 2022-11-04T23:22:29Z

src/tests/Common/mergedrunner.targets

@@ -15,7 +15,7 @@
      Produce an error if any project references were removed due to conflict resolution.
      If a ProjectReference is removed due to conflict resolution, then we're likely losing test coverage as it's probably a test that has the same assembly name and version as another test.
    -->
-    <Error Text="@(_ProjectReferencesRemovedDueToConflictResolution->'This project has an assembly name identical to another project: %(FullPath)', '&#010;')" Condition="'@(_ProjectReferencesRemovedDueToConflictResolution)' != ''"  />
+    <Error Text="@(_ProjectReferencesRemovedDueToConflictResolution->'This project has an assembly name identical to another project, if this CoreCLRTestLibrary, you should reference %24(TestLibraryProjectPath) instead of constructing the path yourself: %(FullPath)', '&#010;')" Condition="'@(_ProjectReferencesRemovedDueToConflictResolution)' != ''"  />


Nit - I'm not sure what to make of the wording 'if this CoreCLRTestLibrary', is the intention "if it's CoreCLRTestLibrary" or something else?

I should have written "if this is CoreCLRTestLibrary"

The errors that happen if you do this wrong, are entirely inscrutable, and extremely difficult to debug.

trylek · 2022-11-04T23:27:52Z

src/tests/Directory.Build.targets

+      <NumberOfStripesToUseInStress>1</NumberOfStripesToUseInStress> <!-- Set a custom number of stripes in stress to allow very large merged groups to function in the presence of GCStress -->
+    </PropertyGroup>
+
+    <!-- Generate a set of stress marker files to be used in stress scenarios in CI runs -->


Could we add a comment describing how the number of SequenceOfIntegers items correlates to the stripe width and total test count as a guidance for future engineers trying to tweak these magic constants? (Nit - for brevity I'd consider rolling several values, e.g. 10, into the same item line using the semicolon separator but that's just my personal feeling, I don't insist and in a way your formulation is more straightforward.)

Ah, so what happens here is that this is used to do some math. There is no correlation here at all, other than the number of stripes must be less than or equal to the maximum number here. I'm writing a small markdown file describing various project file options for use in our test infra, which will describe what a developer can actually use.

Would it be perhaps possible to use your md as the nucleus of the doc I mentioned above i.e. a more general explanation of how merged test runners work, what environment variables and command line options are relevant and so on? I'm certainly not saying you should write all of that, I'm just pointing it out as it might affect your choice of location and naming for the file.

trylek

Looks great to me, thanks David!

… as some documentation on the command line parameters for merged test runner assemblies

davidwrighton requested review from jkoritzinsky and trylek November 4, 2022 23:00

dotnet-issue-labeler bot added the area-Infrastructure-coreclr label Nov 4, 2022

ghost assigned davidwrighton Nov 4, 2022

trylek reviewed Nov 4, 2022

View reviewed changes

jkoritzinsky reviewed Nov 4, 2022

View reviewed changes

trylek reviewed Nov 4, 2022

View reviewed changes

jkoritzinsky reviewed Nov 4, 2022

View reviewed changes

trylek reviewed Nov 4, 2022

View reviewed changes

trylek approved these changes Nov 4, 2022

View reviewed changes

davidwrighton added 2 commits November 7, 2022 11:40

Add some documentation about project files for coreclr tests, as well…

c49ef39

… as some documentation on the command line parameters for merged test runner assemblies

Fix type

064ca9b

jkoritzinsky approved these changes Nov 7, 2022

View reviewed changes

davidwrighton merged commit 939f050 into dotnet:main Nov 8, 2022

ghost locked as resolved and limited conversation to collaborators Dec 8, 2022

davidwrighton deleted the coreclr_test_infra_improvements branch April 13, 2023 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve coreclr test infra #77929

Improve coreclr test infra #77929

davidwrighton commented Nov 4, 2022

ghost commented Nov 4, 2022

trylek Nov 4, 2022

jkoritzinsky Nov 4, 2022

trylek Nov 4, 2022

jkoritzinsky Nov 4, 2022

jkoritzinsky Nov 4, 2022

trylek Nov 4, 2022

davidwrighton Nov 4, 2022

jkoritzinsky Nov 4, 2022

trylek Nov 4, 2022

trylek Nov 4, 2022

davidwrighton Nov 4, 2022

davidwrighton Nov 4, 2022

trylek Nov 4, 2022 •

edited

Loading

davidwrighton Nov 4, 2022

trylek Nov 4, 2022

trylek left a comment

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

Improve coreclr test infra #77929

Improve coreclr test infra #77929

Conversation

davidwrighton commented Nov 4, 2022

ghost commented Nov 4, 2022

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

trylek Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trylek left a comment

Choose a reason for hiding this comment

trylek Nov 4, 2022 •

edited

Loading