Add target framework information to the AttachDebugger callback #3701
Conversation
|
Just a draft right now, won't even build because of public apis. I could probably make the custom launcher use internal interface that has single verison of api as well, and then adapt it for use in FrameworkHandle and logger, but it was just references in too many places. |
src/Microsoft.TestPlatform.CommunicationUtilities/Messages/MessageType.cs
Show resolved
Hide resolved
src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyDiscoveryManager.cs
Show resolved
Hide resolved
nohwnd
force-pushed
the
add-info-to-debugger
branch
from
a88d78b
to
6b87355
Compare
… how to do this properly via settings
… into add-info-to-debugger
| Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.Interfaces.AttachDebuggerInfo.TargetFramework.set -> void | ||
| Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.Interfaces.AttachDebuggerInfo.Version.get -> System.Version |
There was a problem hiding this comment.
all those apis were marked as obsolete before, we can safely change them. And will probably mark them as obsolete again, while we pilot this with VS.
| @@ -21,7 +21,7 @@ namespace TestPlatform.Playground; | |||
|
|
|||
| internal class Program | |||
| { | |||
| static void Main(string[] args) | |||
There was a problem hiding this comment.
Is there any particular reason that prompted this change ?
There was a problem hiding this comment.
Yes, because args are not used.
There was a problem hiding this comment.
I will just revert all code in Playground before merging as always (unless I forget, as usual :D).
playground/MSTest1/UnitTest1.cs
Outdated
|
|
||
| [TestClass] | ||
| public class UnitTest1 | ||
| [ExtensionUri("uri://myadapter")] |
There was a problem hiding this comment.
Can we give this adapter a more suggestive name ? Asking because it will probably end up in the telemetry data and it would be nice to know it's ours.
There was a problem hiding this comment.
I will just revert all code in Playground before merging as always (unless I forget, as usual :D).
There was a problem hiding this comment.
This won't end up in approved telemetry, and in unapproved telemetry anyone can post anything so it does not really matter.
playground/MSTest1/UnitTest1.cs
Outdated
| @@ -1,18 +1,47 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | |||
|
|
|||
| using Microsoft.VisualStudio.TestTools.UnitTesting; | |||
There was a problem hiding this comment.
Don't know where to write this comment so I'll write it here. Given that this file no longer contains unit tests, should we change the name from UnitTest1.cs to something else ?
There was a problem hiding this comment.
I will just revert all code in Playground before merging as always (unless I forget, as usual :D).
|
|
||
| @"C:\p\vstest\playground\MSTest1\bin\Debug\net472\MSTest1.TestAdapter.dll", | ||
|
|
||
| // @"C:\t\TestProject13_\TestProject1\bin\Debug\net48\TestProject1.dll", |
There was a problem hiding this comment.
Can we remove those commented-out sections ?
There was a problem hiding this comment.
I will just revert all code in Playground before merging as always (unless I forget, as usual :D).
| <PropertyGroup> | ||
| <TestPlatformRoot Condition="$(TestPlatformRoot) == ''">..\..\</TestPlatformRoot> | ||
| <TargetLatestRuntimePatch>true</TargetLatestRuntimePatch> | ||
| <!-- MSB3270 Suppress warnings about testhost being x64 (AMD64)/x86 when imported into AnyCPU (MSIL) projects. --> |
There was a problem hiding this comment.
Is there anything we can do about those warnings so that their suppression is no longer needed ?
There was a problem hiding this comment.
I will just revert all code in Playground before merging as always (unless I forget, as usual :D). Also it it a file auto generated by VS. Not sure what started happening, but I see - Backup generated into my project from the dogfooding VS quite often lately.
There was a problem hiding this comment.
@nohwnd What about adding a git exclusion for backup projects?
| @@ -27,4 +25,22 @@ public TestProcessAttachDebuggerPayload(int pid) | |||
| /// </summary> | |||
| [DataMember] | |||
| public int ProcessID { get; set; } | |||
|
|
|||
| [DataMember] | |||
| // Added in version 7. | |||
There was a problem hiding this comment.
Would the use of ProtocolVersion attribute make sense here ?
There was a problem hiding this comment.
Yes, I commented the same thing above. I just did not want to fiddle with usings, and adding too many overloads for that attribute, I will do that later.
src/Microsoft.TestPlatform.ObjectModel/TestProcessAttachDebuggerPayload.cs
Show resolved
Hide resolved
src/Microsoft.TestPlatform.ObjectModel/TestProcessAttachDebuggerPayload.cs
Show resolved
Hide resolved
src/Microsoft.TestPlatform.PlatformAbstractions/VersionAttribute.cs
Outdated
Show resolved
Hide resolved
| @@ -1436,16 +1438,46 @@ private void AttachDebuggerToProcess(ITestHostLauncher customHostLauncher, Messa | |||
|
|
|||
| try | |||
| { | |||
| var pid = _dataSerializer.DeserializePayload<int>(message); | |||
| // Handle EditorAttachDebugger2. | |||
| if (message.MessageType == MessageType.EditorAttachDebugger2) | |||
There was a problem hiding this comment.
Can we employ a Builder pattern here ? Seeing the code is mostly identical for the two message types but the only relevant difference is the way we build the AttachDebuggerInfo object, it looks like we can avoid duplicating code.
There was a problem hiding this comment.
Let's merge it as is, and you can definitely PR a more elegant code, because I don't see one.
src/Microsoft.TestPlatform.Client/DesignMode/DesignModeTestHostLauncher.cs
Show resolved
Hide resolved
test/Microsoft.TestPlatform.AcceptanceTests/TranslationLayerTests/CustomTestHostTests.cs
Show resolved
Hide resolved
Co-authored-by: Amaury Levé <amaury.leve@gmail.com>
Description
Add target framework information to the attach debugger callback so IDE (like VS) don't have to guess what engine to use for a given dll. This will either keep the information that is sent by testhost, or if the target framework is kept at the default, it will use the target framework of the given testhost.
Lot of the changes are because the interfaces that are used internally are also public, and so I can't really force ITestRuntimeProvider to tell me what framework it is running as.
Some steps towards additional compatibility work are done here. This PR introduces a new message, that is expandable EditorAttachDebugger2, that replaces EditorAttachDebugger that just carries an int. On the edge of the process we decide what message should be sent, to keep the message compatible.
Then in TranslationLayer, we are adapting the new message for use with either of the launcher implementations.
Adding new versions of interfaces is not great, but doing it properly is difficult when we have multiple non-expandable messages, so I want to give myself some more time to come up with a good design.