Improve and enable writing test wrappers to disk

[markples]

CodeBuilder utility (mainly handles indentation) was lifted from the PROSE codebase. I omitted the last two revisions to it as they opened a few API questions for me and I didn't need the functionality anyway.

Then use it to format the generated code in XUnitWrapperGenerator. Diffs are probably best viewed with whitespace ignored. Writing simple text (Append/AppendLine) is the same as StringBuilder. Indentation is specified (usually with a using that cleans up) and then added automatically.

Interesting tidbits:

• Factored AppendAliasMap from the 3 identical uses
• The loop structure for breaking the tests into groups of 50 doesn't allow using. I did the push/pop of indentation manually rather than trying to restructure it.

Add a new MergedTestRunner.targets for relevant logic. Enable EmitCompilerGeneratedFiles in it.

Questions:

• Should we leave EmitCompilerGeneratedFiles off and instead recommend turning it on at the point when someone hits problems and needs to see it? [agreement to automatically turn it on for merged groups]
• I didn't convert GenerateXHarnessTestRunner because I haven't seen that file while working on merged test groups. Should I? [Edit: I'll need to convert it or add some CodeBuilder->string transitions to fix things] [converted]
• Is there something like CodeBuilder already in the repo? It wouldn't be very hard to switch to something else similar and avoid duplication.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[markples]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[markples]

@trylek @jkoritzinsky Could one of you please take a look at this? This is part of the usability work for merged test groups - making the generated code easier to look at if necessary (wish I'd done this earlier!) Thanks!

[trylek]

I think it's reasonable to emit compiler-generated files for the merged wrappers (as opposed to the simple wrappers where I'd be worried about tons of files being generated potentially degrading Roslyn perf) but I think it's somewhat ugly to need to explicitly put the property on each merged wrapper; could we instead tweak the scripts so that EmitCompilerGeneratedFiles defaults to true when IsMergedTestRunnerAssembly is set to true?

[markples]

Thanks for the review and feedback!

You're definitely correct about changing each merged wrapper being ugly. In fact, I think there might be a few other things that we might factor out of those project files, but I will save those for later since they're unrelated to this. I always feel like I'm going to change Directory.Build.* incorrectly (partly, should .props set a default and allow each project to override it or should .targets set a default if unspecified - in this case it seems like it needs to be .targets since it depends on the other property).

I updated XHarness to the new style (after looking at the code, it is better to keep them consistent anyway). Just fyi - I haven't tested either of these other than what I'm going to automatically get now from the CI.

[trylek]

Overall looks great to me, thanks Mark! That indeed makes both the emitter and the emitted code much easier to reason about. I would recommend waiting for Jeremy's response before merging in to make sure I haven't overlooked some aspect he's more familiar with. For the XHarness runner, AFAIK that is only used for Mono testing, I guess that's why you didn't hit that when working on this change.

[markples]

@trylek @jkoritzinsky Thank you both. I've run into a (simple?) problem with not setting EmitCompilerGeneratedFiles in each project file. We want it to be set if IsMergedTestRunnerAssembly is set. Directory.Build.props is very early - before a test's csproj has set IsMergedTestRunnerAssembly. However, Directory.Build.targets is very late - after Microsoft.Managed.Core.targets. In it, EmitCompilerGeneratedFiles is set to false if unspecified, and CompilerGeneratedFilesOutputPath is set iff EmitCompilerGeneratedFiles is set. We can successfully override EmitCompilerGeneratedFiles in Directory.Build.targets, but then the path in unset which leads to a warning and no output.

I feel like I'm missing something about best practices and conditional defaults. Is there a typical solution to this?

The best that I can think of is having all proj (or just the ones for merged tests at first?) manually import a new file. It could

• Set a default for EmitCompilerGeneratedFiles
• Do the item munging that is needed for "extern alias"es to work for the merged wrappers to refer to individual tests
• Replace the line moving MergedWrapperProjectReference to ProjectReference in each proj file
• Consider defaults for "normal" proj (say, include **/*.cs or something like that if nothing has been included already? I'm less convinced by this one, though in doing the proj rewriting for merged groups, seeing the line adding the same $(whatever).cs into the compile item in almost every proj already had me thinking about a possible default a bit)

[markples]

After poking around, Directory.Build.targets does appear to indeed to be too late in the imports from the SDK. I added a new file (src/tests/MergedTestRunner.targets - would welcome new naming. I chose .targets over .props because it is imported at the end of the .csproj, which seems to be the general convention, though it does not introduce any actual Targets) and put the merged test duplication in there. I did not try to do something more general (like the 4th bullet in my previous comment).

[trylek]

Thanks Mark, I really like this approach! I believe it also opens the door for various future merged wrapper modifications without fighting the overcomplicated recursive build script system every single time.

[markples]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[markples]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[markples]

All failures are the known "NuGet-Migrations" one