-
Notifications
You must be signed in to change notification settings - Fork 646
Conversation
btw, I guess build didn't failed due to this PR changes. How do we re-trigger it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the month long wait, was caught up with other work.
I have left a few comments, other than that looks great!
You will have to rebase or merge from master though.
src/goRunTestCodelens.ts
Outdated
let config = Object.assign({}, this.debugConfig, { args: ['-test.run', func.name], program: path.dirname(document.fileName) }); | ||
const args = ['-test.run', func.name]; | ||
const program = path.dirname(document.fileName); | ||
const env = vsConfig['testEnvVars'] || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the getTestEnvVars()
function from util.ts
here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the codelens point of view, It's just creating a VSCode debug configuration with env
and envFile
that is later processed/merged by GoDelve, like it does with any user-configured launch.json
.
import { GoDocumentSymbolProvider } from './goOutline'; | ||
|
||
export class GoRunTestCodeLensProvider implements CodeLensProvider { | ||
private readonly debugConfig: any = { | ||
'name': 'Launch', | ||
'type': 'go', | ||
'request': 'launch', | ||
'mode': 'test', | ||
'env': { | ||
'GOPATH': process.env['GOPATH'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
am guessing you are removing this because you expect this to be passed in the testEnvFile?
For someone who has set the GOPATH in the settings, but doesn't use the testEnvVars
or the new testEnvFile
setting, the debug test codelens will not work if this is removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed because of this line. GoDebug is always loading everything process.env
. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point
let fileEnv = {}; | ||
let testEnvFile = config['testEnvFile']; | ||
if (testEnvFile) { | ||
testEnvFile = testEnvFile.replace(/\${workspaceRoot}/g, vscode.workspace.rootPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use the resolvePath
method here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, didn't see this function. Will change it.
@@ -296,6 +297,21 @@ export function getToolsEnvVars(): any { | |||
return Object.assign({}, process.env, toolsEnvVars); | |||
} | |||
|
|||
export function getTestEnvVars(): any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
goTest.ts
would be a better place for this function as that file has all things test related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll move it :)
@@ -5,9 +5,10 @@ | |||
|
|||
import vscode = require('vscode'); | |||
import path = require('path'); | |||
import { getGoRuntimePath, getBinPathWithPreferredGopath, resolvePath } from './goPath'; | |||
import { parseEnvFile, getGoRuntimePath, getBinPathWithPreferredGopath, resolvePath, stripBOM } from './goPath'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stripBOM not needed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, I'll remove it.
The main purpose of this pull request is to enable use of .env files when running/debugging tests through VSCode no matter what triggered it (CodeLens, Tasks, Command ...)
This pull request contains:
go.testEnvFile
configuration option with same intent as envFile from vscode debug settings;process.env
,go.toolsEnvVars
,go.testEnvFile
andgo.testEnvVars
;go.testEnvFile
andgo.testEnvVars
;I didn't like this bit here
testEnvFile = testEnvFile.replace(/\${workspaceRoot}/g, vscode.workspace.rootPath);
, but didn't find any other way to do so.Regards,
Guilherme