Add support for [ConditionalFact] based on TestProperties

[roncain]

We need a way to optionally run tests that are not normally
run by default. This includes tests that have special requirements
for setup or environment.

We also need such tests to show as "skipped" rather than being
silently stripped from the test results, as happens with the
xunit -notrait option.

[roncain]

Reviewers: this is a proposed approach to solving #958. It demonstrates how one could define a new TestProperty and use [ConditionalFact] that checks it. If we decide to take this approach, I will update the PR with whatever additional categories we need. The output of an OuterLoop test run for this particular test looks like this:

                     Discovering: Security.TransportSecurity.Tests (TaskId:2904)
                     Discovered:  Security.TransportSecurity.Tests (TaskId:2904)
                     Starting:    Security.TransportSecurity.Tests (TaskId:2904)
                        Https_ClientCredentialTypeTests.DigestAuthentication_RoundTrips_Echo [SKIP] (TaskId:2904)
                           "Domain_Joined_Tests_Enabled" returned false. (TaskId:2904)
                     Finished:    Security.TransportSecurity.Tests (TaskId:2904)
                      (TaskId:2904)
                     === TEST EXECUTION SUMMARY === (TaskId:2904)
                        Security.TransportSecurity.Tests  Total: 35, Errors: 0, Failed: 0, Skipped: 1, Time: 24.016s (TaskId:2904)

I'm using this PR to discuss the approach. The use of TestProperties allows the choice to be injected into the test binaries, meaning this will work equally well locally and in tests shipped off to execute in Helix. The use of a unique test property per category allows them to be set independently without a more complex "include these" and "exclude these" mechanism.

Note that ToF does not support [ConditionalFact] but as long as we define a dummy ConditionalFactAttribute in ToF tests, the tests will build and just naturally skip the tests

Feedback is welcome.
/cc @iamjasonp @zhenlan @StephenBonikowsky @hongdai

[roncain]

Reviewers: ConditionalFactDiscover only works with members of the type in which it appears, including base types or nested types. You can't specify an arbitrary helper type. So we either need to use derivation like this or duplicate the helper methods in every test class where we use them.

[iamjasonp]

Yuck. Derivation seems like the best solution here.

[roncain]

I should have added the note that this behavior matches what the 'nameof' operator does and is probably the reason it was scoped to this behavior. CoreFx has adopted nameof for its [ConditionalFact] usage.

[iamjasonp]

How does this work with multiple categories? For example, if something requires both Domain Join and Certificates, can we specify both, or does this mean we have another TestCategory?

---

For ToF tests, are we just assuming that all "special" tests are going to be skipped? For example, I would think that some of the "ambient credential" tests would work if the machines are on prem and domain joined - this approach with [ConditionalFact] basically means "we're ok with skipping these tests in ToF".

I don't disagree with this approach, just wondering if there's less "big hammer" way of doing this.

---

Is there any case where we want these test categories to be enabled by default then disabled on demand instead?

[roncain]

@iamjasonp -- one can't use multiple [ConditionalFact]'s per method, so if a particular test requires more than one condition, then yes, I believe a separate static bool method would need to be added for the combination (e.g. "I need to be both on prem and domain-joined"). Alternatively, if one condition always depended on a 2nd condition, then the helper method itself could call it, something like the way our Endpoint.cs resources call on additionally required resources.

I suspect in time we may want some conditions to involve code to auto-detect whether they are on or off by default (like the canonical example "Am I running elevated?"). In that case, I think we might consider a 3-state value for the TestProperty in those cases (true, false, auto) let the test helper decide. So my statement that all default to false is probably too strong. I suspect that if we introduce auto-detect for some, that will be their default. Naturally, we would want an explicit 'true' or 'false' to be able to override.

Update: I forgot to mention [ConditionalFact] is somewhat limited in the skip message it emits -- it just parrots back the member name appearing in the attribute. I'd like to consider recommending an improved [ConditionalFact] that allows the helper method to provide a better skip message. Or consider expanding [ConditionalFact] to allow multiple members to be invoked, allowing both a multi-condition expression as well as a better message for which of the conditions was not met.

[roncain]

I opened issue dotnet/buildtools#610 to allow [ConditionalFact] to evaluate multiple conditions

[roncain]

Reviewers: if there is buy-in of this approach, I would like to update this PR to adapt to review comments and add whatever conditions we know we need right now, then merge. This will lay the foundation for other conditions to be added as we discover them.

/cc @iamjasonp @zhenlan @StephenBonikowsky @hongdai @mconnew

[roncain]

Reviewers: I would now like review feedback and to merge this once approved, The intent of this PR is just to introduce the functionality and wire up one test to demonstrate it works. Changing individual tests to use it is expected to come as separate PR's. I have chosen condition names based on my read of #958 and so am looking for feedback on whether the initial set will do.

Note: if needed, we might change the default values based on OS if we think we need that. I am also hoping dotnet/buildtools#610 is approved so we can combine conditions, but I don't want this PR blocked by that.

/cc @iamjasonp @zhenlan

[iamjasonp]

so if a particular test requires more than one condition, then yes, I believe a separate static bool method would need to be added for the combination (e.g. "I need to be both on prem and domain-joined").

After dotnet/buildtools#619 is merged, it looks like we can specify multiple conditions, right? That alleviates any worries I have for this issue.

I suspect in time we may want some conditions to involve code to auto-detect whether they are on or off by default (like the canonical example "Am I running elevated?").

Yeah, that's probably true; that said from the test runner's perspective, I think it means that a condition is "implied auto detect" vs. "implied false"? Today it's "implied false", so no tests in any [ConditionalFact] will run unless explicitly turned on. This behaviour is probably ok today; we can probably solve this problem later.

if needed, we might change the default values based on OS if we think we need that.

Perhaps; it might be useful. Let's use #958 to explore this?

[zhenlan]

When a test has ConditionalFact on it, it means it will not work unless certain condition(s) is/are met, so it makes sense to me that these tests are disabled/skipped by default (before we have anything auto).

The naming can be tweaked as I prefer client/server prefixes for clarity, but that can be discussed in #958 and update later.

This PR LGTM. Thanks @roncain.