Added support for debugging external test processes#2325
Conversation
cvpoienaru
requested review from
AbhitejJohn,
jakubch1 and
shyamnamboodiripad
and removed request for
jakubch1
…//github.com/cvpoienaru/vstest into copoiena/debugging-external-test-processes-2
…//github.com/cvpoienaru/vstest into copoiena/debugging-external-test-processes-2
…//github.com/cvpoienaru/vstest into copoiena/debugging-external-test-processes-2
…//github.com/cvpoienaru/vstest into copoiena/debugging-external-test-processes-2
…//github.com/cvpoienaru/vstest into copoiena/debugging-external-test-processes-2
cvpoienaru
changed the title
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
I would look into using default implementation for interfaces, which was introduced in c# 8, that should avoid adding a new 2 interfaces, and the change propagating through the other interfaces that consume the original interface. This feature was made for this exact purpose (from what I understood), to allow extending published interfaces with new functionality.
…sthost process is launched.
| /// Creates a new instance of this class. | ||
| /// </summary> | ||
| /// <param name="pid">The process id the debugger should attach to.</param> | ||
| public TestProcessAttachDebuggerPayload(int pid) |
There was a problem hiding this comment.
int pid [](start = 48, length = 7)
Well the other comment got me asking this - What is our plan for attaching to a process on another compute resource(docker container/remote VM)? Would we need to add anything else in this structure to avoid creating a TestProcessAttachDebuggerPayload2?
There was a problem hiding this comment.
I believe so far the remote debugger does supports attaching to process by PID in the docker case and that would not require any API changes here. There is a bug with the old launch and attach workflow where the debugger APIs were not returning the PID - @drognanar is working with the docker / debugger folks on a fix for that.
There was a problem hiding this comment.
I see, would a property bag still be useful in this case? In case Attach needs additional information.
There was a problem hiding this comment.
Do you mean an open-ended dictionary of string-object or something else? I'd hate to add something to the API without understanding the scenario. Its probably ok to rev the API again if we need to - but I don't think we should create an API without fully understanding how it will be used.
There was a problem hiding this comment.
So one of the things that came up for the LaunchProcessWithDebuggerAttached was to be able to pass back DebuggerGuids. With Attach though, I'm thinking off something like remote machine details for instance. A string-string would just do given the content can be serialized.
There was a problem hiding this comment.
Right now I'm definitely taking a dependency on us using vsdbg to start a debugger and passing a JSON specifically for .NET. My original thought was to link the debugger choice back to the containers. If we would want to have a C# project launch a debugger for a node.exe, then we would need to have additional parameters in this method.
Also, I've been passing back RemoteEnvironment from the request back to VS, so we don't need to specify that again in the testhost (unless the testhost would start a test process on another machine than where the testhost lives).
There was a problem hiding this comment.
In short currently we're using the information from the test container, the runtime provider and the pid in order to create an appropriate attach request.
There was a problem hiding this comment.
Yeah, non-managed workflows was what I was thinking off as well. I'm not sure what other parameters would be required there.
There was a problem hiding this comment.
Adding in a string-string property bag and terming it as options wouldn't hurt much I'd think.
There was a problem hiding this comment.
I think if we have to do that we'd also need to add it to the LaunchAndAttach API for completeness - including to the IFrameworkHandle APIs that are exposed to adapter.
I'm a bit loath to do all that as part of the current change as its not required for this change + we haven't really proved that we need it (as far as I can tell we don't have a real need for it for Docker so far). We may discover that we need something else / different API shape when we actually get to implementing remote debugging support where additional data is required (for example SSH / remote VM).
So I feel it may be best to wait and introduce those APIs as and when the need arises.
I got really excited when this was first proposed. It would've saved us a lot of headaches down the road, but unfortunately won't work on today's implementation and that's kind of a bummer :( Quoting @shyamnamboodiripad reply: (#2325 (comment)) "Unfortunately, default interface implementation depends on CLR features that are not available when targeting net472 (requires netcoreapp3.0). Since VS and many test adapters still run atop net472 I don't believe we can use default interface implementation yet." |
| this.testPlatformEventSource.AdapterExecutionStop(this.testRunCache.TotalExecutedTests - currentTotalTests); | ||
| attachedToTestHost = true; | ||
| var pid = Process.GetCurrentProcess().Id; | ||
| if (!this.frameworkHandle.AttachDebuggerToProcess(pid)) |
There was a problem hiding this comment.
AttachDebuggerToProcess [](start = 46, length = 23)
Do we already have telemetry that logs the protocol versions across vstest.console, testhost and in this case how many adapters implement ITestExecutor2? It would be useful for us to understand usage stats.
There was a problem hiding this comment.
I'll open another follow-up task to add telemetry.
|
How are we tracking the documentation update and filling in the test matrix we discussed? |
|
I'll also open another issue to keep track of the documentation task. |
cvpoienaru
changed the title
6 participants
Description
Added support for debugging external test processes.
Related issue
https://github.com/microsoft/vstest-docs/blob/ec090fe5dc2b47948c7b771ea0ecf25d978f164b/RFCs/0029-Debugging-External-Test-Processes.md