Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 22, 2025

  • Analyze the issue and understand the codebase
  • Identify key files to modify
  • Implement validation in TestCategoryAttribute to treat empty string as null
  • Modify filter condition logic to match empty string with null categories
  • Update FastFilter to handle empty string filters
  • Add comprehensive unit tests for TestCategoryAttribute (5 tests)
  • Add comprehensive unit tests for Condition empty string handling (15 tests)
  • Add integration tests for empty category filtering with all operators (4 tests)
  • Address PR feedback: Convert ConditionTests to use MSTest assertions
  • Fix formatting issues (SA1513, IDE2003) - add required blank lines
  • Verify logic correctness with simulation
  • Verify code follows .editorconfig style guidelines
  • Verify no public API surface changes needed
  • Run CodeQL security scan (no issues detected)
  • Build and run tests (blocked by external network dependency issues)

Summary

Successfully implemented support for filtering uncategorized tests using empty TestCategory values.

Recent Changes

  • Fixed formatting issues by adding blank lines after closing braces where required by StyleCop rules
  • Addressed SA1513 errors in Condition.cs
  • Addressed IDE2003 error in FastFilter.cs
Original prompt

This section details on the original issue you should resolve

<issue_title>Filter for tests that are uncategorized</issue_title>
<issue_description>

Discussed in #5134

Originally posted by mscottford February 25, 2025
I'd like the ability to generate a list of tests that don't have a category specified. My use case is that I'm trying to monitor the number of tests in my test suite that have not been categorized so that I can ensure the number continues to go down over time as the team adds categories to tests.

I found microsoft/vstest#2818 which concluded with a note that such a feature wouldn't be implemented in vstest, so I thought I'd bring up the idea here.

I took a peek at the source for --treenode-filter to get a sense about whether or not it would work. The only thing that isn't clear to me from reading through it is how categories would be represented for tests that haven't had a category defined. Would that be an empty string? And then I could write an expression that searches for tests with a 0 length category? Perhaps with ^$.

</issue_description>

<agent_instructions>can you try implementing this? focus on testing it heavily. ensure that empty string category either fails in validation of TestCategory attribute, or is considered the same as null category.</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@nohwnd Yeah I guess figuring out the most intuitive way to represent null is the hardest thing here. It should also survive easily being provided on command line to a child process, and being combined with other categories. E.g. run all tests that are non categorized, or have short run time:

category == short || category == ???

@nohwnd Our proposal here is to simply leave the value empty, such as `testcategory=`

This has some edge cases regarding quotes and spaces.

for --filter

The current implementation already seems to be cutting trailing whitespace between = and the end, or the next operator.

testcategory=aaa , testcategory= aaa will match [TestCategory("aaa")] in both cases.

testcategory= now fails, so while [TestCategory("")] and [TestCategory(" ")] [TestCategory(null)] are all possible to define, they are unmatchable by the current filter. After the change only [TestCategory(" ")] will be unmatchable, because of the whitespace trimming. I think this is okay, as we would not recommend anyone to use whitespace only based categories.

" is used as literal, so as long as the user properly escapes the value, it can be used in the filter, if you specify testcategory=`"aaa`" it will match [TestCategory("\"aaa\"")]

This makes it impossible for us to handle cases where the category is surrounded by whitespace, or is just whitespace [TestCategory("aaa ")]` because we cannot provide the value including the whitespace, because of the trimming and adding quotes will include the quotes in the literal. This is also okay to me, because the current filter already has no way of matching those cases either.

This would of course apply to all the operators and possible categories. Special care needs to be taken around the implicit filter, that is defined as FullyQualifiedName~theprovidedstring, because there would empty string to semantically mean "no filter" rather than "all methods that have empty fqn", but providing empty string to --filter already throws "Unhandled exception. System.ArgumentNullException: Value cannot be null. (Parameter 'filterString')" which is ideal.

We might consider which filters do and do not support empty value, because for example for the FullyQualifiedName it probably does not make sense to accept empty value, and it might prevent the user from providing an incorrect filter.

for --tree-node-filter

TBD

@nohwnd Adding to 3.10 to keep it on our radar, but design needs to be fiinalized first.

Fixes #5136


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] Add filter for uncategorized tests Add support for filtering uncategorized tests with empty TestCategory filter Oct 22, 2025
Copilot finished work on behalf of nohwnd October 22, 2025 08:19
Copilot AI requested a review from nohwnd October 22, 2025 08:19
Copilot AI requested a review from Evangelink October 31, 2025 08:58
Copilot finished work on behalf of Evangelink October 31, 2025 08:58
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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make sure to have consistent behavior in TestCategoryBaseAttribute as well.
Or instead of simply ignoring null/empty/whitespace, it should be a clear error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation treats empty/null/whitespace as uncategorized in TestCategoryAttribute. Since TestCategoryBaseAttribute is abstract and used for custom implementations, the validation is in the concrete TestCategoryAttribute class. Users creating custom category attributes can implement their own validation logic in their derived classes. Would you prefer we throw an ArgumentException for empty/null/whitespace instead of silently treating them as uncategorized?

Guard.NotNull(propertyValueProvider);

bool matched = false;
foreach (string name in FilterProperties.Keys)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this whole implementation is a copy from vstest.

  1. Is there no public API from VSTest that we can call here?
  2. I think we should get those changes first in vstest repo, test it there, and then replicate here in testfx.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

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.

Copy link
Contributor Author

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?

Copilot AI requested a review from nohwnd November 4, 2025 11:55
Copilot finished work on behalf of nohwnd November 4, 2025 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Filter for tests that are uncategorized

4 participants