[Efficiency Improver] perf: skip PropertyBag construction in BFSTestNodeVisitor when filter has no property expressions

[Evangelink]

Add ContainsPropertyFilters to TreeNodeFilter — computed once at parse time. When false, BFSTestNodeVisitor reuses a static empty PropertyBag instead of allocating a new PropertyBag (+ N Property linked-list nodes) per traversed test node.

Proxy metric: heap allocations per test node during BFS traversal with a TreeNodeFilter that contains no [Trait=Value] property conditions (the common case).
Before: 1 PropertyBag + N Property nodes per node (N = number of properties)
After: 0 allocations (static empty bag reused)

Fixes #7883

[Copilot]

Pull request overview

This PR reduces per-node allocations in the MSTest engine’s BFS traversal by avoiding PropertyBag construction when the TreeNodeFilter does not contain any property-based filter expressions.

Changes:

• Add TreeNodeFilter.ContainsPropertyFilters, computed once during filter parsing by scanning the parsed filter expression tree.
• Update BFSTestNodeVisitor to reuse a static empty PropertyBag when ContainsPropertyFilters is false, avoiding per-node allocations.

Show a summary per file

File	Description
src/Platform/Microsoft.Testing.Platform/Requests/TreeNodeFilter/TreeNodeFilter.cs	Adds ContainsPropertyFilters and a helper to detect presence of property filter expressions in the parsed filter.
src/Adapter/MSTest.Engine/Engine/BFSTestNodeVisitor.cs	Reuses a static empty PropertyBag when property filters aren’t present; otherwise constructs the bag as before.

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 0

[github-actions]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-04-28
Repository: microsoft/testfx

Key Findings

The PR modifies only production code (src/) — no test files were changed. The optimization is logically sound, but the new code path lacks direct test coverage in two places:

1. ContainsPropertyFilters property not tested — TreeNodeFilterTests covers every parsing scenario but never asserts the new flag's value. Since the entire optimization is predicated on this flag, a parsing regression (e.g., a new expression type not recognized by HasPropertyFilterExpression) would silently misroute the BFS logic.

2. ContainsPropertyFilters = true BFS path not exercised — All BFSTestNodeVisitorTests use path-only filters, exercising only the EmptyPropertyBag optimization. The new PropertyBag(currentNode.Properties) path (when the filter contains property expressions) has no BFS-level test.

Recommendations

• Add [DataRow] tests to TreeNodeFilterTests asserting ContainsPropertyFilters is false for path-only filters and true for property-bearing filters.
• Add a BFSTestNodeVisitorTests test using a filter like "/A[Tag=Fast]" to cover the full-bag code path and verify correct node inclusion/exclusion based on properties.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[github-actions]

[Coverage] No BFSTestNodeVisitorTests test exercises the ContainsPropertyFilters = true branch — i.e., a TreeNodeFilter that contains a property expression (e.g. "/A[Tag=Fast]").

All existing BFS tests use path-only filters, so only the optimised EmptyPropertyBag path is exercised. If a regression were introduced in the new PropertyBag(currentNode.Properties) path (e.g., properties not being forwarded correctly), it would go undetected.

Suggestion: Add a BFS test such as:

// filter has a property expression — ContainsPropertyFilters == true
var filter = new TreeNodeFilter("/A[Tag=Fast]");
// node has a TestMetadataProperty("Tag", "Fast") → should be included
// node has no matching property → should be excluded

This validates both the true path and that the optimisation is consistent with the non-optimised path.

[github-actions]

[Coverage] The new ContainsPropertyFilters property has no unit tests.

TreeNodeFilterTests already has tests for every other parsing behavior, but none assert the value of ContainsPropertyFilters. The optimization in BFSTestNodeVisitor is entirely predicated on this flag being computed correctly, so a regression here would silently cause nodes to be mis-filtered (empty bag used when properties should be inspected).

Suggestion: Add [DataRow] cases to TreeNodeFilterTests covering:

