Convert all tests under baseservices to the merged test infrastructure

[trylek]

This change converts all remaining tests under baseservices to use the merged test model.
Apart from a few tiny tweaks I found a somewhat funny case in the test

baseservices/RuntimeConfiguration/TestConfig.csproj

This test had two problematic characteristics w.r.t. test merging - it was launching itself as
a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally
didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"
app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were
actually superfluous because the test uses two additional attributes (ConfigProperty and
EnvVar) to mark the "interesting" methods.

Thanks

Tomas

/cc @dotnet/runtime-infrastructure

[ghost]

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This change converts all remaining tests under baseservices to use the merged test model.
Apart from a few tiny tweaks I found a somewhat funny case in the test

baseservices/RuntimeConfiguration/TestConfig.csproj

This test had two problematic characteristics w.r.t. test merging - it was launching itself as
a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally
didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"
app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were
actually superfluous because the test uses two additional attributes (ConfigProperty and
EnvVar) to mark the "interesting" methods.

Thanks

Tomas

/cc @dotnet/runtime-infrastructure

Author:	trylek
Assignees:	trylek
Labels:	area-System.Reflection.Metadata
Milestone:	-

[markples]

How have you checked whether all tests have been preserved across the conversion? It seems like the changes to how the internal test harness work could impact the number of reported tests and make this difficult. (If so, do you have recommendations on how to review this with that in mind?)

[markples]

For the tests that define a Main, I believe that BuildAsStandalone will be broken since it will try to generate an entry point. I think setting ReferenceXUnitWrapperGenerator to false will fix this.

[trylek]

Well, in the local testing I have so far just monitored the number of tests; in fact even that has changed slightly as I split one or two tests to multiple [Fact]s (I tried to clearly separate those individual changes from the bulk conversions as individual commits on the PR). I'm still working on fixing the BasicTestWithMcj test, once I'm done with that, I think I'll compare the output xml files, I think the last time I wrote a one-off managed app for the purpose, I need to take a look if I still have it around but it's about half an hour's work to write.

[trylek]

Thanks for the suggestion regarding ReferenceXUnitWrapperGenerator, that should make it nicely visible which tests are problematic for some future cleanup wave.

[ghost]

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This change converts all remaining tests under baseservices to use the merged test model.
Apart from a few tiny tweaks I found a somewhat funny case in the test

baseservices/RuntimeConfiguration/TestConfig.csproj

This test had two problematic characteristics w.r.t. test merging - it was launching itself as
a child process and it (ab)used the [Fact] attributes for "its own mini-harness" that naturally
didn't go well with the merged test infra. I have refactored the test to use a separate "Tester"
app (akin to similar cases like ParallelCrash) and I removed the [Fact] attributes as they were
actually superfluous because the test uses two additional attributes (ConfigProperty and
EnvVar) to mark the "interesting" methods.

Thanks

Tomas

/cc @dotnet/runtime-infrastructure

Author:	trylek
Assignees:	trylek
Labels:	area-System.Reflection.Metadata, area-Infrastructure-coreclr
Milestone:	-

[trylek]

@markples - I have stood up the xml differ and I have managed to convince myself that the results are fine:

Left-only tests (5 total):
--------------------------
baseservices\exceptions\exceptioninterop\ExceptionInterop_ro\ExceptionInterop_ro
baseservices\exceptions\exceptioninterop\ExceptionInterop\ExceptionInterop
baseservices\exceptions\simple\ParallelCrashMainThread\ParallelCrashMainThread
baseservices\exceptions\simple\ParallelCrashWorkerThreads\ParallelCrashWorkerThreads
baseservices\TieredCompilation\BasicTestWithMcj\RunBasicTestWithMcj

