-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
src/tests tree test xunit-based source generated runner #60846
src/tests tree test xunit-based source generated runner #60846
Conversation
…nt Main method that runs all static parameterless [Fact] attributes in sequence, wrapped in a simple try-catch.
…_4 test suite to use the generator.
… basic cases of OS and runtime-specific filtering.
Tagging subscribers to this area: @dotnet/runtime-infrastructure Issue DetailsThis draft PR includes a Roslyn source generator that detects public methods defined in the current (being compiled) assembly or any referenced assembly that are marked with XUnit attributes and generates a basic Main method that runs all of the discovered tests in order and returns It has support for The enum files are copied from This PR also includes examples of using the new
cc: @trylek
|
namespace Xunit | ||
{ | ||
[Flags] | ||
public enum TestPlatforms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reflect on Microsoft.DotNet.XUnitExtensions
in generator to avoid duplicating these types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, but using the Roslyn symbol APIs to reflect like that is not fast. I think eventually making this generator live in dotnet/arcade might be a better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In future versions of the generator (v1 milestone of the new test infra), we need to support spitting out an XUnit-style results file as well as basic test filtering, so living in arcade would make more sense then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be careful not to regress managed build time too significantly. While ultimately we'll optimize it by compiling tests as larger apps, it remains to be seen whether compiling tests in merged form makes sense for all pipelines - some of the GC stress pipelines come to mind where some tests are timing out already today even without merging - so that the "standalone" mode is likely to stay as a valid way of running tests at least for niche scenarios. If we pay the reflection startup cost just once per Roslyn generation of all the wrappers, it's probably just fine but paying the cost for every single test may be significant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking in generator case, startup performance is not a major issue (e.g. we can discover types using reflection (or perhaps Roslyn symbol APIs) to collect required infos in a static constructor just once during the lifetime of compilation).
I don't like caching it like that due to the fact that it's technically not correct and can break some assumptions that Roslyn has about the lifetime of a generator (as Roslyn can throw them away or reuse them in many different ways including in compiler server scenarios)
src/tests/Common/XUnitWrapperGenerator/XUnitWrapperGenerator.cs
Outdated
Show resolved
Hide resolved
@jkoritzinsky, what is your thinking about those cases where we execute a |
There are a few options here:
|
From a cursory look, many of the tests that set If we really want to source-generate the entry-points for these tests, we could write a generator specifically for |
Thanks Jeremy, that sounds reasonable; naturally it would be ideal to simplify the test project structure wherever possible and the RunOnly tests with CLRTestProjectToRun (in fact there are more tests of this type that are incorrectly marked as BuildAndRun as discussed in the issue thread) certainly do add complexity so if we're on track for something simpler, I'm all for it. Further down the road we'll also need to resolve other corner cases like tests returning weird exit codes (some or all of these cannot be merged as the negative exit codes signify intended runtime crashes in negative tests). |
One other interesting side note regarding the ilproj projects: Many projects come in groups with the same entrypoint name (e.g. the 1500 or so TypeGeneratorTests under Loader all have the entrypoint |
Using extern aliases that should be possible, albeit not clean. |
…ames for tests like the TypeGenerator suite). Add support for theories to handle the RunOnly tests that aren't failure cases and don't require out-of-proc execution.
@trylek I've added support for reference aliases (to handle the TypeGeneratorTests problem) and theory support (to handle a large part of the RunOnly problem) with examples for each. |
Beautiful, thanks Jeremy! Just for my education, where do the reference alias infos actually live? Is that part of the reference assembly table in the ECMA metadata within the MSIL file? |
The alias name is passed to Roslyn through the project file. After compilation, I think it's just represented the same way an assembly-qualified reference written in IL is represented. The |
I've become able to expand my ilproj analyzer to run larger tests and I hit an interesting new twist: as you probably remember, last year Steve MacLean refactored platform-specific tests to use variant projects, e.g. here: https://github.com/dotnet/runtime/blob/main/src/tests/JIT/Regression/JitBlue/DevDiv_278523/DevDiv_278523_Target_32Bit.ilproj I think this particular combo is relatively easy to implement but there are other similar tests that only apply to certain OSes, architectures and / or other build modes. As these tests only build conditionally, their output assembly also gets only produced conditionally and to my knowledge there's no equivalent functionality to make the "extern alias" clauses conditional - when the conditional assembly is not produced (because the current build configuration doesn't support the test), the extern alias becomes kind of orphaned and fails the build. This is definitely a problem for constructing a target-agnostic wrapper. I'm almost done for today but I make it my priority to figure out the solution for this case tomorrow. |
For the arch-specific tests, you can add a ConditionalFact attribute that points to a property that checks if the pointer size is 32 or 64 bit. For OSes, the SkipOnPlatform attribute can be used to skip on specific operating systems. Basically we'll always compile the tests, but we'll either conditionally compile them into the runner, or conditionally execute them based on the detected platform. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me, thanks Jeremy! I haven't noticed any major blocking issues, just a few nitpicks.
testInfos = FilterForSkippedTargetFrameworkMonikers(testInfos, (int)filterAttribute.ConstructorArguments[0].Value!); | ||
break; | ||
case "Xunit.SkipOnCoreClrAttribute": | ||
if (options.GlobalOptions.RuntimeFlavor() != "CoreCLR") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be a big problem to postpone the platform / runtime check to the actual runtime execution? We can do it in a future change but ideally we should be able to use the same managed build for Mono and CoreCLR (I don't believe we're doing that now, we have two different legs for that purpose but that's also cost that could be reduced).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only mechanism known today to detect runtime flavor at runtime easily is though reflection, which is very heavyweight for the src/tests test tree. I'm trying to keep the platform checks as cheap as possible so as to not make the "bring up" tests in the JIT significantly less useful, which is why I try to hard-code them at generation time when possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I think it's OK to leave as is for now, I can look into it as part of the new-style Helix publishing; in particular I believe we're setting up about two dozens of environment variables that get set on the Helix execution machine and these include target architecture and OS so I guess we could use these for the runtime checks. We can also add an arbitrary number of additional environment variables as we need to express any of the execution characteristics.
testInfos = FilterForSkippedTargetFrameworkMonikers(testInfos, (int)filterAttribute.ConstructorArguments[0].Value!); | ||
break; | ||
case "Xunit.SkipOnCoreClrAttribute": | ||
if (options.GlobalOptions.RuntimeFlavor() != "CoreCLR") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be a big problem to postpone the platform / runtime check to the actual runtime execution? We can do it in a future change but ideally we should be able to use the same managed build for Mono and CoreCLR (I don't believe we're doing that now, we have two different legs for that purpose but that's also cost that could be reduced).
… (but not actual support since I don't feel like writing a parser right now) for the full dotnet test --filter syntax.
…line discussion.
…tBase.cs to just use the new generator instead as the generator is now implicitly referenced in the same scenarios where XunitBase.cs was used.
/azp run runtime-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
…dly with the dependent project having dependencies (like on the XUnit wrapper generator).
/azp run runtime-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
…hy my alternative design wasn't working.
7bf0c1e
to
db68ce4
Compare
/azp run runtime-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
This draft PR includes a Roslyn source generator that detects public methods defined in the current (being compiled) assembly or any referenced assembly that are marked with XUnit attributes and generates a basic Main method that runs all of the discovered tests in order and returns
100
if all tests finish with no exceptions or101
otherwise. It also includes support for a merged runner for future usage that reports test results with in the XUnit results format and runtime test filtering. The generator supports test filtering at compile time as well to simplify debugging.It has support for
[Fact]
and[ConditionalFact]
tests,[Theory]
and[ConditionalTheory]
parameterized tests, and all XUnitExtensions attributes exceptSkipOnCIAttribute
.The enum files are copied from
Microsoft.DotNet.XUnitExtensions
indotnet/arcade
.This PR also includes examples of using the new
[Fact]
-based approach with three test suites:[Fact]
attribute on the.entrypoint
method and making that method parameterless) that enables both standalone running of an IL test and using the source generator to generate a wrapper assembly in C# that runs the same tests.cc: @trylek