• /**, /A/B, /(A|B), /(A&B) → ContainsPropertyFilters == false
• /*.UnitTests[Tag=Fast], /**[A=B], /(A[Tag=Fast]&B) → ContainsPropertyFilters == true

[github-actions]

Summary

Workflow: Expert Code Reviewer 🧠
Date: 2026-04-28
Repository: microsoft/testfx

Key Findings

[Defensive — Minor] EmptyPropertyBag is a shared, reusable mutable object whose immutability is assumed but not enforced. See inline comment on line 14 of BFSTestNodeVisitor.cs.

Positive Observations

• Correctness of the optimization: HasPropertyFilterExpression correctly traverses the full FilterExpression tree at any nesting depth (ValueAndPropertyExpression → direct hit; OperatorExpression → recursive). When ContainsPropertyFilters is false, MatchFilterPattern can never reach the ValueAndPropertyExpression switch arm, so MatchProperties is never invoked and the empty PropertyBag is never inspected. The optimization is sound.
• Thread safety: PropertyBag.AsEnumerable() captures current field values into a new enumerator without mutating the bag. Multiple concurrent BFSTestNodeVisitor instances sharing the same EmptyPropertyBag via reads are safe given the current implementation.
• API visibility: ContainsPropertyFilters is correctly scoped as internal — MSTest.Engine is explicitly listed in Microsoft.Testing.Platform's InternalsVisibleTo declarations, so cross-assembly access compiles without issue.
• Constructor equivalence: new PropertyBag(currentNode.Properties) (the params IProperty[] overload) produces the same linked-list structure as the deleted CreatePropertyBagForFilter — both iterate in forward order and prepend, yielding identical lookup behavior under Any().

Overall Assessment

The change is well-reasoned and achieves its stated goal (zero allocations per node for the common no-property-filter case). The single concern is a forward-looking defensive coding observation about the mutable shared bag, not a current bug.

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[github-actions]

[Defensive] EmptyPropertyBag is a shared mutable static instance backed by a PropertyBag that exposes a public Add method.

Mechanism: Today MatchesFilter only reads the bag, so concurrent reads across parallel BFSTestNodeVisitor invocations are safe. However, PropertyBag is not immutable — any future code path (inside this class or a refactor of MatchesFilter) that calls .Add() on the shared instance would permanently corrupt the singleton, silently causing all subsequent no-property-filter traversals to behave incorrectly.

Suggestion: Consider adding a brief comment like // Read-only; must never be mutated to make the invariant explicit, or—if it doesn't break the allocation goal—use a local new PropertyBag() per invocation when !ContainsPropertyFilters (the empty constructor is allocation-cheap relative to the full filter path). At minimum, document the expected immutability contract so the invariant survives future edits.

[Evangelink]

@copilot address review comments

[Copilot]

@copilot address review comments

Addressed in 53855c0:

1. Added a // Read-only; must never be mutated comment to EmptyPropertyBag to make the invariant explicit.
2. Added ContainsPropertyFilters_ReturnsExpectedValue with 7 [DataRow] cases to TreeNodeFilterTests covering both false (path-only: /**, /A/B, /(A|B), /(A&B)) and true (property-bearing: /*.UnitTests[Tag=Fast], /**[A=B], /(A[Tag=Fast]&B)) paths.
3. Added Visit_WhenFilterHasPropertyExpression_OnlyIncludesNodesMatchingProperty to BFSTestNodeVisitorTests, verifying that when ContainsPropertyFilters = true, nodes with a matching property are included and nodes without are excluded.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• 37bvsblobprodcus311.vsblob.vsassets.io
  • Triggering command: /usr/bin/dotnet dotnet restore test/UnitTests/MSTest.Engine.UnitTests/MSTest.Engine.UnitTests.csproj (dns block)
  • Triggering command: /usr/bin/dotnet dotnet build test/UnitTests/MSTest.Engine.UnitTests/MSTest.Engine.UnitTests.csproj -c Release (dns block)
  • Triggering command: /usr/bin/dotnet dotnet build test/UnitTests/MSTest.Engine.UnitTests/MSTest.Engine.UnitTests.csproj -c Release -f net9.0 (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)