Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Fix buildFlags type in 'debug test' Codelens #1248

Merged
merged 2 commits into from
Oct 1, 2017

Conversation

ka2n
Copy link
Contributor

@ka2n ka2n commented Sep 27, 2017

Make the type of buildArgs executed by Codelens consistent with LaunchRequestArguments.
This fix the problem that go.buildTags is ignored by debug test.

@@ -85,7 +85,7 @@ export class GoRunTestCodeLensProvider extends GoBaseCodeLensProvider {
buildFlags.push(`"${vsConfig['buildTags']}"`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is related to buildtags specifically and not buildflags in general, shouldnt we do the join that you did on line 85 instead?

Copy link
Contributor Author

@ka2n ka2n Sep 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Referring to usage and interface declaration, buildFlags seems to assume a string?, or is this reference place wrong(or not just Delve class)? I'll check again, thanks.

@ka2n ka2n force-pushed the codelens-debugtest-buildargs branch from 280e790 to efc1d3d Compare September 28, 2017 05:25
@ramya-rao-a
Copy link
Contributor

Can you give me a sample code where I can test this?
Sorry I don't do a lot of Go coding myself and haven't used build tags much.

@ka2n
Copy link
Contributor Author

ka2n commented Sep 28, 2017

Problem:
I use build tags for excluding some tests when running on CI server.

some_test.go

// +build noci

package some

import "testing"

func TestFoo(t *testing.T) {
 // A test always fail on CI sever
}

Only when I add -tags noci to go test parameters, the test will run.

$ go test .
?       github.com/ka2n/example/some [no test files]
$ go test -tags noci
ok     github.com/ka2n/example/some 0.007s

In VSCode, I want to add -tags noci every test run.

<project_dir>/.vscode/settings.json

{
  "go.buildTags": "noci other_tag"
}
  1. Run run test in codelens of TestFoo, it outputs,
Running tool: /usr/local/Cellar/go/1.9/libexec/bin/go test -timeout 30s -tags noci other_tag -run ^TestFoo$

PASS
ok  	github.com/ka2n/example/some	0.006s
Success: Tests passed.
  1. Run debug test in codelens of TestFoo, error GUI are shown,
    it says Failed to continue: Check the debug console for details.

To check final argument to launch delv, log JSON.stringify(dlvArgs) in this place.

It output:
["test","--headless=true","--listen=127.0.0.1:4887","--build-flags=-tags,\"noci other_tag\"","--","-test.run","TestFoo"]

-tags are malformed from ["-tags", "\"noci other_tag\""](object) to -tags,\"noci other_tag\"(string).

To fix this problem I added this commit.


Second problem is something different situation, but I noted here for someone's reference.

I want to add -ldflags, and specify tags,
<project_dir>/.vscode/settings.json

    "go.buildTags:" "noci other_tag",
    "go.buildFlags": [
        "-ldflags",
        "-X mypackage.myvalue=./"
    ]

run debug test in codelens ofTestFoo
got error output in DEBUG CONSOLE

could not launch process: stat /Users/ka2n/src/github.com/msetsu/cointown/some/debug.test: no such file or directory
Process exiting with code: 1\

In this situation, JSON.stringify(dlvArgs) is ["test","--headless=true","--listen=127.0.0.1:24136","--build-flags=-ldflags,-X mypackage.myvalue=./,-tags,\"integration noci\"","--","-test.run","TestFoo"]

I've try to solve this problem, got partial success, but it make go.buildFlags incompatible with normal run test.

@ramya-rao-a
Copy link
Contributor

I spent an hour trying to reproduce your first problem, but couldnt repro. Turns out this is not an issue in Windows. Then I tried on my Mac and was able to repro the issue instantly.

So, please confirm, the problem seems to be that delve requires buildFlags to be a single string which is space separated and go test requires it to be an array (because we use cp.execFile)

In that case, your initial solution of converting the entire buildFlags to string should help in both issues correct?

@ka2n
Copy link
Contributor Author

ka2n commented Sep 29, 2017

@ramya-rao-a
Thank you for taking a time for this PR and I'm sorry I have not include platform information.

At first, my initial solution was not correct to solve entire problem.
I just write a patch to make goDebug.ts: launchArgs.buildFlags type and actual value match.

So, my latest commit is focus on just go.buildTags, I have to change the PR title :|.
Isgo.buildTags solution will work for Windows? (I cannot test it because I don't have Windows machine)


For both issues correct
In the situation setting are below,

{
  "go.buildFlags": [
    "-tags",
    "noci other_tag",
    "-ldFlags",
    "-X mypackage.myvalue=./"
  ]
}
  • go test argument should: ["test", "-tags", "noci other", "-ldFlags", "-X mypackage.myvalue=./" ]
  • dlv debug argument should: ["--buildFlags", "-tags 'noci other_tag' -ldFlags '-X mypackage.myvalue=./'"] (argument values are quoted. It'll split again by Delve))

But I can't find a way to filter argument value and quote it for Delv arguments.
And, now I think this problem is out of scope of this PR.

@ramya-rao-a
Copy link
Contributor

@ka2n

Hey, my mistake, I am able to see the issue in Windows as well.
I had an old debug.test binary lying around and the debugger was using that so couldn't find the error before.
I cleaned my repo and now I am able to repro in Windows as well.

I made a commit to your branch for quoting both the flags and the flag values and that seem to work well for all cases
So your settings would be

"go.buildFlags": [
    "-ldflags",
    "-X mypackage.myvalue=./"
],
"go.buildTags: "noci"

Both "run tests" and "debug tests" codelens should now work.

Can you give it a try?

@ramya-rao-a ramya-rao-a merged commit 7b9abd6 into microsoft:master Oct 1, 2017
@ka2n
Copy link
Contributor Author

ka2n commented Oct 2, 2017

@ramya-rao-a

I've tested with macOS(10.13 (17A365)), VSCode(Version 1.17.0-insider (1.17.0-insider)), vscode-go(47c8b54) and works fine!

Thank you!

@ka2n ka2n deleted the codelens-debugtest-buildargs branch October 2, 2017 05:41
@ramya-rao-a
Copy link
Contributor

Awesome, thanks for working on this @ka2n
I'll release an update later today

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants