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

Add "Cancel Running Test" status bar item #1218

Merged
merged 8 commits into from
Jun 6, 2018
Merged

Add "Cancel Running Test" status bar item #1218

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9a97d8f
Add "Cancel Running Test" status bar item
tylerb Sep 16, 2017
2a14a26
Only hide status bar item if all tests are done
tylerb Mar 26, 2018
d374869
kill() on process directly. improve output.
tylerb Mar 26, 2018
074b530
Merge remote-tracking branch 'origin/master' into cancel-test
ramya-rao-a Jun 6, 2018
fa0b2d6
Address review comments
tylerb Jun 6, 2018
8be76b1
Fix linting errors and expose command to cancel tests
ramya-rao-a Jun 6, 2018
b4b1594
Cleanup
ramya-rao-a Jun 6, 2018
42460e9
const vs let
ramya-rao-a Jun 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/goMain.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import { showHideStatus } from './goStatus';
import { toggleCoverageCurrentPackage, getCodeCoverage, removeCodeCoverage } from './goCover';
import { initGoCover } from './goCover';
import { testAtCursor, testCurrentPackage, testCurrentFile, testPrevious, testWorkspace } from './goTest';
import { showTestOutput } from './testUtils';
import { showTestOutput, cancelRunningTests } from './testUtils';
import * as goGenerateTests from './goGenerateTests';
import { addImport } from './goImport';
import { installAllTools, checkLanguageServer } from './goInstallTools';
Expand Down Expand Up @@ -266,6 +266,10 @@ export function activate(ctx: vscode.ExtensionContext): void {
showTestOutput();
}));

ctx.subscriptions.push(vscode.commands.registerCommand('go.test.cancel', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should expose this as a command as well. That would help users who are more inclined to use their keyboards and want to avoid the mouse

Copy link
Contributor

Choose a reason for hiding this comment

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

I've pushed a commit to address this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

cancelRunningTests();
}));

ctx.subscriptions.push(vscode.commands.registerCommand('go.import.add', (arg: string) => {
return addImport(typeof arg === 'string' ? arg : null);
}));
Expand Down Expand Up @@ -379,7 +383,7 @@ export function activate(ctx: vscode.ExtensionContext): void {
}

export function deactivate() {
return disposeTelemetryReporter();
return Promise.all([disposeTelemetryReporter(), cancelRunningTests()]);
}

