Support multiple conditions in [ConditionalFact] and [ConditionalTheory]#619
Conversation
Modifies [ConditionalFact] and [ConditionalTheory] attributes to accept a variable number of condition member names. Modifies ConditionalFactDiscoverer and ConditionalTheoryDiscoverer to evaluate all of the conditions in the attributes. Conditions are logically "and'ed", so any condition returning false marks the test to be skipped. Existing tests using [ConditionalFact] or [ConditionalTheory] do not need to be modified. The only behavioral change is the phrasing of the skip message which now begins "Conditions not met:..." Fixes dotnet#610
|
Hi @roncain, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
|
Reviewers: this was tested by manually creating the following tests: The result of running these tests looks like this: |
|
LGTM. Thanks. |
|
Thanks @stephentoub . Is it okay for me to merge or would you like others eyes on this? I am considering your option 3 (special exception throw for custom skip messages) for a future enhancement to these discoverers, but not as part of this PR. |
|
@Priya91, could you take a look as well? |
| List<string> falseConditions = new List<string>(); | ||
|
|
||
| if (symbols.Length == 2) | ||
| foreach (string entry in conditionMemberNames) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
I'm all done being nitpicky. Thanks, Ron :). |
|
I'm waiting for feedback from @Priya91 or @iamjasonp and will merge when I hear back. Alternatively, anyone else can feel free to merge at any time. Feedback from @bartonjs was incorporated into the 2nd commit. |
| string[] symbols = conditionMemberName.Split('.'); | ||
| Type declaringType = testMethodDeclaringType; | ||
|
|
||
| if (symbols.Length == 2) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Nice 😄 LGTM |
|
I was OOF yesterday, and saw this only now. LGTM! :) |
This update allows us to use multiple conditions in [ConditionalFact] tests. This feature was added to xunit.netcore.extensions with PR dotnet/buildtools#619, and now that the latest package is available, we can use it.
This update allows us to use multiple conditions in [ConditionalFact] tests. This feature was added to xunit.netcore.extensions with PR dotnet/buildtools#619, and now that the latest package is available, we can use it.
Modifies [ConditionalFact] and [ConditionalTheory] attributes to accept
a variable number of condition member names.
Modifies ConditionalFactDiscoverer and ConditionalTheoryDiscoverer to
evaluate all of the conditions in the attributes. Conditions are
logically "and'ed", so any condition returning false marks the test
to be skipped.
Existing tests using [ConditionalFact] or [ConditionalTheory] do not
need to be modified. The only behavioral change is the phrasing of
the skip message which now begins "Conditions not met:..."
Fixes #610