-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Enable runtime async for all libraries test projects #123448
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
Conversation
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.
Pull request overview
This pull request enables the runtime async preview feature for all .NET 11+ library test projects, with appropriate exclusions for scenarios where it's not supported.
Changes:
- Add conditional PropertyGroup to enable runtime async feature for compatible test projects
- Remove trailing whitespace on line 92
Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com>
|
@jtschuster Thanks! Anything else? |
jtschuster
left a comment
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, but not sure if there are any other outerloop tests we should run before merging?
Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com>
|
/ba-g tests already ran |
| and '$(TestNativeAot)' != 'true' | ||
| and '$(TestReadyToRun)' != 'true' | ||
| and '$(UseNativeAOTRuntime)' != 'true' | ||
| and '$(TargetOS)' != 'browser' |
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 effectively makes all libraries test projects target platform specific. Libraries test projects are target platform neutral by default. When they are target platform specific, it needs to be explicitly declared in the project file.
For example, System.Net.WebSockets.Client.Tests is conditionally compiled based on TargetOS==browser https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.WebSockets.Client/tests/System.Net.WebSockets.Client.Tests.csproj#L91 and thus it requires browser-specific TFM https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.WebSockets.Client/tests/System.Net.WebSockets.Client.Tests.csproj#L7
This change is going to break inner loop experiences that depend on incremental builds at minimum.
cc @dotnet/area-infrastructure-libraries
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 you mean that if I built the System.Net.WebSockets.Client.Tests with /p:RuntimeFlavor=CoreCLR it would not run with Mono VM in the same artifacts folder ? For me that never worked because Mono and CoreCLR don't coexist well in artifacts already. I never explored the details of why. I have separate checkout directories (most of the time).
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 was thinking about switching between platforms, like CoreCLR on windows and CoreCLR on browser. Switching platforms like this used to work fine, with CoreCLR at least.
Mono and CoreCLR don't coexist well in artifacts already
Right, switching between Mono and CoreCLR does not work with incremental builds.
No description provided.