From a70201aa5dd4c1466e857ec669e253e2c897c9aa Mon Sep 17 00:00:00 2001 From: Polina Sokolova Date: Thu, 14 May 2020 00:00:09 -0700 Subject: [PATCH 1/4] service/dap: Add error checking and tests for buildFlags in launch requests --- _fixtures/buildflagtest.go | 14 ++++++++++++++ pkg/proc/test/support.go | 9 +++++++++ service/dap/server.go | 13 ++++++++++--- service/dap/server_test.go | 17 +++++++++++++++++ 4 files changed, 50 insertions(+), 3 deletions(-) create mode 100644 _fixtures/buildflagtest.go diff --git a/_fixtures/buildflagtest.go b/_fixtures/buildflagtest.go new file mode 100644 index 0000000000..92ae8c5e1d --- /dev/null +++ b/_fixtures/buildflagtest.go @@ -0,0 +1,14 @@ +package main + +import "fmt" + +// To be set via +// go build -ldflags '-X main.Hello=World' +var Hello string + +func main() { + if len(Hello) < 1 { + panic("global main.Hello not set via build flags") + } + fmt.Printf("Hello %s!\n", Hello) +} diff --git a/pkg/proc/test/support.go b/pkg/proc/test/support.go index 394e204a2d..cc9a22ab78 100644 --- a/pkg/proc/test/support.go +++ b/pkg/proc/test/support.go @@ -81,6 +81,12 @@ const ( // BuildFixture will compile the fixture 'name' using the provided build flags. func BuildFixture(name string, flags BuildFlags) Fixture { + return BuildFixtureWithFlags(name, flags, "") +} + +// BuildFixtureWithFlags will compile the fixture 'name' using the provided predefined +// and custom build flags. +func BuildFixtureWithFlags(name string, flags BuildFlags, andflags string) Fixture { if !runningWithFixtures { panic("RunTestsWithFixtures not called") } @@ -144,6 +150,9 @@ func BuildFixture(name string, flags BuildFlags) Fixture { buildFlags = append(buildFlags, "-ldflags=-compressdwarf=false") } } + if andflags != "" { + buildFlags = append(buildFlags, andflags) + } if path != "" { buildFlags = append(buildFlags, name+".go") } diff --git a/service/dap/server.go b/service/dap/server.go index eb1408189f..c2fd0f2028 100644 --- a/service/dap/server.go +++ b/service/dap/server.go @@ -402,9 +402,16 @@ func (s *Server) onLaunchRequest(request *dap.LaunchRequest) { return } - buildFlags, ok := request.Arguments["buildFlags"].(string) - if !ok { - buildFlags = "" + buildFlags := "" + buildFlagsArg, ok := request.Arguments["buildFlags"] + if ok { + buildFlags, ok = buildFlagsArg.(string) + if !ok { + s.sendErrorResponse(request.Request, + FailedToContinue, "Failed to launch", + fmt.Sprintf("'buildFlags' attribute '%v' in debug configuration is not a string.", buildFlagsArg)) + return + } } switch mode { diff --git a/service/dap/server_test.go b/service/dap/server_test.go index d020f16ea2..e1473ae070 100644 --- a/service/dap/server_test.go +++ b/service/dap/server_test.go @@ -376,6 +376,16 @@ func TestLaunchRequestWithArgs(t *testing.T) { }) } +func TestLaunchRequestWithBuildFlags(t *testing.T) { + runTest(t, "buildflagtest", func(client *daptest.Client, fixture protest.Fixture) { + runDebugSession(t, client, func() { + client.LaunchRequestWithArgs(map[string]interface{}{ + "mode": "debug", "program": fixture.Source, + "buildFlags": "-ldflags '-X main.Hello=World'"}) + }) + }) +} + func TestUnupportedCommandResponses(t *testing.T) { var got *dap.ErrorResponse runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) { @@ -586,6 +596,10 @@ func TestBadLaunchRequests(t *testing.T) { expectFailedToLaunchWithMessage(client.ExpectErrorResponse(t), "Failed to launch: value '1' in 'args' attribute in debug configuration is not a string.") + client.LaunchRequestWithArgs(map[string]interface{}{"mode": "debug", "program": fixture.Source, "buildFlags": 123}) + expectFailedToLaunchWithMessage(client.ExpectErrorResponse(t), + "Failed to launch: 'buildFlags' attribute '123' in debug configuration is not a string.") + // Skip detailed message checks for potentially different OS-specific errors. client.LaunchRequest("exec", fixture.Path+"_does_not_exist", stopOnEntry) expectFailedToLaunch(client.ExpectErrorResponse(t)) @@ -596,6 +610,9 @@ func TestBadLaunchRequests(t *testing.T) { client.LaunchRequest("exec", fixture.Source, stopOnEntry) expectFailedToLaunch(client.ExpectErrorResponse(t)) // Not an executable + client.LaunchRequestWithArgs(map[string]interface{}{"mode": "debug", "program": fixture.Source, "buildFlags": "123"}) + expectFailedToLaunch(client.ExpectErrorResponse(t)) // Build error + // We failed to launch the program. Make sure shutdown still works. client.DisconnectRequest() dresp := client.ExpectDisconnectResponse(t) From d947da52fc77bdaa1cbc9da49caed3f8a4461772 Mon Sep 17 00:00:00 2001 From: Polina Sokolova Date: Fri, 15 May 2020 11:21:31 -0700 Subject: [PATCH 2/4] Clarify with comments and better naming --- pkg/proc/test/support.go | 11 ++++++----- service/dap/server_test.go | 8 ++++++++ 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/pkg/proc/test/support.go b/pkg/proc/test/support.go index cc9a22ab78..6517fa8c54 100644 --- a/pkg/proc/test/support.go +++ b/pkg/proc/test/support.go @@ -84,9 +84,10 @@ func BuildFixture(name string, flags BuildFlags) Fixture { return BuildFixtureWithFlags(name, flags, "") } -// BuildFixtureWithFlags will compile the fixture 'name' using the provided predefined -// and custom build flags. -func BuildFixtureWithFlags(name string, flags BuildFlags, andflags string) Fixture { +// BuildFixtureWithFlags will compile the fixture 'name' using the provided +// a la carte build flags as well as additional freeform build flags specified as +// a string to be passed as-is to the 'go' command. +func BuildFixtureWithFlags(name string, flags BuildFlags, moreflags string) Fixture { if !runningWithFixtures { panic("RunTestsWithFixtures not called") } @@ -150,8 +151,8 @@ func BuildFixtureWithFlags(name string, flags BuildFlags, andflags string) Fixtu buildFlags = append(buildFlags, "-ldflags=-compressdwarf=false") } } - if andflags != "" { - buildFlags = append(buildFlags, andflags) + if moreflags != "" { + buildFlags = append(buildFlags, moreflags) } if path != "" { buildFlags = append(buildFlags, name+".go") diff --git a/service/dap/server_test.go b/service/dap/server_test.go index e1473ae070..56af07ea07 100644 --- a/service/dap/server_test.go +++ b/service/dap/server_test.go @@ -366,6 +366,10 @@ func TestLaunchTestRequest(t *testing.T) { }) } +// Tests that 'args' from LaunchRequest are parsed and passed to the target +// program. The target program exits without an error on success, and +// panics on error, causing an unexpected StoppedEvent instead of +// Terminated Event. func TestLaunchRequestWithArgs(t *testing.T) { runTest(t, "testargs", func(client *daptest.Client, fixture protest.Fixture) { runDebugSession(t, client, func() { @@ -376,6 +380,10 @@ func TestLaunchRequestWithArgs(t *testing.T) { }) } +// Tests that 'buildFlags' from LaunchRequest are parsed and passed to the +// compiler. The target program exits without an error on success, and +// panics on error, causing an unexpected StoppedEvent instead of +// TerminatedEvent. func TestLaunchRequestWithBuildFlags(t *testing.T) { runTest(t, "buildflagtest", func(client *daptest.Client, fixture protest.Fixture) { runDebugSession(t, client, func() { From 0cf1dc6f5c2a8c772842b2f18363eed8f0d6fcc1 Mon Sep 17 00:00:00 2001 From: Polina Sokolova Date: Wed, 27 May 2020 00:42:42 -0700 Subject: [PATCH 3/4] Undo redundant support.go changes --- service/dap/server_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/service/dap/server_test.go b/service/dap/server_test.go index 56af07ea07..ff86808b6a 100644 --- a/service/dap/server_test.go +++ b/service/dap/server_test.go @@ -345,7 +345,8 @@ func runDebugSession(t *testing.T, client *daptest.Client, launchRequest func()) func TestLaunchDebugRequest(t *testing.T) { runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) { - // We reuse the harness that builds, but ignore the actual binary. + // We reuse the harness that builds, but ignore the built binary, + // only relying on the source to be built in response to LaunchRequest. runDebugSession(t, client, func() { // Use the default output directory. client.LaunchRequestWithArgs(map[string]interface{}{ @@ -357,7 +358,8 @@ func TestLaunchDebugRequest(t *testing.T) { func TestLaunchTestRequest(t *testing.T) { runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) { runDebugSession(t, client, func() { - // We reuse the harness that builds, but ignore the actual binary. + // We reuse the harness that builds, but ignore the built binary, + // only relying on the source to be built in response to LaunchRequest. fixtures := protest.FindFixturesDir() testdir, _ := filepath.Abs(filepath.Join(fixtures, "buildtest")) client.LaunchRequestWithArgs(map[string]interface{}{ @@ -387,6 +389,8 @@ func TestLaunchRequestWithArgs(t *testing.T) { func TestLaunchRequestWithBuildFlags(t *testing.T) { runTest(t, "buildflagtest", func(client *daptest.Client, fixture protest.Fixture) { runDebugSession(t, client, func() { + // We reuse the harness that builds, but ignore the built binary, + // only relying on the source to be built in response to LaunchRequest. client.LaunchRequestWithArgs(map[string]interface{}{ "mode": "debug", "program": fixture.Source, "buildFlags": "-ldflags '-X main.Hello=World'"}) From c321aa337c542986912d9e1c279053ab4b1fcff0 Mon Sep 17 00:00:00 2001 From: Polina Sokolova Date: Wed, 27 May 2020 00:51:43 -0700 Subject: [PATCH 4/4] Undo redundant support.go changes (attempt #2) --- pkg/proc/test/support.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/pkg/proc/test/support.go b/pkg/proc/test/support.go index 6517fa8c54..394e204a2d 100644 --- a/pkg/proc/test/support.go +++ b/pkg/proc/test/support.go @@ -81,13 +81,6 @@ const ( // BuildFixture will compile the fixture 'name' using the provided build flags. func BuildFixture(name string, flags BuildFlags) Fixture { - return BuildFixtureWithFlags(name, flags, "") -} - -// BuildFixtureWithFlags will compile the fixture 'name' using the provided -// a la carte build flags as well as additional freeform build flags specified as -// a string to be passed as-is to the 'go' command. -func BuildFixtureWithFlags(name string, flags BuildFlags, moreflags string) Fixture { if !runningWithFixtures { panic("RunTestsWithFixtures not called") } @@ -151,9 +144,6 @@ func BuildFixtureWithFlags(name string, flags BuildFlags, moreflags string) Fixt buildFlags = append(buildFlags, "-ldflags=-compressdwarf=false") } } - if moreflags != "" { - buildFlags = append(buildFlags, moreflags) - } if path != "" { buildFlags = append(buildFlags, name+".go") }