Conversation
…inclusion (#4656) When filtering tests by UID (e.g., from Visual Studio), class names like "ABCV" would incorrectly match "ABCVC" because the filter used string.Contains() for matching. This fix changes CouldMatchUidFilter to use word-boundary matching: - Class names must be bounded by '.', '<', or '+' - Method names must be preceded by '.' and followed by '.', '<', or '(' This ensures "ABCV" only matches the exact class name, not substrings like "ABCVC". Fixes #4656
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
Instead of using word-boundary matching for class names, use exact prefix matching with namespace.classname to ensure we don't match classes with the same name in different namespaces.
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. The changes correctly fix the substring matching issue while maintaining proper word-boundary semantics for test UIDs. The implementation properly handles the test ID format and adds comprehensive test coverage. |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. Review SummaryThis PR effectively fixes issue #4656 where substring matching in UID filters caused incorrect test inclusion. The implementation is sound: Correctness: The change from substring matching to prefix and word-boundary matching correctly prevents ABCV from matching ABCVC tests. CLAUDE.md Compliance:
Test Coverage: Comprehensive regression tests in both the engine tests and integration tests validate the fix works correctly in both source-generated and reflection modes. The architectural approach is particularly elegant—by using namespace-aware prefix matching with proper delimiters, the fix ensures exact matches without needing complex regex patterns. |
| var backtickIndex = className.IndexOf('`'); | ||
| if (backtickIndex > 0) | ||
| { | ||
| classNameForMatching = className.Substring(0, backtickIndex); |
There was a problem hiding this comment.
classNameForMatching = className.AsSpan().Slice(0, backtickIndex); and make ReadOnlySpan<char> classNameForMatching
Could prolly use ValueStringBuilder for expectedClassPrefix and expectedGenericClassPrefix but that's pretty pendanitc
…ching
The CouldMatchUidFilter method was failing to match tests when using
VS Test Explorer because:
1. For nested types (e.g., Outer+Inner), Type.Name only returns the
innermost class name, but UIDs contain the full path with '+' separators
2. For classes with constructor parameters, UIDs contain '(' after the
class name (e.g., MyClass(String).0.0.Method), but the code only
checked for '.' or '<'
This fix:
- Adds BuildClassNameForMatching() to construct the full nested type
hierarchy matching TestIdentifierService.WriteTypeNameWithGenerics
- Validates the character after the class name prefix is a valid
boundary: '.', '<', or '('
- Maintains protection against substring matching (ABCV vs ABCVC)
Fixes #4656 (follow-up issue after PR #4659)
https://claude.ai/code/session_016KqnWq2pDNM7sDLSn6LvRe
2 participants
Summary
Fixes #4656
When filtering tests by UID (e.g., from Visual Studio), class names like
ABCVwould incorrectly matchABCVCbecause the filter usedstring.Contains()for matching. This causedTestDiscoveryContext.AllTeststo include tests that shouldn't be there.Root Cause
In
MetadataFilterMatcher.CouldMatchUidFilter(), the code used substring matching:This meant that when checking if test
ABCV.B2matches a filter containing UID forABCVC.B2, it would incorrectly returntruebecause "ABCVC" contains "ABCV" as a substring.The Fix
Changed to use word-boundary matching:
.(namespace/indices),<(generics), or+(nested types).and followed by.,<, or(This ensures
ABCVonly matches the exact class name, not substrings likeABCVC.Test plan
TUnit.TestProject/Bugs/4656/TUnit.Engine.Tests/OverlappingClassNameFilterTests.csABCVC/B2runs exactly 2 tests (not 4 if bug exists)ABCV/*runs exactly 3 tests (not 6 if bug exists)