Uh oh!
There was an error while loading. Please reload this page.
Convert the Remaining Legacy Tests to the Merged Wrapper Model - #98469
Conversation
doesn't build. It can't generate the runner file for some reason.
ghost
commented
Feb 14, 2024
Tagging subscribers to this area: @hoyosjs Issue DetailsThis PR intends to finish the efforts of the Test Consolidation Project. We are aiming to have our entire coreclr test system use the merged wrappers. The previous efforts are detailed in their respective PR's:
This is still a Work In Progress.
|
| { | ||
| Delegates.Run(); | ||
| Devirtualization.Run(); | ||
| // Generics.Run(); |
There was a problem hiding this comment.
Seems like a functional change.
There was a problem hiding this comment.
Oh if you mean having Generics.Run() commented out, that's just temporarily. I was running into some build issues but I wanted to test in CI for a more complete feedback. It will be restored before moving this PR from "Draft" to "Ready for Review".
ivdiazsa
commented
Feb 22, 2024
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
feasible to migrate them, at least for the time being.
ivdiazsa
commented
Mar 6, 2024
@jkoritzinsky Besides reenabling that |
ivdiazsa
commented
Mar 6, 2024
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ivdiazsa
commented
Mar 6, 2024
/azp run runtime-coreclr outerloop |
| <!-- Due to the nature of the MultiModule tests, it is unfeasible to convert | ||
| them to the Merged Wrapper system. | ||
| The main idea of these ones is that we pre-NativeAOT the whole framework, | ||
| then NativeAOT each assembly separately, and then link them all together | ||
| at the end using the native linker. | ||
| So, when we try using the Merged Wrapper system, nothing ends up producing | ||
| the code for xunit.assert, as its assembly is not in the actual framework. | ||
| --> | ||
| <MergedWrapperProjectReference Remove="SmokeTests/MultiModule/*.csproj" /> |
There was a problem hiding this comment.
If we mark the MultiModule tests as "RequiresProcessIsolation", then we shouldn't be getting warnings here. My guess is that I never hooked up the MSBuild logic for the "out of process" tests to exclude them as inputs to ILC. If you want to fix in this this PR, you can. Otherwise, can you file a follow-up issue?
There was a problem hiding this comment.
I can confirm that adding <RequiresProcessIsolation> removes the warnings from this test. So, just to confirm, the follow-up issue is to write the necessary logic to exclude out-of-process tests from being inputs to ILC. Is this accurate?
| <!-- Shouldn't need this: https://github.com/dotnet/linker/issues/2618 --> | ||
| <NoWarn>$(NoWarn);IL2050</NoWarn> | ||
| <RequiresProcessIsolation>true</RequiresProcessIsolation> |
There was a problem hiding this comment.
For tests that are legitimately marked as RequiresProcessIsolation due to something in the test, we should add comments explaining why so when we come around and clean up some of them in the future, we know which ones need to stay.
| <RequiresProcessIsolation>true</RequiresProcessIsolation> | |
| <!-- Registers a global ComWrappers instance for marshalling --> | |
| <RequiresProcessIsolation>true</RequiresProcessIsolation> |
| <AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
| <StartupHookSupport>true</StartupHookSupport> | ||
| <NoWarn>$(NoWarn);IL2026</NoWarn> | ||
| <RequiresProcessIsolation>true</RequiresProcessIsolation> |
There was a problem hiding this comment.
| <RequiresProcessIsolation>true</RequiresProcessIsolation> | |
| <!-- Uses a custom test environment variable --> | |
| <RequiresProcessIsolation>true</RequiresProcessIsolation> |
dotnet#98469 deleted this line. The test is generating half a dozen of these warnings, so put it back.
dotnet#98469 regressed a development scenario around passing command line arguments to this test. Command line argument can selectively choose one test to run. This is extremely useful when there’s test failures. The test does a ton of things and ability to zoom in is useful. Alternatively, we could put the test on the non-merged plan, same way we left nativeaot\SmokeTests\ControlFlowGuard on the non-merged plan (I assume for the same reason because it uses command line arguments?). I would prefer that solution but made a PR for the other approach because if I understand it correctly, we don't want to leave tests like that.
#98469 deleted this line. The test is generating half a dozen of these warnings, so put it back.
This PR intends to finish the efforts of the Test Consolidation Project. We are aiming to have our entire coreclr test system use the merged wrappers. The previous efforts are detailed in their respective PR's: