feat: emit public TestMethodIdentifierProperty and stop advertising vstestProvider#284
Conversation
|
This relates to #282 I imagine? The failures in CI are the same at least. |
|
Would you update this to latest main and resolve the conflicts? I would like to see CI pass before taking a deeper look at what's actually changed/how the changes work. |
…testProvider Emit the public TestMethodIdentifierProperty (location.type/method) from AddAdditionalProperties and replace VSTestBridgeExtensionBaseCapabilities with an ITrxReportCapability-only capability so the vstestProvider capability is no longer advertised. VS Test Explorer then consumes the public location.* properties instead of the legacy vstest.TestCase.* key/value-pair properties (Microsoft.Testing.Platform's internal SerializableKeyValuePairStringProperty). Mirrors MSTest's MSTestCapabilities. Requires bumping Microsoft.Testing.* from 1.9.1 to 2.2.3 because the AddAdditionalProperties hook is newer than 1.9.1. Refs YoloDev#283
Evangelink
force-pushed
the
mtp-emit-public-properties-drop-vstestprovider
branch
from
23fae6c to
bbd99df
Compare
…eException The skipped-test printer added a TestResultMessage with AdditionalInfoCategory. The Microsoft.Testing.Platform VSTest bridge (2.x) only converts StandardError/ StandardOut/DebugTrace message categories when building the TRX report and throws UnreachableException on any other category. Once the adapter stops advertising the vstestProvider capability (this PR), result conversion goes through that TRX path, so a skipped test crashed the whole MTP run under 'dotnet test'. Set TestResult.ErrorMessage instead: the bridge maps it to SkippedTestNodeStateProperty and the legacy VSTest runner shows it as the skip reason, so the reason is preserved on both paths without the unsupported message category. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@Alxandr PR is ready. |
Addresses review feedback: the MSTestCapabilities reference pointed at a moving target (the testfx main branch). Pin it to commit 5c6ea3bf01f1247736fbbbba0ffdd8a8b38840dc so the cited code can't drift. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
This should be considered a breaking change I imagein? |
It should not have any perceiptible impact. Before we were using the test case info we were converting at bridge level, now we are declaring directly but using the same mechanism. Actually, I can only think of it being a break for VS 2022 |
|
You have to create/update a global.json, no? |
Right. Indeed, for NET 10+ it's the case. I am kinda thinking this is .NET breaking change but it's all good if you make it breaking change on your side. |
Alxandr
changed the title
|
Is this ready to be released immediately, or should I wait for something? |
|
It's ready! |
|
Actually, I will probably release 2.3 today so you may want to wait for that. |
|
Ok. Just poke me when you want this released. |
|
Getting some localization issues. I'll wait an extra day and release tomorrow evening. |
|
Thanks for the ping, I was waiting to complete MSTest to ping you but MTP 2.3.0 is shipped so you can bump and release. |
2 participants
What
Emit the public
TestMethodIdentifierPropertyfrom the Expecto adapter and stop advertising the Microsoft.Testing.PlatformvstestProvidercapability, so Visual Studio Test Explorer consumes the public structured node properties instead of the legacyvstest.TestCase.*key/value-pair properties.Why
When the adapter advertises
vstestProvider, VS routes Expecto through a legacy parser that readsvstest.TestCase.*properties, serialized via Microsoft.Testing.Platform''s internalSerializableKeyValuePairStringPropertykey/value bag. The testfx team is deprecating that internal type in favor of public property types. MSTest already moved off this path; NUnit and Expecto are the last two adapters using it (companion NUnit PR: nunit/nunit3-vs-adapter#1456).How
adapter.fs— overrideAddAdditionalPropertiesto emit aTestMethodIdentifierProperty(serialized aslocation.type/location.method). Expecto tests are values in nested lists, not CLR methods, so this is synthesized from the dot-joinedFullyQualifiedName.TestApplicationHelpers.fs— replaceVSTestBridgeExtensionBaseCapabilities(which also implementsINamedFeatureCapability("vstestProvider")) with a small capability implementing only the publicITrxReportCapability. Mirrors MSTest''sMSTestCapabilities.Directory.Packages.props— bumpMicrosoft.Testing.*from1.9.1to2.2.3. TheAddAdditionalPropertiesvirtual hook on the bridge base type is newer than1.9.1, so the override does not compile against1.9.1.Builds clean (
net8.0).Open questions / please review before this leaves draft
adapter.fs): theFullyQualifiedNameis split on the last.intolocation.type/location.method. This assumes the default--join-with .; with--join-with /the split is wrong. There may be a better mapping from Expecto''s test hierarchy — your call.1.x.Identity & sequencing (important)
This change is the right direction but should be sequenced with the VS Test Explorer side, not landed in isolation:
vstest.TestCase.IdGUID to the publicTestNode.Uid. Expecto''sFullyQualifiedNameis already a unique per-test path, so the synthesizedlocation.type/location.methodpreserves per-test identity reasonably well (better than the NUnit method-parameterized case) — but it remains a heuristic split.vstest.original-executor-uriis dropped (the bridge only emits it undervstestProvider). It is used for framework identity (e.g. "Associate to Test Case"). This is at parity with MSTest, which already runs without it on the public path; if a framework-identity feature depends on it, that is a shared testfx gap (needs a public identity property), not Expecto-specific.ITrxReportCapability(the internal on-demand gate isn''t available to external adapters). Harmless on the server-mode wire.Please coordinate with the VS Test Explorer team so navigation / grouping / Test Case association don''t regress before
vstestProvideris fully dropped.Refs #283