service/dap: Add support for debug and test modes #4
Conversation
service/dap/server.go
Outdated
| "github.com/go-delve/delve/pkg/logflags" | ||
| "github.com/go-delve/delve/pkg/proc" | ||
| "github.com/go-delve/delve/service" | ||
| "github.com/go-delve/delve/service/api" | ||
| "github.com/go-delve/delve/service/debugger" | ||
| "github.com/google/go-dap" | ||
| "github.com/google/go-dap" // dap |
There was a problem hiding this comment.
I'm not sure what's adding this, I just recently removed it - this comment is not needed
There was a problem hiding this comment.
I believe Hana added this because the import ends with go-dap, but when you reference the items in the package, you prefix them with "dap".
There was a problem hiding this comment.
I don't think this comment is semantically necessary, though. One of my previous commits just removed it -- 23b8e59 -- so it'd be a shame to add it back.
It's perfectly fine in Go to have an import path "like-this" and referenced as "this", and many examples in Delve itself. E.g. see usage of go-colorable in https://github.com/go-delve/delve/blob/master/pkg/terminal/terminal_windows.go#L9
There was a problem hiding this comment.
Removed. I have no attachment to this comment. I thought it was some kind of convention. It was still in my file as I copied it around trying to avoid conflicts with your commits.
| func (s *Server) onLaunchRequest(request *dap.LaunchRequest) { | ||
| // TODO(polina): Respond with an error if debug session is in progress? | ||
| program, ok := request.Arguments["program"] | ||
|
|
||
| program, ok := request.Arguments["program"].(string) |
There was a problem hiding this comment.
Sorry, I may be missing something here, but has the semantics of what's stored in request.Arguments changed?
Because before this change the ok was saying whether the key "program" is found in the map. After the change it's saying that the conversion to string is successful. Is this an intentional change?
There was a problem hiding this comment.
I was missing a type check before, so if the program was not a string, then my old could would have panic'ed. This new code combines the old map check and the missing type check into one. If the item is in the map and is a string, then ok will be true. If the item is not in the map or is in the map, but is not a string, then ok will be false. https://play.golang.org/p/1oFdo4qNz4-
service/dap/server_test.go
Outdated
| @@ -172,14 +173,50 @@ func TestSetBreakpoint(t *testing.T) { | |||
| }) | |||
| } | |||
|
|
|||
| func TestBadLaunchRequests(t *testing.T) { | |||
| func initLaunchRunDisconnect(t *testing.T, client *daptest.Client, launchRequest func()) { | |||
There was a problem hiding this comment.
Renamed and added a comment.
pkg/gobuild/gobuild.go
Outdated
| } | ||
| } | ||
|
|
||
| // OptFlags generates default build flags to run off optimization and inlining. |
There was a problem hiding this comment.
"run off"? Did you mean "turn off" or something else?
service/dap/daptest/client.go
Outdated
| @@ -137,12 +137,16 @@ func (c *Client) InitializeRequest() { | |||
| } | |||
|
|
|||
| // LaunchRequest sends a 'launch' request. | |||
| func (c *Client) LaunchRequest(program string, stopOnEntry bool) { | |||
| // Takes arguments as interface{} to allow for testing of values | |||
| // of unexpected types since the launch flags are implementation-specific | |||
There was a problem hiding this comment.
This seems a bit concerning from a typing / usability point of view. Functions that take long lists of arguments are not great in the first place, but when all these arguments are untyped it's even worse. It becomes pretty hard to use the function correctly.
What is the actual goal here? Are you testing that integers instead of string paths produce expected errors? Since we don't have Go on the other side, the mapping between types and what goes into JSON isn't very clear anyway. From a cursory look at the other changes in this PR it's really hard to notice where you're actually using non-standard types.
Would it make sense to have a simple-to-use LaunchRequest for the common cases (which will take part in many tests, I imagine), and some special version to test a few corner cases of type handling in isolated cases?
There was a problem hiding this comment.
I changed the code to use 2 versions of LaunchRequest. If you think I need different argument combinations, please let me know.
In general, LaunchRequest is the only struct where part of it won't be decoded with go types, but will be an untyped map. I added code in this CL to test not only for the presence of fields, but also for their type. And the new tests check that the code handles bad/missing values without panic.
|
LGTM! Just one nit to remove the |
|
This is now go-delve#1901. |
This change also: