Create source-only filter package

[Youssef1313]

Fixes #15361

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

[Youssef1313]

@copilot Create a new test project for the new package.

You MUST:

• Open a separate PR for that. Do not push to this PR.
• The new test project shouldn't reference the csproj. Instead, it should manually add the C# source files to Compile item.
• Write basic tests to validate the behavior of the APIs.

[Copilot]

@Youssef1313 I've opened a new pull request, #15641, to work on those changes. Once the pull request is ready, I'll request review from you.

[Youssef1313]

@nohwnd Would you like to change all namespaces in the source-only package?

[Youssef1313]

@copilot Please review this PR and add a comment with your findings. During review, ensure that all classes in the new package are internal and have embedded attribute (except when under #if IS_VSTEST_REPO).

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

src/Microsoft.TestPlatform.Filter.Source/FilterExpression.cs:128

• In ProcessOperator, the Operator.OpenBrace case should report a missing close parenthesis (resource MissingCloseParenthesis / "Error: Missing ')'"), but it currently formats the exception with MissingOperand. This changes the parse error behavior/message for inputs with an unmatched '('. Please use the missing-close-parenthesis message here instead of MissingOperand.
  src/Microsoft.TestPlatform.Filter.Source/TestCaseFilterExpression.cs:23
• Minor formatting: public class TestCaseFilterExpression: ITestCaseFilterExpression is missing a space before the :. This is inconsistent with the surrounding codebase style (e.g., TypeName : IInterface) and may be flagged by analyzers/formatting rules.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

src/Microsoft.TestPlatform.Filter.Source/FilterExpression.cs:128

• ProcessOperator treats an unmatched ( (i.e., Operator.OpenBrace still on the operator stack at the end) as a “Missing operand” error. This changes the error classification/message from the intended “Missing ')'” case and can mislead callers and users when diagnosing invalid filter strings. Use the existing MissingCloseParenthesis resource (and add an equivalent constant for the source-only build) for the OpenBrace case instead of MissingOperand.
  src/Microsoft.TestPlatform.Filter.Source/FilterExpressionWrapper.cs:57
• In the source-only build (!IS_VSTEST_REPO), this constructor no longer validates filterString (the ValidateArg.NotNullOrEmpty guard is compiled out). Passing null will now fail later with a less clear exception rather than ArgumentNullException. Consider adding an explicit if (filterString is null) throw ... / if (filterString.Length == 0) throw ... in the !IS_VSTEST_REPO path to preserve the contract for consumers of the source-only package.
  src/Microsoft.TestPlatform.Filter.Source/TestCaseFilterExpression.cs:23
• Spacing in the class declaration (TestCaseFilterExpression: ITestCaseFilterExpression) doesn’t match the repo’s formatting rules (csharp_space_before_colon_in_inheritance_clause = true). Adjust to TestCaseFilterExpression : ITestCaseFilterExpression to stay consistent with the enforced formatting settings.

[Evangelink]

Code Review Summary

Overall this is a well-structured approach to extracting the filter implementation into a source-only NuGet package. The #if IS_VSTEST_REPO pattern keeps a single source of truth while allowing divergence for internal/external consumers, and the [Embedded] attribute usage follows the standard .NET convention for source-only packages.

Issues found (by priority)

Critical (blocks package functionality):

1. Missing <contentFiles> metadata in nuspec — Without declaring <contentFiles><files include="cs/**/*.cs" buildAction="Compile" /></contentFiles> in the <metadata> block, NuGet won't set buildAction="Compile" on the packaged .cs files. The package will install but the types won't be available to consumers. This makes the package non-functional.

Bug:
2. MissingCloseParenthesis → MissingOperand substitution in FilterExpression.cs — The Operator.OpenBrace error path now reports "Missing operand" instead of the correct "Missing close parenthesis". The MissingCloseParenthesis constant was not added to the #if !IS_VSTEST_REPO fallback constants. This also affects the IS_VSTEST_REPO path since the resource reference changed.

Medium:
3. Duplicated error message strings across Condition.cs, FilterExpression.cs, and FilterHelper.cs — TestCaseFilterFormatException is copy-pasted in multiple files. Consider extracting to a single FilterResources class.
4. Test project missing TestProject/IsTestProject — Tests may not run in CI. ExcludeMicrosoftNetTestSdk appears unused.
5. Dictionary vs ImmutableDictionary thread-safety — The source package uses mutable Dictionary instead of ImmutableDictionary, which could be a problem for consumers evaluating filters concurrently.

Low:
6. Missing space before : in TestCaseFilterExpression (SA1024 violation)
7. Verify nupkg file count of 13 after fixing the <contentFiles> metadata
8. Contains operator validation silently skipped in the source package — worth documenting

What looks good

• The file-sharing approach via <Compile Include> is clean and avoids duplication
• [Embedded] attribute usage is correct
• Test coverage is comprehensive (parsing, evaluation, operators, error cases, fast filter)
• Test project cleverly uses ExcludeAssets="compile" on ObjectModel to avoid conflicts
• The IS_VSTEST_REPO define being set in Directory.Build.targets with an opt-out for the filter package is well-designed

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/Microsoft.TestPlatform.Filter.Source/FilterExpressionWrapper.cs:56

• In the source-only (non-IS_VSTEST_REPO) build, the constructor no longer validates filterString (null/empty). This changes behavior vs the in-repo build and can lead to less helpful exceptions (e.g., NullReferenceException during parsing). Consider using ArgumentException.ThrowIfNullOrEmpty(filterString) (or equivalent) outside the #if so validation is consistent in both builds without depending on ValidateArg.
  src/Microsoft.TestPlatform.Filter.Source/FilterHelper.cs:43
• In the source-only (non-IS_VSTEST_REPO) build, Escape/Unescape skip the null argument validation (currently inside #if IS_VSTEST_REPO). That means passing null will throw NullReferenceException instead of ArgumentNullException, and behavior diverges from the ObjectModel API contract. Consider using ArgumentNullException.ThrowIfNull(str) unconditionally (and keep ValidateArg if desired for the repo build) so both configurations behave the same.

[nohwnd]

Looks good. Consider adding integration test to Library.IntegrationTests that uses the package to ensure it works.

[Youssef1313]

@copilot add an integration test in Microsoft.TestPlatform.Library.IntegrationTests for this package.

[Youssef1313]

@copilot add an integration test in Microsoft.TestPlatform.Library.IntegrationTests for this package.

[Youssef1313]

@copilot Add an integration test in Microsoft.TestPlatform.Library.IntegrationTests for this package.

[Youssef1313]

@copilot Please add explicit #nullable enable in all files in C# code files in Microsoft.TestPlatform.Filter.Source.

[Copilot]

Pull request overview

Copilot reviewed 23 out of 24 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

src/Microsoft.TestPlatform.Filter.Source/FilterExpressionWrapper.cs:58

• In the !IS_VSTEST_REPO build (i.e., when this code is embedded via the source-only package), the constructor does not validate filterString at all. In-repo it throws for null/empty via ValidateArg.NotNullOrEmpty, but out-of-repo it may fail later with less clear exceptions. Consider adding equivalent ArgumentNullException/ArgumentException validation in the else path to keep behavior consistent.
  src/Microsoft.TestPlatform.Filter.Source/FilterExpressionWrapper.cs:159
• In the !IS_VSTEST_REPO build, Evaluate does not check propertyValueProvider for null (the in-repo build does). If a consumer passes null, this will likely throw a NullReferenceException deeper in evaluation. Consider validating the delegate in the non-repo path too to provide a clear ArgumentNullException and keep behavior consistent.
  src/Microsoft.TestPlatform.Filter.Source/TestCaseFilterExpression.cs:90
• In the !IS_VSTEST_REPO build, MatchTestCase doesn’t validate propertyValueProvider for null, while the in-repo implementation does. Adding an ArgumentNullException check in the non-repo path would avoid null-derefs and keep behavior consistent between the in-repo and source-only-package builds.
  src/Microsoft.TestPlatform.Filter.Source/FilterHelper.cs:40
• The XML doc for Escape lists % as a character that will be escaped, but % isn’t included in SpecialCharacters/SpecialCharactersSet. Either update the documentation to match the actual escaped characters, or add % to the escaped character set if it is intended to be supported.