Switch over Loader/classloader/TypeGeneratorTests to use merged wrappers

[trylek]

This is an initial attempt to introduce a group project. As of the initial commit it doesn't yet have any Helix identity of its own - in grouped (non-standalone) mode, the group project replaces its initial component projects and ends up with a single Helix xUnit result (as it still gets run through the xUnit wrapper logic).

We need to decide whether this is acceptable or whether we need to retain the original behavior of each test case reporting its results individually in a way that can be parsed by our scripts. This is going to become even more important once we start pushing towards converting all tests to the new style. Assuming we want to keep the individual test cases visible in AzDO, this change will require an additional delta to implement the relevant logic.

The change switches over the first 100 type generator tests (numbers 0-99); to facilitate lab coverage, it also sets BuildAsStandalone to false by default in contrast to Jeremy's original implementation. I have verified locally that by setting BuildAsStandalone to true I get the original 1501 tests with individual results. By not setting BuildAsStandalone or by setting it to false, I'm receiving the correct number of 1402 tests (1401 original non-merged tests from 100 onward and the one extra grouped test).

Thanks

Tomas

[ghost]

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

Issue Details

---

This is an initial attempt to introduce a group project. As of the initial commit it doesn't yet have any Helix identity of its own - in grouped (non-standalone) mode, the group project replaces its initial component projects and ends up with a single Helix xUnit result (as it still gets run through the xUnit wrapper logic).

We need to decide whether this is acceptable or whether we need to retain the original behavior of each test case reporting its results individually in a way that can be parsed by our scripts. This is going to become even more important once we start pushing towards converting all tests to the new style. Assuming we want to keep the individual test cases visible in AzDO, this change will require an additional delta to implement the relevant logic.

The change switches over the first 100 type generator tests (numbers 0-99); to facilitate lab coverage, it also sets BuildAsStandalone to false by default in contrast to Jeremy's original implementation. I have verified locally that by setting BuildAsStandalone to true I get the original 1501 tests with individual results. By not setting BuildAsStandalone or by setting it to false, I'm receiving the correct number of 1402 tests (1401 original non-merged tests from 100 onward and the one extra grouped test).

Thanks

Tomas

Author:	trylek
Assignees:	-
Labels:	area-Infrastructure-coreclr
Milestone:	-

[trylek]

As a side note, the initial local testing confirmed the previously heralded perf wins - the BuildAsStandalone mode of execution took 50 and 48 seconds on my laptop in two successive runs whereas the non-standalone run (with the first 100 tests grouped) took 45 and 44 seconds.

[BruceForstall]

ends up with a single Helix xUnit result

Don't we want (need?) each test to get its own result? This is true for the libraries tests today, isn't it? Each test result needs to get uploaded to Kusto to be available for query over the results, as well as to be individually visible in AzDO.

[trylek]

@BruceForstall - agreed, I've also come to the conclusion it's not acceptable even temporarily to change the lab behavior in this manner. Jeremy has already introduced logic for emitting the xUnit summaries, I'm now working on piping them through the Helix infrastructure next to the legacy xUnit console results.

[BruceForstall]

fwiw, this change LGTM, but @jkoritzinsky should review it.

[trylek]

@BruceForstall - sure, this change is definitely not ready to be checked in tomorrow. I'm still hitting a couple of test build failures, I need to add the support for processing the new xUnit summaries and I'll definitely wait for Jeremy to review it. At this point I'm publishing it mostly as a heads-up regarding what the future merged projects are going to look like.

[naricc]

@fanyang-mono and I are both takign some time off before the Thanksgiving holiday. I will give this a look over when I get back.

[trylek]

Thanks @naricc for the heads-up, that's just fine of course; as you can see, the change is not even passing yet; furthermore Jeremy is also OOF for Thanksgiving and I definitely need his review too.

[trylek]

@jkoritzinsky - I have improved local reporting in run.py to look somewhat like the pre-existing xunit console reporting. As you can see, the output is not yet ideal. I'd love to collect broad feedback regarding showstopper and nice-to-have improvements.

