Add "Cancel Running Test" status bar item #1218

tylerstillwater · 2017-09-16T18:19:41Z

This PR introduces two new pieces of functionality:

Only one test can run at a time.
a. Previously, if a user invoked a command to run a test while a test was already running, a second, entirely separate test instance would be started.
b. This could cause some unexpected behavior, especially for tests that had a setup/teardown that both test instances were trying to use.
A new "Cancel Running Test" status bar item appears at the bottom left of the status bar whenever a test is running.
a. Clicking this status bar item will cancel the running test.

uudashr · 2017-09-17T03:14:32Z

src/testUtils.ts

-
-			outputChannel.show(true);
-		}
+	return cancelRunningTest().then(success => {


Things to consider. FYI currently more than one test can be run all together.

When the test on save hook enabled {"go.testOnSave": true}, then every time user save file, then it run the test. When one test takes to long then it make sense if it canceled only when the second test run for the same package. If we save file belongs to different package, then it doesn't make sense. This can be happen when we hit "Save All" then multiple test will run.

I hadn't considered that. However, I'm not sure how we can have multiple running tests and also have the ability to cancel running tests. Perhaps keeping a list of all processes in flight and then iterating to cancel all of them?

I've never intentionally run multiple test instances in VSCode before, so the test-per-package-on-save never occurred to me. It seems the output would get very jumbled, though.

@ramya-rao-a I'd love your thoughts here.

@ramya-rao-a bump

am here am here...
Am prepping for an update at the moment, will get right back to this PR later this evening.

Thanks for your patience and sorry for the wait :(

No worries, of course! Thank you for all that you do for the community! :)

Perhaps keeping a list of all processes in flight and then iterating to cancel all of them?

This won't help in the "save all" scenario if you intend to cancel running tests before running a new test. Even though to the user, save all seems like a single event, the extension gets notified once for each document being saved which then triggers the test on the corresponding package.

How about having the cancel test as a command (just like you have now), but have it triggered only by the user and not before running a new test?

You will still have to keep track of all running tests and cancel each of them

That sounds like the best solution if we want to keep the functionality of running tests from multiple packages at once. I'll take a look at it when I have some time. Thanks!

@tylerb @ramya-rao-a Probably there is a hack.
Extension react to the document save event. If we can delay this reaction for example for 1 second interval, capture all the events, after 1 second if we see more than one save events so run the test as one test session (run test only for affected packages).

Probably doesn't need to be 1sec, 500ms is enough.

Something like this https://gist.github.com/uudashr/8d1d74e30b815c3a0eec634a737fdb9b

For now, lets stick to the solution of the user cancelling the tests.

Save All triggering multiple processes also affects build/lint/vet. Let's tackle that in a separate PR

ramya-rao-a · 2017-09-18T17:21:17Z

Slightly busy at work this week, I'll take a look and add my thoughts as soon as possible though.

buyology · 2017-10-11T15:00:28Z

Any update on this? Would be great to have as some tests do not get timed out properly and then the only option is to kill vscode.

tylerstillwater · 2017-11-08T18:40:50Z

I'm still hoping to get this done soon taking all the discussion into account. Work has been crazy lately and I haven't had a chance to wrap this one up.

buyology · 2018-03-21T16:32:15Z

@ramya-rao-a @tylerb — What's left to do on this one? It would be such a great feature to have.

tylerstillwater · 2018-03-21T16:37:49Z

@buyology as per the discussion above, my change of "only allow one test to run at a time" disables some desired behavior. In order for this PR to be wrapped up, the following needs to happen:

1. Revert the "only allow a single test to run at a time" behavior
2. Add "track all tests in flight in a list" functionality
3. On user-initiated test cancellation, cancel all tests in the list.

This is still on my radar, but, unfortunately, pretty far down my list of priorities. I'm consumed by work at the moment.

buyology · 2018-03-21T16:44:06Z

Thanks a lot for the summary 👍. If you don't mind I'll try and see if I can find some time to take a look at this and push this forward

tylerstillwater · 2018-03-21T16:48:39Z

@buyology please do! I want this functionality pretty frequently myself.

tylerstillwater · 2018-03-21T17:29:15Z

I've just rebased to clean up conflicts in my PR

tylerstillwater · 2018-03-21T18:23:39Z

cc @buyology

Decided to take a break from work to just wrap this up. This PR now:

When a test command is run, places the child process in an array of running tests
When the test completes normally, removes the child process from the array.
When the kill command is executed, iterates all running child processes and kills them.
Runs the kill routine at VSCode shutdown to ensure all tests are cleaned up appropriately.

@ramya-rao-a please review

tylerstillwater · 2018-03-26T17:27:47Z

I wanted to use SIGINT to allow go test to handle it and terminate gracefully, but it had no effect when I sent it. Is there something I'm missing? SIGKILL terminates the process immediately, but SIGINT gave no output and did not terminate the process.

ramya-rao-a · 2018-03-26T18:31:30Z

@tylerb Work has been busy this month, I'll get to this PR later this week for sure

tylerstillwater · 2018-03-26T18:42:13Z

@ramya-rao-a it took me 4 months to get to it myself. No worries!

buyology · 2018-04-12T08:34:48Z

@ramya-rao-a — can we please get this is in in the next release? It's such a great feature.

Just look at this sweetness ❤️

buyology · 2018-04-20T12:56:12Z

@ramya-rao-a — any update?

tylerstillwater · 2018-06-04T19:21:28Z

@ramya-rao-a looks like this didn't make it into the latest release. Any particular reason for that? Anything you need me to do?

ramya-rao-a · 2018-06-04T21:20:21Z

One reason I didnt add it to the current release was that I wanted to explore the new feature of notification msg for long running tasks which has the Cancel option.

See https://code.visualstudio.com/updates/v1_22#_extension-authoring

Thoughts?

tylerstillwater · 2018-06-04T22:36:13Z

I didn't know that existed. I like that a lot more than what I have now.

buyology · 2018-06-04T22:41:21Z

Tbh, that progress box looks really intrusive. I really appreciate the elegancy and low keyness in this PR 👍

tylerstillwater · 2018-06-05T15:26:53Z

I'm happy with either approach myself. Mine is done and ready. Perhaps it could be merged and then later updated to the new approach if it turns out that is a better way to do it?

uudashr · 2018-06-05T15:35:25Z

There are two styles here

When I use intellij or eclipse, the test are manually run.
VSCode go, we can hook run test on save.

In my opinion, the first one will fit on the popup. I do care with the result whether it fail or succeed and willing to wait.
The second one, I don't want to wait, don't want to see the process, run it on background and notify me if there the test fail. Here we can put the cancel on the status bar.

So I think we can use both approach.

ramya-rao-a

@tylerb Thanks a ton for your work on this and sorry for the delay.

I have left a few comments, please take a look.

ramya-rao-a · 2018-06-06T00:56:03Z

src/testUtils.ts

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


@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

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

ramya-rao-a · 2018-06-06T00:56:52Z

src/testUtils.ts

-		} else {
-			args.push('-timeout', testconfig.goConfig['testTimeout']);
-		}
+		let args = ['test', ...testconfig.flags, '-timeout', testconfig.goConfig['testTimeout']];


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.

ramya-rao-a · 2018-06-06T00:57:17Z

src/testUtils.ts

@@ -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 });


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

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.

