-
Notifications
You must be signed in to change notification settings - Fork 289
Add support for filtering uncategorized tests with empty TestCategory filter #6779
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
base: main
Are you sure you want to change the base?
Changes from all commits
dc89468
ef1fd6a
c70bf34
03cdb90
2f0c2bd
cf36b81
3b1e7a7
e9dc082
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,8 +80,13 @@ | |
| switch (Operation) | ||
| { | ||
| case Operation.Equal: | ||
| // Special case: empty string filter value matches null/empty/whitespace property (uncategorized tests) | ||
| if (string.IsNullOrWhiteSpace(Value)) | ||
|
Check failure on line 84 in src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/Condition.cs
|
||
| { | ||
| result = multiValue is null or { Length: 0 }; | ||
| } | ||
|
Check failure on line 87 in src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/Condition.cs
|
||
Evangelink marked this conversation as resolved.
Show resolved
Hide resolved
Evangelink marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // if any value in multi-valued property matches 'this.Value', for Equal to evaluate true. | ||
| if (multiValue != null) | ||
| else if (multiValue != null) | ||
| { | ||
| foreach (string propertyValue in multiValue) | ||
| { | ||
|
|
@@ -96,27 +101,42 @@ | |
| break; | ||
|
|
||
| case Operation.NotEqual: | ||
| // all values in multi-valued property should not match 'this.Value' for NotEqual to evaluate true. | ||
| result = true; | ||
|
|
||
| // if value is null. | ||
| if (multiValue != null) | ||
| // Special case: empty string filter value matches null/empty property (uncategorized tests) | ||
| // So NotEqual to empty string should match tests WITH categories | ||
| if (string.IsNullOrWhiteSpace(Value)) | ||
|
Check failure on line 106 in src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/Condition.cs
|
||
| { | ||
| foreach (string propertyValue in multiValue) | ||
| result = multiValue is not null and { Length: > 0 }; | ||
| } | ||
| else | ||
| { | ||
| // all values in multi-valued property should not match 'this.Value' for NotEqual to evaluate true. | ||
| result = true; | ||
|
|
||
| // if value is null. | ||
| if (multiValue != null) | ||
| { | ||
| result = result && !string.Equals(propertyValue, Value, StringComparison.OrdinalIgnoreCase); | ||
| if (!result) | ||
| foreach (string propertyValue in multiValue) | ||
| { | ||
| break; | ||
| result = result && !string.Equals(propertyValue, Value, StringComparison.OrdinalIgnoreCase); | ||
| if (!result) | ||
| { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| break; | ||
|
|
||
| case Operation.Contains: | ||
| // Special case: empty string filter value matches null/empty property (uncategorized tests) | ||
| if (string.IsNullOrWhiteSpace(Value)) | ||
| { | ||
| result = multiValue is null or { Length: 0 }; | ||
| } | ||
Evangelink marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
Check failure on line 137 in src/Platform/Microsoft.Testing.Extensions.VSTestBridge/ObjectModel/Condition.cs
|
||
| // if any value in multi-valued property contains 'this.Value' for 'Contains' to be true. | ||
| if (multiValue != null) | ||
| else if (multiValue != null) | ||
| { | ||
| foreach (string propertyValue in multiValue) | ||
| { | ||
|
|
@@ -132,18 +152,27 @@ | |
| break; | ||
|
|
||
| case Operation.NotContains: | ||
| // all values in multi-valued property should not contain 'this.Value' for NotContains to evaluate true. | ||
| result = true; | ||
|
|
||
| if (multiValue != null) | ||
| // Special case: empty string filter value matches null/empty property (uncategorized tests) | ||
| // So NotContains empty string should match tests WITH categories | ||
| if (string.IsNullOrWhiteSpace(Value)) | ||
| { | ||
| foreach (string propertyValue in multiValue) | ||
| result = multiValue is not null and { Length: > 0 }; | ||
| } | ||
| else | ||
| { | ||
| // all values in multi-valued property should not contain 'this.Value' for NotContains to evaluate true. | ||
| result = true; | ||
|
|
||
| if (multiValue != null) | ||
| { | ||
| RoslynDebug.Assert(propertyValue != null, "PropertyValue can not be null."); | ||
| result = result && !propertyValue.Contains(Value, StringComparison.OrdinalIgnoreCase); | ||
| if (!result) | ||
| foreach (string propertyValue in multiValue) | ||
| { | ||
| break; | ||
| RoslynDebug.Assert(propertyValue != null, "PropertyValue can not be null."); | ||
| result = result && !propertyValue.Contains(Value, StringComparison.OrdinalIgnoreCase); | ||
| if (!result) | ||
| { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -177,16 +206,18 @@ | |
| ThrownFormatExceptionForInvalidCondition(conditionString); | ||
| } | ||
|
|
||
| for (int index = 0; index < 3; index++) | ||
| // Property name (parts[0]) and operator (parts[1]) must not be empty | ||
| if (RoslynString.IsNullOrWhiteSpace(parts[0]) || RoslynString.IsNullOrWhiteSpace(parts[1])) | ||
| { | ||
| if (RoslynString.IsNullOrWhiteSpace(parts[index])) | ||
| { | ||
| ThrownFormatExceptionForInvalidCondition(conditionString); | ||
| } | ||
|
|
||
| parts[index] = parts[index].Trim(); | ||
| ThrownFormatExceptionForInvalidCondition(conditionString); | ||
| } | ||
|
|
||
| // parts[2] (value) can be empty to support filtering for uncategorized tests | ||
| // Trim all parts | ||
| parts[0] = parts[0].Trim(); | ||
| parts[1] = parts[1].Trim(); | ||
| parts[2] = parts[2].Trim(); | ||
|
|
||
| Operation operation = GetOperator(parts[1]); | ||
| Condition condition = new(parts[0], operation, FilterHelper.Unescape(parts[2])); | ||
| return condition; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,9 +46,18 @@ internal bool Evaluate(Func<string, object?> propertyValueProvider) | |
| bool matched = false; | ||
| foreach (string name in FilterProperties.Keys) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this whole implementation is a copy from vstest.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're correct that this is copied from vstest. The VSTestBridge component is designed to provide compatibility with vstest behavior in the new Microsoft.Testing.Platform. The note at the top of these files states "This file is copied as-is from VSTest source code." For this feature, we're implementing it here first as part of the new testing platform. If the team prefers to implement it in vstest first and then port it here, I can hold off on these changes. What's the preferred approach? |
||
| { | ||
| // If there is no value corresponding to given name, treat it as unmatched. | ||
| // Special case: if filter contains empty string, check if property is null/empty (uncategorized) | ||
| bool hasEmptyStringFilter = FilterProperties[name].Any(string.IsNullOrWhiteSpace); | ||
|
|
||
| // If there is no value corresponding to given name, treat it as unmatched unless filtering for empty string. | ||
| if (!TryGetPropertyValue(name, propertyValueProvider, out string? singleValue, out string[]? multiValues)) | ||
| { | ||
| if (hasEmptyStringFilter) | ||
| { | ||
| matched = true; | ||
| break; | ||
| } | ||
Evangelink marked this conversation as resolved.
Show resolved
Hide resolved
Evangelink marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| continue; | ||
| } | ||
|
|
||
|
|
@@ -57,11 +66,16 @@ internal bool Evaluate(Func<string, object?> propertyValueProvider) | |
| string? value = PropertyValueRegex == null ? singleValue : ApplyRegex(singleValue); | ||
| matched = value != null && FilterProperties[name].Contains(value); | ||
| } | ||
| else | ||
| else if (multiValues is { Length: > 0 }) | ||
| { | ||
| IEnumerable<string?>? values = PropertyValueRegex == null ? multiValues : multiValues?.Select(ApplyRegex); | ||
| matched = values?.Any(result => result != null && FilterProperties[name].Contains(result)) == true; | ||
| } | ||
| else if (hasEmptyStringFilter) | ||
| { | ||
| // Empty array matches empty string filter | ||
| matched = true; | ||
| } | ||
|
|
||
| if (matched) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,11 +18,20 @@ public sealed class TestCategoryAttribute : TestCategoryBaseAttribute | |
| /// </param> | ||
| public TestCategoryAttribute(string testCategory) | ||
| { | ||
| List<string> categories = | ||
| [ | ||
| testCategory | ||
| ]; | ||
| TestCategories = categories; | ||
| // Treat empty or whitespace-only strings as null/uncategorized | ||
| // This ensures empty categories are not added to the list | ||
| if (string.IsNullOrWhiteSpace(testCategory)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should make sure to have consistent behavior in TestCategoryBaseAttribute as well.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current implementation treats empty/null/whitespace as uncategorized in |
||
| { | ||
| TestCategories = []; | ||
| } | ||
| else | ||
| { | ||
| List<string> categories = | ||
| [ | ||
| testCategory | ||
| ]; | ||
| TestCategories = categories; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
||
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.
This implementation also seems like a copy from vstest. I'd like to fix it in vstest first.
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.
Same as with FastFilter - this is copied from vstest as noted in the file header. Should we prioritize implementing this in vstest first, or is it acceptable to implement here in the VSTestBridge for the new platform and potentially backport later?