Skip to content

Commit c13bcfe

Browse files
hyangahhyangah
committed
src/testUtils: fix test streaming output handling
The intention of https://go-review.googlesource.com/c/vscode-go/+/246918 was to enable `-json` only if the user already uses `-v` because `-json` automatically enables the verbose mode and `-v` is the way to activate the streaming test output feature in the go command. By mistake we scanned the argument list before the user specified flag list was merged. As a result, `-json` flag was never added. This change corrects the bug. This time, we add tests to avoid regression. In order to make testing easier this CL restructures the goTest function: 1) moves test target argument computation to the getTestTargetPackages, and 2) moves the test command generation into the computeTestCommand function. We also fix a bug that caused the flags located after `-args` to influence our command line computation (-run, -tags). Everything after `-args` should be considered user's custom arg and passed as it is without modification. Fixes #900 Updates #316 Change-Id: Idc11a1ba5d3f5f4cf5467192801c539123b8c241 Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/268839 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: Peter Weinberger <pjw@google.com>
1 parent 2c5a0b9 commit c13bcfe

File tree

2 files changed

+211
-85
lines changed

2 files changed

+211
-85
lines changed

‎src/testUtils.ts‎

Lines changed: 128 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -257,7 +257,6 @@ export async function goTest(testconfig: TestConfig): Promise<boolean> {
257257
return Promise.resolve(false);
258258
}
259259