Right-only tests (24 total):
----------------------------
_ExceptionInterop_ro::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop_ro::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ExceptionInterop::ExceptionInterop.ThrowManagedExceptionThroughNativeAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrame()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFilter()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionAndCatchInFrameWithFinally()
_ExceptionInterop::ExceptionInterop.ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThread()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashMainThreadAndWorkerThreads()
_ParallelCrashTester::ParallelCrashTester.ParallelCrashWorkerThreads()
baseservices\exceptions\stackoverflow\stackoverflow\stackoverflow
baseservices\exceptions\stackoverflow\stackoverflow3\stackoverflow3
baseservices\exceptions\StackTracePreserve\StackTracePreserveTests\StackTracePreserveTests
baseservices\exceptions\unhandled\unhandledTester\unhandledTester
baseservices\finalization\CriticalFinalizer\CriticalFinalizer
baseservices\ilasm_ildasm\regression\vswhidbey305155\305155\305155
baseservices\mono\runningmono\runningmono
baseservices\multidimmarray\enum\enum
baseservices\RuntimeConfiguration\TestConfigTester\TestConfigTester
baseservices\varargs\varargsupport_r\varargsupport_r
baseservices\varargs\varargsupport\varargsupport

There are several interesting observations based on this diff.

• We have an ugly discontinuity regarding test naming based on whether a given test project contains one or multiple [Fact] clauses - when there are multiple Fact clauses, we drop the test path information and just name the test based on the method. I think we should probably concatenate these two or concatenate the test dll / script path with the method name or something, the completely different representation of tests with multiple test cases is confusing and makes it harder for developers to correlate their test failures to the source / binary code.

• We have a subtle difference with regard to reporting tests blocked in issues.targets. The legacy infrastructure filtered them out at the msbuild level so that they never made it into the XUnit wrappers and simply aren't reported in any manner in the results. With the merged infrastructure we filter them out in a different manner and in the report we end up reporting them as "skipped". I think that's actually more appropriate, the only downside is that today there's no indication in the xml results file that the test in question was skipped due to an issues.targets clause; I have filed CoreCLR test infra: improve "Skip" messages for tests blocked in issues.targets #91562 to fix that. In the above list, that pertains to the "Right-only tests" 305155, runningmono, enum, varargsupport_r and varargsupport.

• For stackoverflow, stackoverflow3 or unhandled, I believe these need fixing in the merged wrapper generator. These projects are marked as BuildOnly and shouldn't be actually run, the xml result file pretends it has run the corresponding cmd / sh script but that is nonsense, there's no cmd / sh script getting generated for these. I'll follow up with Jeremy on fixing this, I'm not sure to what extent it's blocking for this particular PR. (In practice the projects actually getting run are stackoverflowtester and unhandledTester introduced as part of this PR.)

Thanks

Tomas

[trylek]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[markples]

Thanks Tomas!

• re: discontinuity in naming: I believe that we ended up in this position because we had a legacy naming system (path/project) which couldn't differentiate between multiple tests in a project, and the "new" way is closer to (or the same as?) standard xunit naming. The discontinuity was kept to minimize the number of tests that were renamed as part of adding merged test groups. However, with the eventual hope of merging the builds too, perhaps moving more towards xunit-style (always use it) would have less eventual churn that going back to incorporating paths? It is important for now that the assembly name is there (though it appears to be?) because we don't dedup class/test names.
• agreed on your analysis and new work item
• I believe that these are being caused by the RequiresProcessIsolation attribute, for which the processing doesn't check for BuildOnly, etc. Is this needed in the final iteration? If not, then instead of excluding BuildOnly, perhaps Dir.build.targets could consider this an error.
  • It seems odd that the wrapper is reporting it as run when it doesn't exist. It looks like it is this code, which appears to be necessary but could be a point where tests are discarded incorrectly.

[trylek]

/azp run runtime-coreclr outerloop

[trylek]

/azp run runtime-extra-platforms

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[trylek]

@markples - I have rebased this change against the latest main and I believe I have addressed all your pending PR feedback, can you please take another look when you have a chance?

Thanks Tomas

[trylek]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).