Parsing test results from (D:\git\runtime4\artifacts\log\TestRunResults_windows_x64_Release)
Analyzing D:\git\runtime4\artifacts\log\TestRun.xml
Analyzing D:\git\runtime4\artifacts\log\TypeGeneratorTests0-99.testRun.xml
Failed test: Generated42Main
# Test output recorded in log file.

Time [secs] | Total | Passed | Failed | Skipped | Assembly Execution Summary
============================================================================
     46.819 |  1401 |   1401 |      0 |       0 | Loader.classloader.XUnitWrapper.dll
      0.417 |   100 |     99 |      1 |       0 | TypeGeneratorTests0-99
----------------------------------------------------------------------------
     47.236 |  1501 |   1500 |      1 |       0 | (total)

Most notably, neither the wrapper name nor the failed test case give any indication of their location in the test tree making developer orientation quite difficult.

Thanks

Tomas

[jkoritzinsky]

I've pushed up a change that should update the display name of a test to give the path to the assembly if it is the only test in the assembly (which is the initial simple migration path for tests and the likely forever story for IL tests)

[trylek]

@jkoritzinsky - I have experimentally added one more wrapper project for the JIT\Methodical\Arrays\range tests to this PR to let me exercise the new functionality in faster Pri0 runs, however the wrapper doesn't seem to be picking up any of the [Fact] methods in the referenced ilproj projects. Can you please advise me how to debug the source generator logic so that I can investigate what's wrong?

[trylek]

OK, so here is my TODO list to things to validate before merging this in:

1. Validate that the merged item runs in Helix on both Windows and Unix.

2. Validate that Helix consumes the merged item xUnit summaries.

3. Validate that a crash in the merged test is properly detected.

4. Validate that a timeout in the merged test is propertly detected.

5. Fix creation of repro scripts for merged tests in local execution. Right now it doesn't work because it creates a script calling the test execution script which doesn't exist for components of merged tests. We should be calling into the merged test execution script and probably use a command-line parameter or environment variable to make the merged test execute just this one test case.

6. This can be put off for a bit but we need some form of validation that inconsistent definition of the test wrappers doesn't end up silently skipping tests. In other words, all tests that are getting built in BuildAsStandalone mode must be either also building in non-standalone mode or they must be referenced from one of the merged test wrappers. Initially I'm thinking about some marker files the merged tests would put in the output directories corresponding to their project references but I'm not yet completely clear on the actual implementation.

7. For converting additional batches of tests we'll need to consult the issues.targets file and inject the ActiveIssue annotations as appropriate. It would be very unfortunate to have to keep dual test exclusion system for a prolonged period of time, I'm thinking about bulk injecting the annotations using my rewriter as part of marking the tests with [Fact] attributes.

8. One thing that complicates this is that today I think there's a bit of deficiency - once we switch a csproj test over to use [Fact] annotations and remove the Main method, we need to remove the Exe OutputType, otherwise the build complains about a missing Main. This however means that in non-standalone (merged) build mode the test expects to be a part of some wrapper; if it's not, it gets silently ignored because the build system sees and builds it as a DLL it expects to be called from the wrapper. I'm not sure whether we must fix this but doing so would decouple conversion of csproj tests to the [Fact] model from the actual creation of wrapper projects. Or should we perhaps keep the Main methods in the converted C# tests akin to what we do for ilproj tests? (clarified by Jeremy's feedback)

[trylek]

@jkoritzinsky - big sigh of relief, the latest outerloop job finally does the right thing - it fails the one instrumented test and properly reports it. As you can easily see here,

https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-61942-head-c24817b9e7d94d119e/Loader-classloader-TypeGeneratorTests-TypeGeneratorTests0-99/1/console.e677d2b5.log?sv=2019-07-07&se=2021-12-26T02%3A19%3A46Z&sr=c&sp=rl&sig=rDpGpO1RdqSkouIwTvaL7odMadAoRFTlk9TnFKp0L9Y%3D

the lengthy log spanning the multiple tests is quite hard for orientation. I'm wondering whether we might be able to mitigate that by 2-3 small improvements to the source generator:

1. Log the name of each test case prior to its execution.
2. Writing out the number of passed / failed / skipped tests at the end of execution.
3. Dumping the list of names of failed tests at the end of execution.

