-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
Enable the fundamentals behind libraries tests compiled via crossgen2 #75230
Enable the fundamentals behind libraries tests compiled via crossgen2 #75230
Conversation
davidwrighton
commented
Sep 8, 2022
- Document how to use the special modes of libraries compilation
- Add the TestCrossgen2 flag to libraries testing
- Unlike TestNativeAot and TestSingleFile, use a self-contained strategy instead of actual single-file to improve debuggability of the produced output
- Tweak the SingleFileTestRunner.cs to avoid the fork bomb problem introduced by RemoteTestExecutor in a self-contained, but not singlefile scenario
- Add support for a set of flags to SingleFileTestRunner to respect the command line options I needed when debugging why the tests were not working.
- Document how to use the special modes of libraries compilation - Add the TestCrossgen2 flag to libraries testing - Unlike TestNativeAot and TestSingleFile, use a self-contained strategy instead of actual single-file to improve debuggability of the produced output - Tweak the SingleFileTestRunner.cs to avoid the fork bomb problem introduced by RemoteTestExecutor in a self-contained, but not singlefile scenario - Add support for a set of flags to SingleFileTestRunner to respect the command line options I needed when debugging why the tests were not working.
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries Issue Details
|
Each test project can potentially have multiple target frameworks. There are some tests that might be OS-specific, or might be testing an API that is available only on some target frameworks, so the `TargetFrameworks` property specifies the valid target frameworks. | ||
|
||
### Running tests in custom compilation modes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ViktorHofer I'm interested in any particular details I missed here. We really need some documentation on this sort of thing, as the behaviors of our tests are not particularly clear to folks who don't look into these systems often.
filters.ExcludedNamespaces.Add(args[i + 1].Trim()); | ||
i++; | ||
} | ||
if (args[i].Equals("-parallel", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for extending this! Especially the -parallel
option is something that I've been doing locally pretty much forever.
I would like us to get rid of this runner eventually (#70432). The xunit runner from Arcade used to support single file testing (including remote executor) but all of that was deleted when UWP support was deleted from Arcade. I'd like to restore it at some point.
I wonder though whether crossgen2 testing needs the single file runner - could it get by with the real runner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect it could use the normal file runner, but this looks like it will work, and I don't understand the libraries testing enough to understand how exactly to make this work with the normal runner without doing more research. With the additional arguments, I don't see a huge benefit to making this different from SingleFile or NativeAOT testing. As a side benefit, enabling R2R singlefile testing here becomes extremely easy should we want to do so.
eng/testing/tests.props
Outdated
@@ -8,6 +8,7 @@ | |||
<VSTestNoLogo>true</VSTestNoLogo> | |||
<ILLinkDescriptorsPath>$(MSBuildThisFileDirectory)ILLinkDescriptors\</ILLinkDescriptorsPath> | |||
<TestSingleFile Condition="'$(TestNativeAot)' == 'true'">true</TestSingleFile> | |||
<TestSingleFile Condition="'$(TestCrossgen2)' == 'true'">true</TestSingleFile> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to just name this TestReadyToRun?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. That's a better name
|
||
To run a test in specific mode, simply build the tests after building the prerequisite subsets, and specify the test mode on a command line such as: | ||
```cmd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Try to keep lists and code examples surrounded by blank lines, as per the recommended Markdown guidelines, so that we keep our md
files as clear of warnings as we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the NIT I pointed out in the Markdown doc file, but otherwise looks good to me! Thanks for enabling this!
…ct references as content
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
@@ -8,6 +8,7 @@ | |||
<VSTestNoLogo>true</VSTestNoLogo> | |||
<ILLinkDescriptorsPath>$(MSBuildThisFileDirectory)ILLinkDescriptors\</ILLinkDescriptorsPath> | |||
<TestSingleFile Condition="'$(TestNativeAot)' == 'true'">true</TestSingleFile> | |||
<TestSingleFile Condition="'$(TestReadyToRun)' == 'true'">true</TestSingleFile> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not blocking because I guess it works, but ReadyToRun is not single file even if you squint really hard and having to opt out of a bunch of things in tests.singlefile.targets is now making things quite confusing. We'll likely have to add more hacks around it in the coming months/years. It's not great that we have to set it.
To run a test in specific mode, simply build the tests after building the prerequisite subsets, and specify the test mode on a command line such as: | ||
```cmd | ||
dotnet build /t:Test /p:Configuration=Release /p:TestReadyToRun=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
dotnet build /t:Test /p:Configuration=Release /p:TestReadyToRun=true | |
dotnet build /t:Test -c Release /p:TestReadyToRun=true |
### Running tests in custom compilation modes | ||
|
||
There are several custom compilation modes for tests. These are enabled by setting a switch during the config. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you mean by "during the config"? I assume you are talking about the test invocation? Might make sense to clarify that part.
@@ -27,6 +27,9 @@ public static int Main(string[] args) | |||
var asm = typeof(SingleFileTestRunner).Assembly; | |||
Console.WriteLine("Running assembly:" + asm.FullName); | |||
|
|||
// The current RemoteExecutor implementation is not compatible with the SingleFileTestRunner | |||
Environment.SetEnvironmentVariable("DOTNET_REMOTEEXECUTOR_SUPPORTED", "0"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a tracking issue for that, to link to?
<!-- R2R testing does not tolerate the combination of a regular project reference and a content reference. --> | ||
<PublishReadyToRunExclude Include="LongPath.dll" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, why does R2R testing not allow a ProjectReference that contributes to the Content
item? Is there a tracking issue for that?
Closing and reopening to get CI to run again and check the failures. It had been some time and they had been deleted. |
@@ -50,5 +50,7 @@ | |||
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> | |||
</ProjectReference> | |||
<ProjectReference Include="..\System.Diagnostics.FileVersionInfo.TestAssembly\System.Diagnostics.FileVersionInfo.TestAssembly.csproj" Condition="'$(TargetOS)' == 'Browser'" /> | |||
<!-- R2R testing does not tolerate the combination of a regular project reference and a content reference. --> | |||
<PublishReadyToRunExclude Include="System.Diagnostics.FileVersionInfo.TestAssembly.dll" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a bug in the SDK which should be fixed there.
This small project is now taken over at PR #80946. |