VSTestBridge+MSTest: Use TestMethodIdentifierProperty and stop sending VSTest-specifics

[Youssef1313]

Fixes #5406

@drognanar Any concerns?

• MSTest won't use the capabilities implementation of the bridge so that it removes vstestProvider (we still keep it for NUnit and Expecto)
• When a framework of the VSTestBridge uses vstestProvider, everything is the same as today.
• When a framework of the VSTestBridge doesn't use vstestProvider (i.e, MSTest for now), we expect ManagedType/ManagedMethod to be present. In that case, we parse them via AdapterUtilities to construct TestMethodIdentifierProperty.
• We stop serializing location.namespace completely. And include both the namespace and type name of TestMethodIdentifierProperty in location.type.

[Youssef1313]

Initially I deleted some tests here when I was removing vstestProvider completely. In the current PR status, some logic is brought back, and I need to plug the tests back per the updated logic (we add the vstest properties under vstestProvider named capability + JsonRpc server mode).

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 92.55319% with 7 lines in your changes missing coverage. Please review.

Project coverage is 73.82%. Comparing base (caeb624) to head (049a619).
Report is 73 commits behind head on main.

Files with missing lines	Patch %	Lines
...ons.VSTestBridge/VSTestBridgedTestFrameworkBase.cs	0.00%	5 Missing ⚠️
...STestBridge/ObjectModel/FrameworkHandlerAdapter.cs	87.50%	1 Missing ⚠️
...Bridge/ObjectModel/TestCaseDiscoverySinkAdapter.cs	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5409      +/-   ##
==========================================
- Coverage   76.24%   73.82%   -2.43%     
==========================================
  Files         603      602       -1     
  Lines       36420    36467      +47     
==========================================
- Hits        27770    26923     -847     
- Misses       8650     9544     +894     

Flag	Coverage Δ
Debug	73.82% <92.55%> (-2.43%)	⬇️
integration	73.82% <92.55%> (-2.43%)	⬇️
production	73.82% <92.55%> (-2.43%)	⬇️
unit	73.82% <92.55%> (-2.43%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...latformAdapter/TestApplicationBuilderExtensions.cs	100.00% <100.00%> (ø)
...abilities/VSTestBridgeExtensionBaseCapabilities.cs	100.00% <ø> (ø)
....VSTestBridge/ObjectModel/ObjectModelConverters.cs	94.28% <100.00%> (+1.47%)	⬆️
...uests/VSTestDiscoverTestExecutionRequestFactory.cs	77.77% <100.00%> (-2.23%)	⬇️
...e/Requests/VSTestRunTestExecutionRequestFactory.cs	76.47% <100.00%> (-3.53%)	⬇️
...ft.Testing.Platform/Messages/TestNodeProperties.cs	92.38% <ø> (ø)
...t.Testing.Platform/ServerMode/JsonRpc/Json/Json.cs	89.55% <100.00%> (+0.09%)	⬆️
...Platform/ServerMode/JsonRpc/SerializerUtilities.cs	91.87% <100.00%> (+0.07%)	⬆️
...STestBridge/ObjectModel/FrameworkHandlerAdapter.cs	58.33% <87.50%> (ø)
...Bridge/ObjectModel/TestCaseDiscoverySinkAdapter.cs	79.48% <83.33%> (-0.52%)	⬇️
... and 1 more

... and 65 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fhnaseer]

Suggestion:

if (TryGetMethodIdentifierProperty(testCase, out TestMethodIdentifierProperty? methodIdentifierProperty))
    testNode.Properties.Add(methodIdentifierProperty);
else if (ShouldAddVSTestProviderProperties(serviceProvider)) // check here about vstest properties rather than inside CopyVSTestProperties function
   CopyVSTestProperties(testCase.Properties, testNode, testCase, testCase.GetPropertyValue, isTrxEnabled)
else
  throw exception?,

Maybe a check as well. If test framework sends both managed and vstest properties, then we can stop or warn them,

[Youssef1313]

MSTestDiscoverer.TestCategory needs to be copied regardless. It could be separated out of CopyVSTestProperties though, but we do it while we are looping already

[fhnaseer]

A suggestion then. Maybe we can rename CopyVSTestProperties to something generic CopyProperties and then pass different flags ShouldAddVSTestProviderProperties, ShouldCopySomething.

[fhnaseer]

Not like the idea of having bool addVSTestProviderProperties here. By just looking at the name CopyVSTestProperties, this function should not do anything if addVSTestProviderProperties = false. So, why someone should call it with false?

I would prefer to have

if (addVstestProperties)
   CopyVSTestProperties(...);

rather than

CopyVSTestProperties(..., false);

[Youssef1313]

Same here.

MSTestDiscoverer.TestCategory needs to be copied regardless.