Thanks

Tomas

[jkoritzinsky]

For item 8 in your list, that's the expected behavior. That's why the default for BuildAsStandalone is true until we make all of the tests covered by the merged wrappers. Then after we switch the default, users will need to manually build individual tests with /p:BuildAsStandalone=true if they want to build a specific test suite as a standalone runnable test.

[trylek]

That's why the default for BuildAsStandalone is true until we make all of the tests covered by the merged wrappers

@jkoritzinsky - I'm not sure if I'm getting this, how can we then gradually transform the source tree to the new model? Your response seems to imply that we can only merge this PR in once all the tests have been transformed to merged wrappers while I believe the entire purpose of our current work so far is to allow for an intermediate heterogeneous phase where some tests have already been converted whereas others have not; I don't think it's realistic to fully convert the entire source tree in a single PR.

[jkoritzinsky]

@trylek The idea is to have everyone migrate their tests to use [Fact] attributes first. Then they can use the old xunit wrappers with the "standalone" runner model in each assembly.

Then, we'll go through and add the merged test runners until we've covered the whole test tree.

Once we've covered the whole test tree, then we will switch the default of BuildAsStandalone to false.

[jkoritzinsky]

Basically, as long as the transition to using the Xunit attributes is done within a subtree before we create the merged runners, then we can do this piecemeal through the whole test tree.

[trylek]

Basically, as long as the transition to using the Xunit attributes is done within a subtree before we create the merged runners, then we can do this piecemeal through the whole test tree.

OK, so for the TypeGeneratorTests the "transition to using the Xunit attributes" has already been carried out. So what else needs to be done beyond the gist of this PR to actually switch them over to the new model where by default they'll use the merged wrapper unless BuildAsStandalone=true is specified? The conversion certainly merits some amount of bake time in the lab before we push the equivalent change to large swathes of additional tests. Alternatively, should we modify the outerloop runs as part of this PR to selectively switch BuildAsStandalone to false so that we have at least some pipeline exercising the merged tests on a regular basis?

[jkoritzinsky]

OK, so for the TypeGeneratorTests the "transition to using the Xunit attributes" has already been carried out. So what else needs to be done beyond the gist of this PR to actually switch them over to the new model where by default they'll use the merged wrapper unless BuildAsStandalone=true is specified?

When you updated the IL tests to have the [Fact] attribute, you already did the first step. This PR also does the second step of adding a merged runner assembly. These steps don't need to be done at the same time. I believe in this PR you already added logic to detect when some tests are run with the merged runner and exclude them from the old system.

The conversion certainly merits some amount of bake time in the lab before we push the equivalent change to large swathes of additional tests. Alternatively, should we modify the outerloop runs as part of this PR to selectively switch BuildAsStandalone to false so that we have at least some pipeline exercising the merged tests on a regular basis?

I think the fact that this PR has some tests already converted (and IIRC you tentatively converted some C# tests as well), so the BuildAsStandalone=false case should be tested once this PR or an equivalent is merged.

[trylek]

This PR does not implement support for running the new-style merged wrappers on mobile platforms. I'm working on that support in a fork of this branch.

@jkoritzinsky - is that blocking for this particular change that only converts Pri1 tests that are not exercised in Mono testing at all? I certainly agree that we need to fix that soon but I'm wondering whether for the sake of expediency we might be able to parallelize the efforts - i.o.w. if we agree that this particular change doesn't hurt Mono testing in any fundamental manner, merging it in would substantially simplify both your device support changes and my continued work on debugging the switch-over of additional tests.

[jkoritzinsky]

I don't think that's a blocker for this PR as it only switches over Pri1 tests that Mono doesn't run. Just wanted to make sure that the deficiency was known before the Mono folks went looking for it.

[jkoritzinsky]

The PR failures are known ongoing issues and aren't related to this work. I think this all looks great!

[jkoritzinsky]

Let's get this in!

[trylek]

Thanks Jeremy for your supportive review. I'm merging this in based on our shared understanding that this particular change cannot regress Mono testing as it only involves Pri1 tests that aren't currently exercised in Mono testing. I'll make sure to address any feedback Mono developers might have in a follow-up change.