ramya-rao-a · 2018-06-06T00:57:57Z

src/testUtils.ts

 				outBuf.done();
 				errBuf.done();

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


Any reason for removing the use of the testType variable?

No reason. Not sure why this got removed.

ramya-rao-a · 2018-06-06T00:58:18Z

src/testUtils.ts

+ */
+export function cancelRunningTests(): Thenable<boolean> {
+	return new Promise<boolean>((resolve, reject) => {
+		let tp: cp.ChildProcess;


This variable is unused and need not be declared here.

tylerstillwater · 2018-06-06T02:00:52Z

Thanks @ramya-rao-a - I'll take a look at all this and clean things up tomorrow.

tylerstillwater · 2018-06-06T02:23:29Z

@ramya-rao-a I've pushed changes to address your review comments. Thanks!

ramya-rao-a · 2018-06-06T03:36:11Z

src/goMain.ts

@@ -266,6 +266,10 @@ export function activate(ctx: vscode.ExtensionContext): void {
 		showTestOutput();
 	}));

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


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

I've pushed a commit to address this

ramya-rao-a · 2018-06-06T06:06:03Z

src/testUtils.ts

+			tp.kill(sendSignal);
+		});
+		// All processes are now dead. Empty the array to prepare for the next run.
+		testProcesses.splice(0, testProcesses.length);


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

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.

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

