-
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
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
95934d2
Enable runtime async for all libraries test projects
agocke 5853c89
Also enable preview features
agocke 12ba43d
Exclude NativeAOT and Mono
agocke c05bfd2
Also skip browser
agocke ca9c6a6
Update eng/testing/tests.targets
agocke e11d2dd
Update eng/testing/tests.targets
agocke File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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
Uh oh!
There was an error while loading. Please reload this page.
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.Testswith/p:RuntimeFlavor=CoreCLRit would not run with Mono VM in the same artifacts folder ? For me that never worked because Mono and CoreCLR don't coexist well inartifactsalready. 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.
Right, switching between Mono and CoreCLR does not work with incremental builds.
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.
Sorry, I'm still not getting it -- what's the scenario where this could be broken? You compile the libraries tests for windows, then move it over to Linux and run the same test assembly there?
Uh oh!
There was an error while loading. Please reload this page.
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.
For example, you compile a library test project (that does not have browser TFM) for Windows, and then you compile the same project for browser in the same enlistment. The re-compilation for browser will be skipped assuming the incremental build works as expected.
You can see the problem by looking at the binary paths under artifacts. For example, if you compile
src\libraries\Microsoft.Extensions.DependencyInjection\tests\DI.Tests, the test binary path is going to beartifacts\bin\Microsoft.Extensions.DependencyInjection.Tests\Debug\net11.0\Microsoft.Extensions.DependencyInjection.Tests.dll. The platform is not mentioned in the path.Uh oh!
There was an error while loading. Please reload this page.
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.
Maybe that
'$(TargetOS)' != 'browser'condition doesn't need to be there because'$(RuntimeFlavor)' != 'Mono'is enough ? Because the coreCLR interpreter (browser) can do asyncv2, right ?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.
Ok, I think I understand now. Basically, because test assemblies are libraries, they do not get RID-targeting by default, meaning that all RIDs basically collapse together into a single path and MSBuild does not re-evaluate.
I'm not sure I have a great way to fix this immediately. The obvious answer, that I'm working on right now, is moving the tests to xunit3 and making them Exe projects instead of libraries. This would implicitly give everything RID targeting and should solve the problem.
Unfortunately I've also hit some test failures after moving to xunit3 that I'm still debugging with Brad, so the PR isn't ready yet.
I will see if I can come up with a cheap intermediate fix, like touching a file with the name of the TFM.
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 expect that we will have the same problem with enabling runtime async for shipping libraries. You won't get away with this shortcut there. It may be better to spend time on solving that (and then retrofit the solutions to tests if it is still relevant).