Make mono runtime tests stop using patching

[naricc]

Runtime tests on mono still rely on using the PatchCoreCLRCoreRoot target, for architectures other than Android and Wasm:

runtime/src/mono/mono.proj

Line 105 in ec26ee5

<Target Name="PatchCoreClrCoreRoot">

This step is confusing when running the tests locally, time consuming, and brittle. It was necessary when the tests were first extended to work on mono, but work since should make it possible to use use a corerun build with mono.

Before actually removing the target we also need to make sure that the automated performance runs do not use it. @DrewScoggins

[ghost]

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

[naricc]

Also do we need this dependency?

- job: mono_common_test_build_p0_AnyOS_AnyCPU_release
   condition: >-
   and(succeeded(), and(succeeded(), or(
   eq(dependencies.checkout.outputs['SetPathVars_mono.containsChange'], true),
  eq(dependencies.checkout.outputs['SetPathVars_runtimetests.containsChange'], true),
  eq(variables['isFullMatrix'], true))))
 container:
 alias: mcr.microsoft.com/dotnet-buildtools/prereqs:centos-7-359e48e-20200313130914
 continueOnError: false
 dependsOn:
  - checkout
  - coreclr__product_build_Linux_x64_release
  - libraries_build_Linux_x64_Release

[naricc]

I also checked with @DrewScoggins; performance runs are not using that target.

[safern]

@naricc I added blocking-clean-ci as this manifests as:

Artifact CoreCLRProduct__Linux_arm64_release not found for build

Whenever the Mono runtime tests start running before CoreCLR build is done.

[naricc]

@akoeplinger @steveisok I think the easiest way to do this is to use the test host the libraries tests are currently using, instead of corerun (which currently requires building coreclr). Is there any issue if the runtime tests depend on that test host?

[steveisok]

Not that I know of.

[marek-safar]

Is there any issue if the runtime tests depend on that test host?

This sounds to me like a painful path for runtime developers. I think we want to keep runtime/VM tests runner simple to make tracking any problems less painful. What is blocking us from moving corerun to shared location?

/cc @vargaz @lambdageek

[naricc]

@jkoritzinsky Is there anything blocking us from moving corerun to a shared location and building it independently of coreclr, as per @marek-safar's comment?

[safern]

Didn't runtime tests depend on libraries anyways? At least on CI they download libraries assets. cc: @trylek

[trylek]

Runtime tests do depend on libraries, these are used to populate the CORE_ROOT folder which is subsequently used as a "test-specific framework drop". That by itself shouldn't prevent moving corerun to a location shared between CoreCLR and Mono as the tool is usable for both. For the test host, I was under the impression that it by itself was also using the libraries build (to populate its version of the "test-specific framework drop") so I guess all the paths lead in a similar direction.

[marek-safar]

Runtime tests do depend on libraries

Which libraries are needed? I thought System.Private.Corelib should be enough for runtime tests and the remaining tests are in libraries tests so can take any dependency.

[trylek]

I don't know how to produce an exhaustive list but it's definitely more than System.Private.CoreLib, I'm not aware of any hard definition of a "set of assemblies usable in runtime tests". A random text search for "using" statements under src\tests shows e.g. System.IO, System.Linq, System.ComponentModel, System.Numerics, System.Reflection.Emit, System.Security.Cryptography, System.Text.RegularExpressions, System.Net.Http or System.Globalization. Perhaps there are actually not that many but we'd have to find out; in general, I think the rule of thumb for test placement in the runtime repo has been "focusing on exercising / stressing some runtime feature", not necessarily needing just System.Private.CoreLib. We may be able to clean this up or at least put some order in the dependencies but for the 10K tests it's probably going to take some time.

[safern]

You need a shared framework to run the tests as they run as XUnit .NET tests and I believe they use the live built shared framework, right? Or do we just use the live libraries to build the tests?

[jkoritzinsky]

The XUnit harness doesn’t use the live built runtime any more. We moved to pulling down a copy of the runtime bundled with the SDK we build with for running xunit a while ago.

[trylek]

I think it's actually even more convoluted - the XUnit wrappers and XUnit console use the bundled framework as Jeremy says, however the tests themselves (running out-of-proc) use the framework in CORE_ROOT. This is one of the reasons why a couple of weeks back @nattress had to move xunit out of the CORE_ROOT folder exactly due to incompability between the two SDK versions.

[jkoritzinsky]

Yep. The harness uses the downloaded framework, but the tests themselves use CORE_ROOT.

Additionally, for some of the tests, (at least for CoreCLR runs) we use the additional tools available in CORE_ROOT that we don't ship in the shared framework. I think having two different ways to form CORE_ROOT is reasonable, but I don't think sharing a test host layout with libraries would work well.

[naricc]

Ok, so my conclusion from this is that I should not use the libraries test host, but instead should make core_run build independently of coreclr, move the source for it to a new location, and then use corerun to as normal for the runtime tests. I will proceed with that approach.

[naricc]

Using this issue to continue to document my findings, it seems like there are actually two coreruns, one in coreclr/src/hosts/corerun and another in coreclr/src/hosts/unixcorerun. One unixcorerun is used on unix paltforms, and the other is used one Windows.

It seems like two seperate tasks to move them, so I will do the unixcorerun first, since it seems a little simpler, and it is the one we need to remove the patch step.

Also, does any one have any preference on the location I move it to? I was thinking /src/hosts.

[trylek]

I would initially suggest the already existing src/native - I actually have a vague recollection @jkotas recently suggested that as a location we could move the various hosts to as well.

[jkoritzinsky]

I think it should go in src/tests since it’s a test only host. Or into the new shared folder that the shared event pipe source is going into.

[SamMonoRT]

Moving the test enhancement issues to 7.0.0

[lambdageek]

I started a checklist #58266 for some cleanup work to make working with runtime tests easier for folks primarily building Mono - ideally only building corerun and not the rest of coreclr and its CoreLib, etc. Removing this "patching" behavior will be one of the steps along the way.

[krwq]

Is this a dup of #51584 ? Per @safern's comment the message on both issues seems the same

[naricc]

@krwq This is a necessary thing to do for #51584. I am not sure if it is the whole thing.