function runBuilds(document: vscode.TextDocument, goConfig: vscode.WorkspaceConfiguration) {
Expand Down
103 changes: 80 additions & 23 deletions src/testUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,16 @@ import { getToolsEnvVars, getGoVersion, LineBuffer, SemVersion, resolvePath, get
import { GoDocumentSymbolProvider } from './goOutline';
import { getNonVendorPackages } from './goPackages';

let sendSignal = "SIGKILL"
let outputChannel = vscode.window.createOutputChannel('Go Tests');
let statusBarItem = vscode.window.createStatusBarItem(vscode.StatusBarAlignment.Left);
statusBarItem.command = 'go.test.cancel';
statusBarItem.text = 'Cancel Running Tests';

/**
* testProcesses holds a list of currently running test processes.
*/
let testProcesses = [];


/**
Expand Down Expand Up @@ -117,33 +126,42 @@ export function getBenchmarkFunctions(doc: vscode.TextDocument, token: vscode.Ca
*/
export function goTest(testconfig: TestConfig): Thenable<boolean> {
return new Promise<boolean>((resolve, reject) => {
outputChannel.clear();

// We only want to clear the outputChannel if there are no tests in flight.
// We do not want to clear it if tests are already running, as that could
// lose valuable output.
if (testProcesses.length < 1) {
outputChannel.clear();
}

if (!testconfig.background) {
let buildTags: string = testconfig.goConfig['buildTags'];
let args: Array<string> = ['test', ...testconfig.flags];
let testType: string = testconfig.isBenchmark ? 'Benchmarks' : 'Tests';
Copy link
Contributor

Choose a reason for hiding this comment

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

@tylerb The above 3 variables do not really get used later. buildTags and args get re-assigned after this if block. Was this intentional?

The net affect is that when benchmarks are run,

  • the -benchmem flag is not used and so the memory related stats wont be shown
  • the -run=^$ flag is not used which means all the tests will be run when the user just wants to run the single benchmark

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what happened here. I'm updating it to reflect the current setup of this function as it exists in master.


if (testconfig.isBenchmark) {
args.push('-benchmem', '-run=^$');
} else {
args.push('-timeout', testconfig.goConfig['testTimeout']);
}
if (buildTags && testconfig.flags.indexOf('-tags') === -1) {
args.push('-tags', buildTags);
}

let testEnvVars = getTestEnvVars(testconfig.goConfig);
let goRuntimePath = getGoRuntimePath();
outputChannel.show(true);
}

let buildTags: string = testconfig.goConfig['buildTags'];
let args: Array<string> = ['test', ...testconfig.flags];
let testType: string = testconfig.isBenchmark ? 'Benchmarks' : 'Tests';

if (testconfig.isBenchmark) {
args.push('-benchmem', '-run=^$');
} else {
args.push('-timeout', testconfig.goConfig['testTimeout']);
}
let args = ['test', ...testconfig.flags, '-timeout', testconfig.goConfig['testTimeout']];
Copy link
Contributor

Choose a reason for hiding this comment

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

This will apply the timeout when running benchmarks as well. I am guessing when benchmarks are run, they should be allowed to take as much time as they need and shouldnt be bound with the timeout.

if (buildTags && testconfig.flags.indexOf('-tags') === -1) {
args.push('-tags', buildTags);
args.push('-tags');
args.push(buildTags);
}

let testEnvVars = getTestEnvVars(testconfig.goConfig);
let goRuntimePath = getGoRuntimePath();

if (!goRuntimePath) {
vscode.window.showInformationMessage('Cannot find "go" binary. Update PATH or GOROOT appropriately');
return Promise.resolve();
}

// Append the package name to args to enable running tests in symlinked directories
let currentGoWorkspace = getCurrentGoWorkspaceFromGOPATH(getCurrentGoPath(), testconfig.dir);
if (currentGoWorkspace && !testconfig.includeSubDirectories) {
Expand All @@ -162,7 +180,7 @@ export function goTest(testconfig: TestConfig): Thenable<boolean> {

args.push(...targets);

let proc = cp.spawn(goRuntimePath, args, { env: testEnvVars, cwd: testconfig.dir });
let tp = cp.spawn(goRuntimePath, args, { env: testEnvVars, cwd: testconfig.dir, detached: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not clear on why we need to run this in a detached mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this was necessary in order to kill the process later when cancel was invoked. However, I may be wrong. I seem to recall seeing some odd behavior without it being detached, but I wrote this a long time ago.

const outBuf = new LineBuffer();
const errBuf = new LineBuffer();

Expand Down Expand Up @@ -195,22 +213,46 @@ export function goTest(testconfig: TestConfig): Thenable<boolean> {
errBuf.onLine(line => outputChannel.appendLine(expandFilePathInOutput(line, testconfig.dir)));
errBuf.onDone(last => last && outputChannel.appendLine(expandFilePathInOutput(last, testconfig.dir)));

proc.stdout.on('data', chunk => outBuf.append(chunk.toString()));
proc.stderr.on('data', chunk => errBuf.append(chunk.toString()));
tp.stdout.on('data', chunk => outBuf.append(chunk.toString()));
tp.stderr.on('data', chunk => errBuf.append(chunk.toString()));

proc.on('close', code => {
statusBarItem.show();

tp.on('close', (code, signal) => {
outBuf.done();
errBuf.done();

if (code) {
outputChannel.appendLine(`Error: ${testType} failed.`);
outputChannel.appendLine('Error: Tests failed.');
} else if (signal === sendSignal) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for removing the use of the testType variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason. Not sure why this got removed.

outputChannel.appendLine('Error: Tests terminated by user.');
} else {
outputChannel.appendLine(`Success: ${testType} passed.`);
outputChannel.appendLine('Success: Tests passed.');
}

// We need to remove this particular test process from the array of test
// processes so that a subsequent cancel does not attempt to kill a
// process that no longer exists. This is only an issue if we have
// multiple test processes running in parallel.
//
// If this test process was killed by calling cancelRunningTests, the
// array will be empty and this entry will not be found or removed.
let index = testProcesses.indexOf(tp, 0);
if (index > -1) {
testProcesses.splice(index, 1);
}

if (testProcesses.length == 0) {
statusBarItem.hide();
}

resolve(code === 0);
});

testProcesses.push(tp);

}, err => {
outputChannel.appendLine(`Error: ${testType} failed.`);
outputChannel.appendLine('Error: Tests failed.');
outputChannel.appendLine(err);
resolve(false);
});
Expand All @@ -224,6 +266,21 @@ export function showTestOutput() {
outputChannel.show(true);
}

/**
* Iterates the list of currently running test processes and kills them all.
*/
export function cancelRunningTests(): Thenable<boolean> {
return new Promise<boolean>((resolve, reject) => {
let tp: cp.ChildProcess;
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable is unused and need not be declared here.

testProcesses.forEach(function(tp){
tp.kill(sendSignal);
});
// All processes are now dead. Empty the array to prepare for the next run.
testProcesses.splice(0, testProcesses.length);
Copy link
Contributor

@ramya-rao-a ramya-rao-a Jun 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Killed test processes go through line 239 where they are removed from the testProcesses array. Do we need to empty the array again here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, I do not know. I don't know if the kills are done in the background while the cancelRunningTests function continues to run. I wrote it such that we would have a clean state were that the case. Is that the case? I know little about the runtime here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not really sure..
But, there is no harm emptying the array at this point so we can keep it

resolve(true);
});
}

function expandFilePathInOutput(output: string, cwd: string): string {
let lines = output.split('\n');
for (let i = 0; i < lines.length; i++) {
Expand Down