src/goInstallTools: avoid error when tool is in dev version
The error was unhandled and prevented debugging feature from
starting. This failure wasn't detected because of the buggy
tests.
Also clarify what happens when `go version -m` is not what
expected in the comment.
And fix the broken version comparison test suite
- testShouldUpdateTool is async, so the test should return
Promise (or await)
- suite.only was committed by mistake
Change-Id: I6855bfcc3fc83d74d0e5fe9d35d38d8d1e6ad48b
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/303050
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
diff --git a/src/goInstallTools.ts b/src/goInstallTools.ts
index 9d37f52..7c4e4ba 100644
--- a/src/goInstallTools.ts
+++ b/src/goInstallTools.ts
@@ -629,16 +629,29 @@
// inspectGoToolVersion reads the go version and module version
// of the given go tool using `go version -m` command.
-export async function inspectGoToolVersion(binPath: string): Promise<{ goVersion?: string; moduleVersion?: string }> {
+export const inspectGoToolVersion = defaultInspectGoToolVersion;
+async function defaultInspectGoToolVersion(binPath: string): Promise<{ goVersion?: string; moduleVersion?: string }> {
const goCmd = getBinPath('go');
const execFile = util.promisify(cp.execFile);
try {
const { stdout } = await execFile(goCmd, ['version', '-m', binPath]);
- /* The output format will look like this:
+ /* The output format will look like this
+
+ if the binary was built in module mode.
/Users/hakim/go/bin/gopls: go1.16
path golang.org/x/tools/gopls
mod golang.org/x/tools/gopls v0.6.6 h1:GmCsAKZMEb1BD1BTWnQrMyx4FmNThlEsmuFiJbLBXio=
dep github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
+
+ if the binary was built in GOPATH mode => the following code will throw an error which will be handled.
+ /Users/hakim/go/bin/gopls: go1.16
+
+ if the binary was built in dev branch, in module mode => the following code will not throw an error,
+ and return (devel) as the moduleVersion.
+ /Users/hakim/go/bin/gopls: go1.16
+ path golang.org/x/tools/gopls
+ mod golang.org/x/tools/gopls (devel)
+ dep github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
*/
const lines = stdout.split('\n', 3);
const goVersion = lines[0].split(/\s+/)[1];
@@ -667,6 +680,10 @@
return false; // failed to inspect the tool version.
}
const localVersion = semver.parse(moduleVersion, { includePrerelease: true });
+ if (!localVersion) {
+ // local version can't be determined. e.g. (devel)
+ return false;
+ }
return semver.lt(localVersion, tool.latestVersion);
// update only if the local version is older than the desired version.
diff --git a/test/gopls/update.test.ts b/test/gopls/update.test.ts
index bf8f095..d27e97c 100644
--- a/test/gopls/update.test.ts
+++ b/test/gopls/update.test.ts
@@ -187,7 +187,7 @@
});
});
-suite.only('version comparison', () => {
+suite('version comparison', () => {
const tool = getTool('dlv-dap');
const latestVersion = tool.latestVersion;
@@ -197,34 +197,35 @@
async function testShouldUpdateTool(expected: boolean, moduleVersion?: string) {
sinon.stub(goInstallTools, 'inspectGoToolVersion').returns(Promise.resolve({ moduleVersion }));
+ const got = await goInstallTools.shouldUpdateTool(tool, '/bin/path/to/dlv-dap');
assert.strictEqual(
expected,
- goInstallTools.shouldUpdateTool(tool, '/bin/path/to/dlv-dap'),
+ got,
`hard-coded minimum: ${tool.latestVersion.toString()} vs localVersion: ${moduleVersion}`
);
}
test('local delve is old', async () => {
- testShouldUpdateTool(true, 'v1.6.0');
+ await testShouldUpdateTool(true, 'v1.6.0');
});
test('local delve is the minimum required version', async () => {
- testShouldUpdateTool(false, 'v' + latestVersion.toString());
+ await testShouldUpdateTool(false, 'v' + latestVersion.toString());
});
test('local delve is newer', async () => {
- testShouldUpdateTool(false, `v${latestVersion.major}.${latestVersion.minor + 1}.0`);
+ await testShouldUpdateTool(false, `v${latestVersion.major}.${latestVersion.minor + 1}.0`);
});
test('local delve is slightly older', async () => {
- testShouldUpdateTool(
+ await testShouldUpdateTool(
true,
- `v{$latestVersion.major}.${latestVersion.minor}.${latestVersion.patch}-0.20201231000000-5360c6286949`
+ `v${latestVersion.major}.${latestVersion.minor}.${latestVersion.patch}-0.20201231000000-5360c6286949`
);
});
test('local delve is slightly newer', async () => {
- testShouldUpdateTool(
+ await testShouldUpdateTool(
false,
`v{$latestVersion.major}.${latestVersion.minor}.${latestVersion.patch}-0.30211231000000-5360c6286949`
);
@@ -232,16 +233,16 @@
test('local delve version is unknown', async () => {
// maybe a wrapper shellscript?
- testShouldUpdateTool(false, undefined);
+ await testShouldUpdateTool(false, undefined);
});
test('local delve version is non-sense', async () => {
// maybe a wrapper shellscript?
- testShouldUpdateTool(false, 'hello');
+ await testShouldUpdateTool(false, 'hello');
});
test('local delve version is non-sense again', async () => {
// maybe a wrapper shellscript?
- testShouldUpdateTool(false, '');
+ await testShouldUpdateTool(false, '');
});
});
