Uh oh!
There was an error while loading. Please reload this page.
Improve coreclr test infra - #77929
Conversation
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
commented
Nov 4, 2022
Tagging subscribers to this area: @hoyosjs Issue DetailsThese 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
|
| 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)); |
| for (int i = 0; i < filterArgs.Length; i++) | ||
| { | ||
| if (filterArgs[i].StartsWith("-stripe")) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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)); |
There was a problem hiding this comment.
| thrownewArgumentException("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)); | |
| thrownewArgumentException("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)); |
| { | ||
| return true; | ||
| // Test stripe, if true, then report success | ||
| return ((System.Threading.Interlocked.Increment(ref _shouldRunQuery)) % _stripeCount) == _stripe; |
There was a problem hiding this comment.
This is a very clever and cheap way to handle striping. I like it!
| // 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| </PropertyGroup> | ||
| <!-- Generate a set of stress marker files to be used in stress scenarios in CI runs --> | ||
| <ItemGroup Condition="'$(NumberOfStripesToUseInStress)' != '' and '$(IsMergedTestRunnerAssembly)' == 'true'"> |
There was a problem hiding this comment.
We should add a backlog item to introduce an MSBuild task for this instead of hard-coding a list of 0-99.
| public bool ShouldRunTest(string fullyQualifiedName, string displayName, string[]? traits = null) | ||
| { | ||
| bool shouldRun = false; |
There was a problem hiding this comment.
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 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)', '
')" 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)', '
')" Condition="'@(_ProjectReferencesRemovedDueToConflictResolution)' != ''" /> |
There was a problem hiding this comment.
Nit - I'm not sure what to make of the wording 'if this CoreCLRTestLibrary', is the intention "if it's CoreCLRTestLibrary" or something else?
There was a problem hiding this comment.
I should have written "if this is CoreCLRTestLibrary"
There was a problem hiding this comment.
The errors that happen if you do this wrong, are entirely inscrutable, and extremely difficult to debug.
| <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 --> |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Looks great to me, thanks David!
… as some documentation on the command line parameters for merged test runner assemblies
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