tylerstillwater · 2018-06-06T17:10:16Z

Thanks for all your help on this @ramya-rao-a! I'm definitely still learning.

ramya-rao-a · 2018-06-06T17:24:16Z

My pleasure @tylerb! I am learning too :)

buyology · 2018-06-06T19:01:08Z

Great work on this! 🥇So nice to finally have this! 🌞

uudashr reviewed Sep 17, 2017

View reviewed changes

ramya-rao-a force-pushed the master branch 2 times, most recently from 7259f77 to f6936fa Compare October 2, 2017 20:31

ramya-rao-a force-pushed the master branch 3 times, most recently from 89e20b7 to 3d8a1f0 Compare November 4, 2017 19:44

ramya-rao-a force-pushed the master branch 2 times, most recently from 7d591f1 to a8a68e2 Compare November 19, 2017 17:39

ramya-rao-a force-pushed the master branch from bbfe8ba to e0e5d56 Compare January 23, 2018 22:31

Add "Cancel Running Test" status bar item

9a97d8f

tylerb added 2 commits March 26, 2018 11:26

Only hide status bar item if all tests are done

2a14a26

kill() on process directly. improve output.

d374869

Merge remote-tracking branch 'origin/master' into cancel-test

074b530

ramya-rao-a suggested changes Jun 6, 2018

View reviewed changes

Address review comments

fa0b2d6

ramya-rao-a reviewed Jun 6, 2018

View reviewed changes

ramya-rao-a added 2 commits June 5, 2018 23:07

Fix linting errors and expose command to cancel tests

8be76b1

Cleanup

b4b1594

ramya-rao-a approved these changes Jun 6, 2018

View reviewed changes

const vs let

42460e9

ramya-rao-a merged commit 9faef72 into microsoft:master Jun 6, 2018

Add "Cancel Running Test" status bar item #1218

Add "Cancel Running Test" status bar item #1218

Conversation

tylerstillwater commented Sep 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a commented Sep 18, 2017

buyology commented Oct 11, 2017

tylerstillwater commented Nov 8, 2017

buyology commented Mar 21, 2018

tylerstillwater commented Mar 21, 2018

buyology commented Mar 21, 2018

tylerstillwater commented Mar 21, 2018

tylerstillwater commented Mar 21, 2018

tylerstillwater commented Mar 21, 2018

tylerstillwater commented Mar 26, 2018

ramya-rao-a commented Mar 26, 2018

tylerstillwater commented Mar 26, 2018

buyology commented Apr 12, 2018 • edited Loading

buyology commented Apr 20, 2018

tylerstillwater commented Jun 4, 2018

ramya-rao-a commented Jun 4, 2018

tylerstillwater commented Jun 4, 2018

buyology commented Jun 4, 2018 • edited Loading

tylerstillwater commented Jun 5, 2018

uudashr commented Jun 5, 2018

ramya-rao-a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerstillwater commented Jun 6, 2018

tylerstillwater commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerstillwater commented Jun 6, 2018

ramya-rao-a commented Jun 6, 2018

buyology commented Jun 6, 2018

tylerstillwater commented Sep 16, 2017 •

edited

Loading

buyology commented Apr 12, 2018 •

edited

Loading

buyology commented Jun 4, 2018 •

edited

Loading

ramya-rao-a Jun 6, 2018 •

edited

Loading