260-
const tmpCoverPath = testconfig.applyCodeCoverage ? getTempFilePath('go-code-cover') : undefined;
261260
// We do not want to clear it if tests are already running, as that could
262261
// lose valuable output.
263262
if (runningTestProcesses.length < 1) {
@@ -268,90 +267,15 @@ export async function goTest(testconfig: TestConfig): Promise<boolean> {
268267
outputChannel.show(true);
269268
}
270269

271-
const testTags: string = getTestTags(testconfig.goConfig);
272-
const args: Array<string> = ['test'];
273270
const testType: string = testconfig.isBenchmark ? 'Benchmarks' : 'Tests';
274271

275-
if (testconfig.isBenchmark) {
276-
args.push('-benchmem', '-run=^$');
277-
} else {
278-
args.push('-timeout', testconfig.goConfig['testTimeout']);
279-
if (testconfig.applyCodeCoverage) {
280-
args.push('-coverprofile=' + tmpCoverPath);
281-
const coverMode = testconfig.goConfig['coverMode'];
282-
switch (coverMode) {
283-
case 'default':
284-
break;
285-
case 'set': case 'count': case 'atomic':
286-
args.push('-covermode', coverMode);
287-
break;
288-
default:
289-
vscode.window.showWarningMessage(
290-
`go.coverMode=${coverMode} is illegal. Use 'set', 'count', 'atomic', or 'default'.`
291-
);
292-
}
293-
}
294-
}
295-
296-
if (testTags && testconfig.flags.indexOf('-tags') === -1) {
297-
args.push('-tags', testTags);
298-
}
272+
// compute test target package
273+
const { targets, pkgMap, currentGoWorkspace } = await getTestTargetPackages(testconfig, outputChannel);
299274

300-
const targets = testconfig.includeSubDirectories ? ['./...'] : targetArgs(testconfig);
275+
// generate full test args.
276+
const { args, outArgs, tmpCoverPath, addJSONFlag } = computeTestCommand(testconfig, targets);
301277

302-
let currentGoWorkspace = '';
303-
let getCurrentPackagePromise: Promise<string>;
304-
let pkgMapPromise: Promise<Map<string, string>>;
305-
if (testconfig.isMod) {
306-
getCurrentPackagePromise = getCurrentPackage(testconfig.dir);
307-
// We need the mapping to get absolute paths for the files in the test output.
308-
pkgMapPromise = getNonVendorPackages(testconfig.dir, !!testconfig.includeSubDirectories);
309-
} else { // GOPATH mode
310-
currentGoWorkspace = getCurrentGoWorkspaceFromGOPATH(getCurrentGoPath(), testconfig.dir);
311-
getCurrentPackagePromise = Promise.resolve(
312-
currentGoWorkspace ? testconfig.dir.substr(currentGoWorkspace.length + 1) : '');
313-
// We dont need mapping, as we can derive the absolute paths from package path
314-
pkgMapPromise = Promise.resolve(null);
315-
}
316-
317-
let pkgMap = new Map<string, string>();
318-
// run go list to populate pkgMap and currentPackage necessary to adjust the test output later.
319-
try {
320-
const [pkgMap0, currentPackage] = await Promise.all([pkgMapPromise, getCurrentPackagePromise]);
321-
if (pkgMap0) {
322-
pkgMap = pkgMap0;
323-
}
324-
// Use the package name to be in the args to enable running tests in symlinked directories
325-
// TODO(hyangah): check why modules mode didn't set currentPackage.
326-
if (!testconfig.includeSubDirectories && currentPackage) {
327-
targets.splice(0, 0, currentPackage);
328-
}
329-
} catch (err) {
330-
outputChannel.appendLine(`warning: failed to compute package mapping... ${err}`);
331-
}
332-
333-
const outTargets = args.slice(0);
334-
if (targets.length > 4) {
335-
outTargets.push('<long arguments omitted>');
336-
} else {
337-
outTargets.push(...targets);
338-
}
339-
340-
if (args.includes('-v') && !args.includes('-json')) {
341-
args.push('-json');
342-
}
343-
344-
args.push(...targets);
345-
346-
// ensure that user provided flags are appended last (allow use of -args ...)
347-
// ignore user provided -run flag if we are already using it
348-
if (args.indexOf('-run') > -1) {
349-
removeRunFlag(testconfig.flags);
350-
}
351-
args.push(...testconfig.flags);
352-
outTargets.push(...testconfig.flags);
353-
354-
outputChannel.appendLine(['Running tool:', goRuntimePath, ...outTargets].join(' '));
278+
outputChannel.appendLine(['Running tool:', goRuntimePath, ...outArgs].join(' '));
355279
outputChannel.appendLine('');
356280

357281
let testResult = false;
@@ -363,7 +287,7 @@ export async function goTest(testconfig: TestConfig): Promise<boolean> {
363287
const errBuf = new LineBuffer();
364288

365289
const testResultLines: string[] = [];
366-
const processTestResultLine = args.includes('-json') ?
290+
const processTestResultLine = addJSONFlag ?
367291
processTestResultLineInJSONMode(pkgMap, currentGoWorkspace, outputChannel) :
368292
processTestResultLineInStandardMode(pkgMap, currentGoWorkspace, testResultLines, outputChannel);
369293

@@ -410,12 +334,133 @@ export async function goTest(testconfig: TestConfig): Promise<boolean> {
410334
outputChannel.appendLine(`Error: ${testType} failed.`);
411335
outputChannel.appendLine(err);
412336
}
413-
if (testconfig.applyCodeCoverage) {
337+
if (tmpCoverPath) {
414338
await applyCodeCoverageToAllEditors(tmpCoverPath, testconfig.dir);
415339
}
416340
return testResult;
417341
}
418342

343+
async function getTestTargetPackages(testconfig: TestConfig, outputChannel: vscode.OutputChannel) {
344+
const targets = testconfig.includeSubDirectories ? ['./...'] : [];
345+
let currentGoWorkspace = '';
346+
let getCurrentPackagePromise: Promise<string>;
347+
let pkgMapPromise: Promise<Map<string, string>>;
348+
if (testconfig.isMod) {
349+
getCurrentPackagePromise = getCurrentPackage(testconfig.dir);
350+
// We need the mapping to get absolute paths for the files in the test output.
351+
pkgMapPromise = getNonVendorPackages(testconfig.dir, !!testconfig.includeSubDirectories);
352+
} else { // GOPATH mode
353+
currentGoWorkspace = getCurrentGoWorkspaceFromGOPATH(getCurrentGoPath(), testconfig.dir);
354+
getCurrentPackagePromise = Promise.resolve(
355+
currentGoWorkspace ? testconfig.dir.substr(currentGoWorkspace.length + 1) : '');
356+
// We dont need mapping, as we can derive the absolute paths from package path
357+
pkgMapPromise = Promise.resolve(null);
358+
}
359+
360+
let pkgMap = new Map<string, string>();
361+
// run go list to populate pkgMap and currentPackage necessary to adjust the test output later.
362+
try {
363+
const [pkgMap0, currentPackage] = await Promise.all([pkgMapPromise, getCurrentPackagePromise]);
364+
if (pkgMap0) {
365+
pkgMap = pkgMap0;
366+
}
367+
// Use the package name to be in the args to enable running tests in symlinked directories
368+
// TODO(hyangah): check why modules mode didn't set currentPackage.
369+
if (!testconfig.includeSubDirectories && currentPackage) {
370+
targets.splice(0, 0, currentPackage);
371+
}
372+
} catch (err) {
373+
outputChannel.appendLine(`warning: failed to compute package mapping... ${err}`);
374+
}
375+
return { targets, pkgMap, currentGoWorkspace };
376+
}
377+
378+
// computeTestCommand returns the test command argument list and extra info necessary
379+
// to post process the test results.
380+
// Exported for testing.
381+
export function computeTestCommand(testconfig: TestConfig, targets: string[])
382+
: {
383+
args: Array<string>, // test command args.
384+
outArgs: Array<string>, // compact test command args to show to user.
385+
tmpCoverPath?: string, // coverage file path if coverage info is necessary.
386+
addJSONFlag: boolean // true if we add extra -json flag for stream processing.
387+
} {
388+
const args: Array<string> = ['test'];
389+
// user-specified flags
390+
const argsFlagIdx = testconfig.flags?.indexOf('-args') ?? -1;
391+
const userFlags = argsFlagIdx < 0 ? testconfig.flags : testconfig.flags.slice(0, argsFlagIdx);
392+
const userArgsFlags = argsFlagIdx < 0 ? [] : testconfig.flags.slice(argsFlagIdx);
393+
394+
// flags to limit test time
395+
if (testconfig.isBenchmark) {
396+
args.push('-benchmem', '-run=^$');
397+
} else {
398+
args.push('-timeout', testconfig.goConfig['testTimeout']);
399+
}
400+
401+
// tags flags only if user didn't set -tags yet.
402+
const testTags: string = getTestTags(testconfig.goConfig);
403+
if (testTags && userFlags.indexOf('-tags') === -1) {
404+
args.push('-tags', testTags);
405+
}
406+
407+
// coverage flags
408+
let tmpCoverPath: string;
409+
if (testconfig.applyCodeCoverage) {
410+
tmpCoverPath = getTempFilePath('go-code-cover');
411+
args.push('-coverprofile=' + tmpCoverPath);
412+
const coverMode = testconfig.goConfig['coverMode'];
413+
switch (coverMode) {
414+
case 'default':
415+
break;
416+
case 'set': case 'count': case 'atomic':
417+
args.push('-covermode', coverMode);
418+
break;
419+
default:
420+
vscode.window.showWarningMessage(
421+
`go.coverMode=${coverMode} is illegal. Use 'set', 'count', 'atomic', or 'default'.`
422+
);
423+
}
424+
}
425+
426+
// all other test run/benchmark flags
427+
args.push(...targetArgs(testconfig));
428+
429+
const outArgs = args.slice(0); // command to show
430+
431+
// if user set -v, set -json to emulate streaming test output
432+
const addJSONFlag = userFlags.includes('-v') && !userFlags.includes('-json');
433+
if (addJSONFlag) {
434+
args.push('-json'); // this is not shown to the user.
435+
}
436+
437+
if (targets.length > 4) {
438+
outArgs.push('<long arguments omitted>');
439+
} else {
440+
outArgs.push(...targets);
441+
}
442+
args.push(...targets);
443+
444+
// ensure that user provided flags are appended last (allow use of -args ...)
445+
// ignore user provided -run flag if we are already using it
446+
if (args.indexOf('-run') > -1) {
447+
removeRunFlag(userFlags);
448+
}
449+
450+
args.push(...userFlags);
451+
outArgs.push(...userFlags);
452+
453+
args.push(...userArgsFlags);
454+
outArgs.push(...userArgsFlags);
455+
456+
return {
457+
args,
458+
outArgs,
459+
tmpCoverPath,
460+
addJSONFlag
461+
};
462+
}
463+
419464
function processTestResultLineInJSONMode(
420465
pkgMap: Map<string, string>,
421466
currentGoWorkspace: string,

‎test/integration/test.test.ts‎

Lines changed: 83 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,89 @@ import os = require('os');
1111
import path = require('path');
1212
import sinon = require('sinon');
1313
import vscode = require('vscode');
14-
import { getTestFlags, goTest } from '../../src/testUtils';
15-
import { rmdirRecursive } from '../../src/util';
14+
import { computeTestCommand, getTestFlags, goTest } from '../../src/testUtils';
15+
import { getGoConfig, rmdirRecursive } from '../../src/util';
16+
17+
suite('Test Go Test Args', () => {
18+
function runTest(param: {
19+
expectedArgs: string,
20+
expectedOutArgs: string,
21+
flags?: string[],
22+
functions?: string[],
23+
isBenchmark?: boolean
24+
}) {
25+
26+
const {args, outArgs} = computeTestCommand({
27+
dir: '',
28+
goConfig: getGoConfig(),
29+
flags: param.flags || [],
30+
functions: param.functions || [],
31+
isBenchmark: param.isBenchmark || false,
32+
applyCodeCoverage: false,
33+
}, ['./...']);
34+
35+
assert.strictEqual(args.join(' '), param.expectedArgs, 'actual command');
36+
assert.strictEqual(outArgs.join(' '), param.expectedOutArgs, 'displayed command');
37+
}
38+
39+
test('default config', () => {
40+
runTest({
41+
expectedArgs: 'test -timeout 30s ./...',
42+
expectedOutArgs: 'test -timeout 30s ./...'
43+
});
44+
});
45+
test('user flag [-v] enables -json flag', () => {
46+
runTest({
47+
expectedArgs: 'test -timeout 30s -json ./... -v',
48+
expectedOutArgs: 'test -timeout 30s ./... -v',
49+
flags: ['-v']
50+
});
51+
});
52+
test('user flag [-json -v] prevents -json flag addition', () => {
53+
runTest({
54+
expectedArgs: 'test -timeout 30s ./... -json -v',
55+
expectedOutArgs: 'test -timeout 30s ./... -json -v',
56+
flags: ['-json', '-v']
57+
});
58+
});
59+
test('user flag [-args] does not crash', () => {
60+
runTest({
61+
expectedArgs: 'test -timeout 30s ./... -args',
62+
expectedOutArgs: 'test -timeout 30s ./... -args',
63+
flags: ['-args']
64+
});
65+
});
66+
test('user flag [-args -v] does not enable -json flag', () => {
67+
runTest({
68+
expectedArgs: 'test -timeout 30s ./... -args -v',
69+
expectedOutArgs: 'test -timeout 30s ./... -args -v',
70+
flags: ['-args', '-v']
71+
});
72+
});
73+
test('specifying functions adds -run flags', () => {
74+
runTest({
75+
expectedArgs: 'test -timeout 30s -run ^(TestA|TestB)$ ./...',
76+
expectedOutArgs: 'test -timeout 30s -run ^(TestA|TestB)$ ./...',
77+
functions: ['TestA', 'TestB']
78+
});
79+
});
80+
test('functions & benchmark adds -bench flags and skips timeout', () => {
81+
runTest({
82+
expectedArgs: 'test -benchmem -run=^$ -bench ^(TestA|TestB)$ ./...',
83+
expectedOutArgs: 'test -benchmem -run=^$ -bench ^(TestA|TestB)$ ./...',
84+
functions: ['TestA', 'TestB'],
85+
isBenchmark: true,
86+
});
87+
});
88+
test('user -run flag is ignored when functions are provided', () => {
89+
runTest({
90+
expectedArgs: 'test -timeout 30s -run ^(TestA|TestB)$ ./...',
91+
expectedOutArgs: 'test -timeout 30s -run ^(TestA|TestB)$ ./...',
92+
functions: ['TestA', 'TestB'],
93+
flags: ['-run', 'TestC']
94+
});
95+
});
96+
});
1697

1798
suite('Test Go Test', function () {
1899
this.timeout(10000);

0 commit comments

Comments
 (0)