Skip methods in test-merging if they use the named argument 'Skip' for the Fact attributes

[TIHan]

No description provided.

[ghost]

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

Issue Details

---

null

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

[TIHan]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[TIHan]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[TIHan]

@dotnet/jit-contrib this is ready

[markples]

@jkoritzinsky I was wondering about the intentions for the attribute system. Is the goal (subject to prioritizing the work of course) full compatibility with xunit, or doing that within some context of dotnet/runtime-specific goals? For example, was this intentionally left off because something like ActiveIssueAttribute should be used instead? Do we want to be propagating "skips" up to DevOps for reporting? And so on. Thanks!

[markples]

This seems like a prime example for the newer C# switch syntax

case "Xunit.ConditionalFactAttribute" when !hasSkip:

though perhaps the right solution is to put all of these into filterAttributes so that filtering can then drop them like other filters do (and skip reporting, etc., would have a single place to be implemented).

[jkoritzinsky]

I was wondering about the intentions for the attribute system. Is the goal (subject to prioritizing the work of course) full compatibility with xunit, or doing that within some context of dotnet/runtime-specific goals? For example, was this intentionally left off because something like ActiveIssueAttribute should be used instead?

The goal is to support whatever subset of xunit + Microsoft.DotNet.XUnitExtensions that we use in the src/tests tree or used similar features of in the legacy test system. There have been talks about building more of a true xunit replacement that's all source-generated, but this system is not meant to do that (though it might be a base for it in the future).

As we start to use more features of XUnit in these tests, I'm fully onboard with adding capabilities to the generator. I didn't include it initially as we rarely use [Fact(Skip = "Message")], so I didn't realize we were expecting to use it. I'd prefer that we use [ActiveIssue] when it's a good fit, but I don't see a reason to say "don't use the Skip property ever".

Do we want to be propagating "skips" up to DevOps for reporting? And so on. Thanks!

I don't think we emit code to record unconditional skips today, but that's a good idea. We should probably do that for reporting purposes.

[BruceForstall]

We really need to expand the documentation on how to disable tests -- https://github.com/dotnet/runtime/blob/main/docs/workflow/ci/disabling-tests.md -- to include examples of all the ways you can/should disable coreclr tests. There is a link there to the doc on disabling libraries tests.

[TIHan]

@BruceForstall there is more documentation regarding new ways to disable tests: https://github.com/dotnet/runtime/blob/main/docs/workflow/testing/libraries/filtering-tests.md

I think it's fair to handle the Skip named argument, but I think @markples is right that it might be better to use ActiveIssue instead of the Skip named argument for the particular test in which I used Skip. I will update that particular test and also update the documentation to talk about the Skip named argument.