Honor /TestCaseFilter on the MTP execution path - #16282
Closed
Azat Mukhametshin (azat-msft) wants to merge 7 commits into
Closed
Honor /TestCaseFilter on the MTP execution path#16282Azat Mukhametshin (azat-msft) wants to merge 7 commits into
Azat Mukhametshin (azat-msft) wants to merge 7 commits into
Conversation
On the MTP-as-testhost path a /TestCaseFilter run arrives as sources with no specific tests, and MtpProxyExecutionManager.StartTestRun ran every test because MTP has no notion of the vstest filter expression. Now, when a TestCaseFilter is present, the source is discovered over MTP, the expression is evaluated against the discovered tests (traits and boolean operators included via TestCaseFilterExpression) and only the matching test-node uids are run. A non-matching filter runs no tests instead of the whole suite. Adds acceptance test RunMtpApplicationHonorsTestCaseFilter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fb7de7a6-b8a1-4ebb-8266-5c5a948f245c
Copilot started reviewing on behalf of
Azat Mukhametshin (azat-msft)
July 14, 2026 10:26
View session
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the Microsoft.Testing.Platform (MTP) execution path in vstest.console so /TestCaseFilter:<expr> is honored during execution (previously it was silently ignored and all tests ran). It does so by performing an MTP discovery pass when a filter is present (and no explicit /Tests list is provided), evaluating the vstest filter expression against discovered tests, and then executing only the matching test-node UIDs.
Changes:
- Add a filtered execution path in
MtpProxyExecutionManagerthat discovers tests and appliesTestCaseFilterExpressionto select which tests to run. - Add an acceptance integration test validating that an MTP run honors
/TestCaseFilterand only executes matching tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/Microsoft.TestPlatform.Acceptance.IntegrationTests/MtpUnderVstestTests.cs | Adds an acceptance test verifying /TestCaseFilter limits executed tests for an MTP app. |
| src/Microsoft.TestPlatform.CrossPlatEngine/Client/MTP/MtpProxyExecutionManager.cs | Implements discovery + filter evaluation for MTP execution when /TestCaseFilter is provided without specific tests. |
- BuildPropertyProvider now exposes every property carried on the test case (by its filter label) plus the Name/DisplayName aliases and traits, instead of a hard-coded FullyQualifiedName/DisplayName/Name subset. Filters on other properties (e.g. Source) previously evaluated to "no value" and silently matched nothing; they now behave like the classic path. - DiscoverSourceTests waits for the discovery-completion sentinel honoring the cancellation token and logs a warning on timeout, so a /TestCaseFilter is not silently evaluated against an incomplete discovery set. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fb7de7a6-b8a1-4ebb-8266-5c5a948f245c
Member
Author
|
Addressed the review feedback (commit 140a078):
Verified: |
Copilot started reviewing on behalf of
Azat Mukhametshin (azat-msft)
July 14, 2026 11:13
View session
Comment on lines
+105
to
+106
| // The filter matched nothing for this source: run no tests (rather than the whole | ||
| // suite). An empty, non-null list flows through as "run exactly these zero tests". |
| } | ||
| }; | ||
|
|
||
| connection.Start(source, EnvironmentVariables, MtpClientHelpers.GetConnectionTimeout()); |
Comment on lines
+463
to
+465
| // Wait for the discovery-completion sentinel, honoring cancellation. If it does not arrive within | ||
| // the window the discovered set may be incomplete, which would make a /TestCaseFilter run execute | ||
| // the wrong tests, so make that explicit in the logs instead of failing silently. |
Comment on lines
+468
to
+470
| EqtTrace.Warning( | ||
| "MtpProxyExecutionManager.DiscoverSourceTests: discovery for '{0}' did not signal completion within the wait window; a /TestCaseFilter may be evaluated against an incomplete discovery set.", | ||
| source); |
Copilot started reviewing on behalf of
Azat Mukhametshin (azat-msft)
July 14, 2026 12:23
View session
Comment on lines
+113
to
+115
| // The filter matched nothing for this source: run no tests (rather than the whole | ||
| // suite). An empty, non-null list flows through as "run exactly these zero tests". | ||
| if (testsToRun.Count == 0) |
Comment on lines
+436
to
+438
| var discovered = new List<TestCase>(); | ||
| var completed = new ManualResetEventSlim(false); | ||
|
|
Comment on lines
+471
to
+475
| // Wait for the discovery-completion sentinel, honoring cancellation. If it does not arrive within | ||
| // the window the discovered set may be incomplete, which would make a /TestCaseFilter run execute | ||
| // the wrong tests, so make that explicit in the logs instead of failing silently. | ||
| if (!completed.Wait(TimeSpan.FromSeconds(3), _cancellationTokenSource.Token)) | ||
| { |
The previous comment claimed an empty, non-null test list would flow through as "run exactly these zero tests". That is misleading: the continue skips RunSource, and RunSource cannot express an empty selection (it omits the MTP tests filter when the list is empty, which MTP treats as "run every test"). Reword the comment so a future change does not remove the continue and accidentally run the whole suite for a non-matching filter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fb7de7a6-b8a1-4ebb-8266-5c5a948f245c
Copilot started reviewing on behalf of
Azat Mukhametshin (azat-msft)
July 14, 2026 12:29
View session
| private List<TestCase> DiscoverSourceTests(string source, IInternalTestRunEventsHandler eventHandler) | ||
| { | ||
| var discovered = new List<TestCase>(); | ||
| var completed = new ManualResetEventSlim(false); |
Comment on lines
+564
to
+565
| } | ||
|
|
Member
Author
|
No more reviews are needed, the PR is a WIP |
- Start the discovery pass with no environment variables so the data-collector profiler variables (execution-only) are not injected, mirroring MtpProxyDiscoveryManager. - Dispose the ManualResetEventSlim in DiscoverSourceTests via a using declaration to avoid leaking a wait handle. - Reword the discovery-completion comment/warning to reflect the ordered stream guarantee: the discovered set is complete once the DiscoverTests response returns; the sentinel wait is a non-critical drain. - Add an acceptance test asserting a non-matching /TestCaseFilter runs zero tests on the MTP path instead of the whole suite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 61d9a1c3-3c9f-4dab-8ba3-5baba8d55ed6
Copilot started reviewing on behalf of
Azat Mukhametshin (azat-msft)
July 14, 2026 13:58
View session
Comment on lines
+432
to
+436
| /// <summary> | ||
| /// Runs an MTP discovery pass against <paramref name="source"/> and returns the discovered tests. Used | ||
| /// to resolve a <c>/TestCaseFilter</c> into a concrete set of tests to run by uid. | ||
| /// </summary> | ||
| private List<TestCase> DiscoverSourceTests(string source, IInternalTestRunEventsHandler eventHandler) |
- RunMtpApplicationSurfacesPerTestStandardOutput asserted the old 4-test summary (2,1,1); the shared MtpMSTestProject asset gained a 5th test (RunSettingsEnvironmentVariableIsInjected), so the run is (3,1,1). This is a semantic collision between microsoft#16283 and microsoft#16284 that only surfaces when both are on main. Update the expected summary. - Address review feedback: extract the near-identical MTP discovery routine from MtpProxyDiscoveryManager and MtpProxyExecutionManager into a single MtpClientHelpers.DiscoverSourceTests helper so the two paths cannot drift (timeouts, cancellation, sentinel drain, env-var policy). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 61d9a1c3-3c9f-4dab-8ba3-5baba8d55ed6
Copilot started reviewing on behalf of
Azat Mukhametshin (azat-msft)
July 14, 2026 14:45
View session
Comment on lines
+201
to
+205
| // /TestCaseFilter must scope an MTP run just like it does on the classic path. MTP has no notion of the | ||
| // vstest filter expression, so vstest.console discovers the app, evaluates the expression against the | ||
| // discovered tests and runs only the matching test-node uids. Before this the filter was silently | ||
| // ignored and the whole suite ran (2/1/1). The filter here selects only the two passing tests. | ||
| [TestMatrix(testHost: Target.Net)] |
Comment on lines
+226
to
+229
| // A /TestCaseFilter that matches nothing must run zero tests on the MTP path, not fall back to running | ||
| // the whole suite. This guards the regression the feature targets: before, a non-matching filter was | ||
| // silently ignored and every test ran (2/1/1). Here the filter matches no test, so nothing runs. | ||
| [TestMatrix(testHost: Target.Net)] |
Comment on lines
+504
to
+505
| return name => properties.TryGetValue(name, out List<string>? values) ? values.ToArray() : null; | ||
| } |
BuildPropertyProvider now returns the single value directly (instead of a one-element array) when a property has exactly one value, matching how the classic TestCaseFilterExpression provider behaves. Also generalize the two TestCaseFilter test comments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 61d9a1c3-3c9f-4dab-8ba3-5baba8d55ed6
Copilot started reviewing on behalf of
Azat Mukhametshin (azat-msft)
July 14, 2026 19:36
View session
Comment on lines
+113
to
+120
| // The filter matched nothing for this source. Skip the source entirely: RunSource | ||
| // cannot express "run zero tests" — it only sends the MTP tests filter when the list | ||
| // has entries and otherwise omits it, which MTP treats as "run every test". So calling | ||
| // RunSource with an empty list would run the whole suite; the continue avoids that. | ||
| if (testsToRun.Count == 0) | ||
| { | ||
| continue; | ||
| } |
Comment on lines
+416
to
+420
| var filterExpression = new TestCaseFilterExpression(filterWrapper); | ||
|
|
||
| List<TestCase> discovered = MtpClientHelpers.DiscoverSourceTests(source, eventHandler.HandleLogMessage, _cancellationTokenSource.Token); | ||
|
|
||
| var matched = new List<TestCase>(); |
Member
|
feels like we are adding bunch of overhead by doing the discovery and then filtering, lets hold on on this one, ideally we would get official mtp client from MTP (by extracting the one in VS) and use it here, so we don't have to keep fixing the client over an over, working on that one. |
Azat Mukhametshin (azat-msft)
marked this pull request as draft
July 17, 2026 09:59
Member
|
Sorry for the long silence on this one. First, an update on what you were told to wait for: the official client landed. #16300 retargeted the MTP client onto The part I got wrong: I assumed the rewrite would bring filtering with it. It did not. I checked I filed #16379 to track it, and I wrote your approach into the issue, because that is the part that is expensive to rediscover:
Thank you for both of those, the second one especially. That empty run is the same class of bug as the one this PR fixes, and it would have shipped hidden inside the fix if you had not handled it. I am closing this PR. Not because the work was wrong, it was not, but because the diff cannot be rebased any more, it edits If you want to pick up #16379 on top of the current client, please do, it is yours. If not, that is fine too, it is written down now and it will not get lost again. 🤖 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On the
vstest.consolepath that runs a Microsoft.Testing.Platform (MTP) app as its own testhost (#16201),/TestCaseFilter:<expr>was silently ignored during execution — every test ran regardless of the filter./Tests(specific names) worked, but any filter expression was dropped.Root cause
A
/TestCaseFilterrun reachesMtpProxyExecutionManageras sources with no specific tests.BuildWorkthen yields the source withnulltests andRunSourceruns everything. MTP has no notion of the vstest filter expression, and nothing translated it, so the filter had no effect.Fix
When a
TestCaseFilteris present and there are no specific tests, discover the source over MTP, evaluate the expression against the discovered tests withTestCaseFilterExpression(traits + boolean operators, same semantics as the classic path), and run only the matching test-node uids. A non-matching filter now runs zero tests instead of the whole suite./Testsis unchanged.Test
New acceptance test
RunMtpApplicationHonorsTestCaseFilterruns the MTP app with/TestCaseFilter:"DisplayName~TestPasses"and asserts only the two matching tests run (2/0/0) instead of the full 2/1/1 suite.Validation
RunMtpApplicationHonorsTestCaseFilter: 2/2 matrix cases pass (fails onmain— filter ignored, whole suite runs).Microsoft.TestPlatform.CrossPlatEngineand the acceptance project: clean.~BasicTests-> 2 (was 33); non-matching -> 0.