Merge CoreMangLib, Exceptions and Interop folders #91901
Conversation
|
Tagging subscribers to this area: @hoyosjs Issue DetailsThis change converts tests in the CoreMangLib, Exceptions and Interop to the merged model. As next step I plan to work on review feedback to my previous PR merging baseservices and on merging the remaining tests. Thanks Tomas
|
|
/cc @ivsiazsa @dotnet/runtime-infrastructure |
| @@ -1,5 +1,7 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
| <PropertyGroup> | |||
| <!-- Needed for mechanical merging of all remaining tests, this particular project may not actually need process isolation --> | |||
| <RequiresProcessIsolation>true</RequiresProcessIsolation> | |||
| <OutputType>exe</OutputType> | |||
There was a problem hiding this comment.
It looks like the lowercase 'exe' slipped through the scan.
There was a problem hiding this comment.
(though, now that I think about it, I guess that's where ReqProcIso ends up anyway, though I think we automatically set it (kind of hacky in Dir.B.targets since it is after the SDK has had a look at it))
There was a problem hiding this comment.
I think you're right that we shouldn't keep obsolete OutputType=exe properties on most tests except the tiny subset opting out of the merged wrappers for special reasons like the half a dozen tests under baseservices that do weird things. I have experimentally removed all these annotations from the interop tests and I'm retesting the change locally, I'll be happy to update the PR by removing those if it turns out to be working.
| MonoAPI.Tests.MonoAPISupport.Setup(); | ||
| int result; | ||
| result = test_0_setjmp_exn_handler (); | ||
| Assert.Equal(0, result); |
There was a problem hiding this comment.
I suspect that this (and the others) end up being fine (mono test, int equality), but I don't know for sure that these don't end up being run in some configuration that breaks. But hopefully outerloop (and extra platforms?) is good enough to check that?
There was a problem hiding this comment.
All I can say to this now is that in general Mono tests are supported by the merged test infrastructure, @jkoritzinsky went to great lengths to make that fully functional and IIRC Mono people were happy about his implementation in terms of efficiency and cleanness.
There was a problem hiding this comment.
Sorry, I wasn't clear. I just don't know if this test will end up running in some other configuration (from a quick look, I don't see it being excluded from other runtimes). However, I meant it more as a "heads up" in case something breaks, not an objection to the PR.
There was a problem hiding this comment.
If we have concerns, we can use the SkipOnCoreClrAttribute on the test and it will automatically be skipped on non-Mono platforms. Tbh I think that's a better solution than suppressing the test in issues.targets.
src/tests/Interop/PInvoke/Array/MarshalArrayAsParam/AsDefault/AsDefaultTest.cs
Show resolved
Hide resolved
| return 100; | ||
| } catch (Exception e){ | ||
| Console.WriteLine($"Test Failure: {e}"); | ||
| return 101; | ||
| } |
There was a problem hiding this comment.
I like this cleanup that we get!
trylek
force-pushed
the
MergeCoreMangLibExceptionsInterop
branch
from
51753b7
to
ef78e29
Compare
trylek
force-pushed
the
MergeCoreMangLibExceptionsInterop
branch
from
ef78e29
to
c0620fa
Compare
trylek
force-pushed
the
MergeCoreMangLibExceptionsInterop
branch
from
3102df5
to
7e50880
Compare
This is needed so that they don't copy their outputs to the merged wrapper folder, triggering errors in Mono builds because some output file names overlap. Thanks Tomas
trylek
force-pushed
the
MergeCoreMangLibExceptionsInterop
branch
from
011550e
to
f930ffd
Compare
|
/azp run runtime-coreclr outerloop |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
ghost
locked as resolved and limited conversation to collaborators
This change converts tests in the CoreMangLib, Exceptions and Interop to the merged model. As next step I plan to work on review feedback to my previous PR merging baseservices and on merging the remaining tests.
Thanks
Tomas