Consume FSAC test hierarchy and enable MTP tests - #2180
shayanhabibi wants to merge 7 commits into
Conversation
FsAutoComplete now reports grouping nodes and parent ids, and normalises parameterised test names itself. Rebuilding the tree here by splitting fully-qualified names would disagree with the server as soon as a project runs under Microsoft.Testing Platform, whose nodes carry parent ids the names do not encode. TestItemDTO carries Id, ParentId and IsLeaf, and discovery builds its hierarchy by following those links. getFullname_withNestedParamTests is gone: the server's FullName already distinguishes theory cases, so applying the same rule here appended the case parameters twice. Name inference stays for the TRX and AST paths, which report names alone.
Exposes the FSAC flag that routes projects setting IsTestingPlatformApplication through the Microsoft.Testing.Platform server protocol. Off by default.
Microsoft.Testing.Platform runs a test named by the opaque uid it was discovered under, rather than by a filter expression over its name, so a test carries that uid from discovery through to the run. A grouping node holds no uid of its own, so selecting one is read as the runnable tests beneath it. A test discovered through VSTest carries no uid and is still named by the filter expression, which is sent alongside.
shayanhabibi
left a comment
shayanhabibi
left a comment
There was a problem hiding this comment.
Open question: Should we discriminate between DTOs targeting VSTest/MTP instead of optional fields
|
See the other PR for the heart of the implementation. I do want to pursue some cleanup of Ionide settings (ie grouping them) and other facets, but this is the foundational work. Open Question: Should I continue adding these other facets, and we cherry pick them into separate PRs later/just merge them all later? |
Unique idsI haven't gotten to look closely at this, but I see one item that concerns me
This sounds like VSTest and MTP behave in different and incompatible ways through the same api, causing a leaky abstraction that requires consumers to understand how certain calls will behave based on implicit context. Is there a way we can bridge the gap? Have unique identifiers for both MTP and VSTest? I notice VsTestConsoleWrapper.RunTests can run a sequence of TestCases, and a TestCase can be constructed with FullyQualifiedName, ExecutorUri, and Source. To my understanding, that combination should be unique. Perhaps we can package these into a unique identifier. Then both VSTest and MTP could run specific tests by an "id". The id is provided by the server and interpreted by the server and treated as a black box by the client. Consumers could get themselves into trouble by trying to parse out id components, but that'd be unsupported shenanigans requiring some reverse engineering. Unique Ids and Test FiltersRelated. What happens if both test ids and a filter are specified in the run request? Union types don't properly exist over process boundaries. We could throw an error if both are specified. In any case, it's a behavior we should think about. |
|
Things get really finicky as we start to treat one field as another. Strictly speaking, display names are not unique ids. The default filter works on display names, not unique ids. There is a separate filter argument in MTP for specifying unique ids. For us in FSharp, where our tests are constructed as values instead of associated with types/members, we essentially treat them the same. So I don't disagree with you at all - yeah I'll backport VSTest API to fill fields that are required in MTP so we can remove fields that are optional only because one API uses it - however, we should be careful when communicating how filters work (as you point out). I don't actually know if MTP allows you to specify uid filters with normal name filters. Something I'll look into. But - we should be able to request both. I'll report back after review |
|
I think I didn't correctly communicate my full idea. I didn't mean to suggest stuffing unique ids into the filter or filters into unique ids. I would expect filter and id-based runs to be distinct. I'm suggesting that both filters and id-based runs are available for both MTP and VSTest, and work consistently between the two. All tests could be returned from FSAC with a unique id. FSAC could service run requests by these ids using any underlying platform.
UPDATE: I looked at the VSTest code for TestCase, and they're actually constructing a Guid based on the three TestCase members for internal use as a unique id. So I think we'd be pretty safe assuming the combo of FullyQualifiedName, ExecutorUri, and Source can act as an id. // Part of TestCase
private Guid GetTestId()
{
string text = Source;
try {
text = Path.GetFileName(text);
} catch {
}
string text2 = ExecutorUri?.ToString() + text;
text2 += GetFullyQualifiedName();
return EqtHash.GuidFromString(text2);
} |
FsAutoComplete now gives every test an opaque id that routes it to its project, target framework and platform, and runs a selection named by those ids. A test explorer item keeps the id FSAC reported for it, and a run sends the ids of the tests under the selection and nothing else: FSAC rejects ids sent together with a filter expression, and works out the projects to run from the ids themselves. The filter expression stays with the dotnet CLI path. A selected test FSAC did not report, such as one found only in code, has no id. Discovery runs once to find it, and a test still without one is marked errored instead of widening the run. Results and started notifications are matched to the explorer by that id, so tests that share a name within a project, such as theory rows run on Microsoft.Testing.Platform, no longer collide. The name-based id is only a fallback for items and results that carry no id. PlatformUid is gone from TestItemDTO, and TestUids is now TestIds.
An explorer item is still named by its project and full name, and FSAC does not keep full names unique. A grouping can share its name with a test, and on Microsoft.Testing.Platform, where the full name is the display name, several tests can share one. Siblings that share a name got one explorer id, so the item added last replaced the others: a grouping hid the test of its name, and only one of several same-named tests stayed in the tree. The previous commit matched results by FSAC's id, but the tree still collapsed these tests, so they did not stay apart as it said. A grouping is now shown as one node with the test of its name, and that test runs along with the tests under it. Each further test of a name gets a numbered name. Selections and missing results are told apart by FSAC's id where there is one. Before, tests of one name under different parents were deduplicated to one, so the others were never sent to FSAC.
|
@farlee2121 Gotcha, that makes sense - I've reworked it so every test FSAC hands out gets an opaque id, and id runs and filter runs are separate paths that behave the same on VSTest and MTP. The VSTest tripleI tried the
xUnit and MSTest set their own What I've got now
Side effect: two existing bugs fell out - duplicate display names were being merged (dropping tests), and a leaf with the same name as a group was dropping the group. Got this locally across both repos; I'll push once I've smoke tested the explorer in VS Code. |
Editing a test file merged the new code locations into the explorer by adding every test whose line had moved. Adding replaces the item of the same id, and a test found in code carries no FSAC id and none of the children FSAC reported under it, such as theory rows. An edited test so lost its id and could no longer be run: it was "not found by test discovery". A test from code that is already in the tree now has its range updated in place, and its children are merged the same way. Tests new to the tree are still added and tests gone from the code are still removed.
Refresh builds only the projects it takes for test projects, and it knew them by the VSTest packages alone. A project that opts into Microsoft.Testing.Platform, such as one on xunit.v3 or MSTest.Sdk, need not reference those packages, so it was never built and FSAC found no tests in it. FSAC does not pass on IsTestingPlatformApplication, so such a project is now told by the Microsoft.Testing.Platform package its runner resolves, or by the IsTestProject property FSAC does report.
|
@farlee2121 Pushed - smoke tested the explorer in VS Code against a mixed VSTest + MTP workspace and it all behaves. Plain-terms rundown of where it's at: The id rework (what you asked for)
Fixes since the last pushDid an adversarial review pass over both PRs and fixed the bigger things it turned up:
Still open / heads up
Would appreciate another look when you get a chance 🙏 |
|
Looking good. Just one rename suggestion and a few spots I'll want to run the regression tests on. |
Let me know; I did run against your regression library earlier in the dev which brought edge cases up. Maybe we should include them as a suite in FSAC? |
Manual Testing ResultsI found a few unexpected behaviors while testing
Heads up that MTPv2.MSTest.Tests has some odd behavior. Visual Studio won't run it if MTP filtersRegarding filters for MTP. I think it's fine if we delay that feature. I found this suggestion that MSTest or NUnit might have a filter converter VSTestBridge has some very basic support for filter translation. But really only for Uid-based test filtering The NUnit filter conversion is to their own filter system. |
2 participants
Problem
Ionide's test explorer infers grouping nodes by splitting test names and selects tests with VSTest filter expressions. FSAC's new test tree and Microsoft.Testing.Platform (MTP) path need the extension to consume explicit parent links and select MTP tests by UID.
Change
FSharp.enableTestingPlatformsetting and pass it to FSAC.Depends on ionide/FsAutoComplete#1552. Tracks #2069.
Validation
This remains a draft pending CI and review; GitHub currently reports no checks for the fork PR. UID-based MTP selection is implemented. The sample xUnit adapter rejects MTP graph-filter requests, so arbitrary graph expressions are outside this PR.