Skip to content

Commit

Permalink
fix #1041 when debugging a test do not cancel it when re-discovering …
Browse files Browse the repository at this point in the history 
…tests (#1354)

* fix 1041 when debugging a test do not cancel it when re-discovering tests

* create enum for creation of cancellation token

* dispose correct cancellationToken
  • Loading branch information
@DonJayamanne
DonJayamanne authored Oct 30, 2017
1 parent 1e54bd9 commit 45a456b
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 43 deletions.
81 changes: 55 additions & 26 deletions src/client/unittests/common/baseTestManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,25 @@ import { resetTestResults, displayTestErrorMessage, storeDiscoveredTests } from
import { Installer, Product } from '../../common/installer';
import { isNotInstalledError } from '../../common/helpers';

enum CancellationTokenType {
testDicovery,
testRunner
}

export abstract class BaseTestManager {
private tests: Tests;
private _status: TestStatus = TestStatus.Unknown;
private cancellationTokenSource: vscode.CancellationTokenSource;
private testDiscoveryCancellationTokenSource: vscode.CancellationTokenSource;
private testRunnerCancellationTokenSource: vscode.CancellationTokenSource;
private installer: Installer;
protected get cancellationToken(): vscode.CancellationToken {
if (this.cancellationTokenSource) {
return this.cancellationTokenSource.token;
protected get testDiscoveryCancellationToken(): vscode.CancellationToken {
if (this.testDiscoveryCancellationTokenSource) {
return this.testDiscoveryCancellationTokenSource.token;
}
}
protected get testRunnerCancellationToken(): vscode.CancellationToken {
if (this.testRunnerCancellationTokenSource) {
return this.testRunnerCancellationTokenSource.token;
}
}
public dispose() {
Expand All @@ -21,8 +32,11 @@ export abstract class BaseTestManager {
return this._status;
}
public stop() {
if (this.cancellationTokenSource) {
this.cancellationTokenSource.cancel();
if (this.testDiscoveryCancellationTokenSource) {
this.testDiscoveryCancellationTokenSource.cancel();
}
if (this.testRunnerCancellationTokenSource) {
this.testRunnerCancellationTokenSource.cancel();
}
}
constructor(private testProvider: string, private product: Product, protected rootDirectory: string, protected outputChannel: vscode.OutputChannel) {
Expand All @@ -40,18 +54,29 @@ export abstract class BaseTestManager {

resetTestResults(this.tests);
}
private createCancellationToken() {
this.disposeCancellationToken();
this.cancellationTokenSource = new vscode.CancellationTokenSource();
private createCancellationToken(tokenType: CancellationTokenType) {
this.disposeCancellationToken(tokenType);
if (tokenType === CancellationTokenType.testDicovery) {
this.testDiscoveryCancellationTokenSource = new vscode.CancellationTokenSource();
} else {
this.testRunnerCancellationTokenSource = new vscode.CancellationTokenSource();
}
}
private disposeCancellationToken() {
if (this.cancellationTokenSource) {
this.cancellationTokenSource.dispose();
private disposeCancellationToken(tokenType: CancellationTokenType) {
if (tokenType === CancellationTokenType.testDicovery) {
if (this.testDiscoveryCancellationTokenSource) {
this.testDiscoveryCancellationTokenSource.dispose();
}
this.testDiscoveryCancellationTokenSource = null;
} else {
if (this.testRunnerCancellationTokenSource) {
this.testRunnerCancellationTokenSource.dispose();
}
this.testRunnerCancellationTokenSource = null;
}
this.cancellationTokenSource = null;
}
private discoverTestsPromise: Promise<Tests>;
discoverTests(ignoreCache: boolean = false, quietMode: boolean = false): Promise<Tests> {
discoverTests(ignoreCache: boolean = false, quietMode: boolean = false, isUserInitiated: boolean = true): Promise<Tests> {
if (this.discoverTestsPromise) {
return this.discoverTestsPromise;
}
Expand All @@ -61,8 +86,10 @@ export abstract class BaseTestManager {
return Promise.resolve(this.tests);
}
this._status = TestStatus.Discovering;

this.createCancellationToken();
if (isUserInitiated) {
this.stop();
}
this.createCancellationToken(CancellationTokenType.testDicovery);
return this.discoverTestsPromise = this.discoverTestsImpl(ignoreCache)
.then(tests => {
this.tests = tests;
Expand All @@ -85,7 +112,7 @@ export abstract class BaseTestManager {
displayTestErrorMessage('There were some errors in disovering unit tests');
}
storeDiscoveredTests(tests);
this.disposeCancellationToken();
this.disposeCancellationToken(CancellationTokenType.testDicovery);

return tests;
}).catch(reason => {
Expand All @@ -95,7 +122,7 @@ export abstract class BaseTestManager {

this.tests = null;
this.discoverTestsPromise = null;
if (this.cancellationToken && this.cancellationToken.isCancellationRequested) {
if (this.testDiscoveryCancellationToken && this.testDiscoveryCancellationToken.isCancellationRequested) {
reason = CANCELLATION_REASON;
this._status = TestStatus.Idle;
}
Expand All @@ -105,7 +132,7 @@ export abstract class BaseTestManager {
this.outputChannel.appendLine('' + reason);
}
storeDiscoveredTests(null);
this.disposeCancellationToken();
this.disposeCancellationToken(CancellationTokenType.testDicovery);
return Promise.reject(reason);
});
}
Expand Down Expand Up @@ -144,14 +171,15 @@ export abstract class BaseTestManager {
}

this._status = TestStatus.Running;
this.createCancellationToken();
this.stop();
this.createCancellationToken(CancellationTokenType.testDicovery);
// If running failed tests, then don't clear the previously build UnitTests
// If we do so, then we end up re-discovering the unit tests and clearing previously cached list of failed tests
// Similarly, if running a specific test or test file, don't clear the cache (possible tests have some state information retained)
const clearDiscoveredTestCache = runFailedTests || moreInfo.Run_Specific_File || moreInfo.Run_Specific_Class || moreInfo.Run_Specific_Function ? false : true;
return this.discoverTests(clearDiscoveredTestCache, true)
return this.discoverTests(clearDiscoveredTestCache, true, true)
.catch(reason => {
if (this.cancellationToken && this.cancellationToken.isCancellationRequested) {
if (this.testDiscoveryCancellationToken && this.testDiscoveryCancellationToken.isCancellationRequested) {
return Promise.reject<Tests>(reason);
}
displayTestErrorMessage('Errors in discovering tests, continuing with tests');
Expand All @@ -161,22 +189,23 @@ export abstract class BaseTestManager {
};
})
.then(tests => {
this.createCancellationToken(CancellationTokenType.testRunner);
return this.runTestImpl(tests, testsToRun, runFailedTests, debug);
}).then(() => {
this._status = TestStatus.Idle;
this.disposeCancellationToken();
this.disposeCancellationToken(CancellationTokenType.testRunner);
return this.tests;
}).catch(reason => {
if (this.cancellationToken && this.cancellationToken.isCancellationRequested) {
if (this.testRunnerCancellationToken && this.testRunnerCancellationToken.isCancellationRequested) {
reason = CANCELLATION_REASON;
this._status = TestStatus.Idle;
}
else {
this._status = TestStatus.Error;
}
this.disposeCancellationToken();
this.disposeCancellationToken(CancellationTokenType.testRunner);
return Promise.reject<Tests>(reason);
});
}
abstract runTestImpl(tests: Tests, testsToRun?: TestsToRun, runFailedTests?: boolean, debug?: boolean): Promise<any>;
}
}
22 changes: 11 additions & 11 deletions src/client/unittests/main.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export function activate(context: vscode.ExtensionContext, outputChannel: vscode
if (settings.unitTest.nosetestsEnabled || settings.unitTest.pyTestEnabled || settings.unitTest.unittestEnabled) {
// Ignore the exceptions returned
// This function is invoked via a command which will be invoked else where in the extension
discoverTests(true).catch(() => {
discoverTests(true, false).catch(() => {
// Ignore the errors
});
}
Expand All @@ -61,7 +61,7 @@ async function onDocumentSaved(doc: vscode.TextDocument): Promise<void> {
return;
}

let tests = await testManager.discoverTests(false, true);
let tests = await testManager.discoverTests(false, true, false);
if (!tests || !Array.isArray(tests.testFiles) || tests.testFiles.length === 0) {
return;
}
Expand All @@ -72,7 +72,7 @@ async function onDocumentSaved(doc: vscode.TextDocument): Promise<void> {
if (timeoutId) {
clearTimeout(timeoutId);
}
timeoutId = setTimeout(() => { discoverTests(true); }, 1000);
timeoutId = setTimeout(() => { discoverTests(true, false); }, 1000);
}

function dispose() {
Expand All @@ -91,7 +91,7 @@ function registerCommands(): vscode.Disposable[] {
disposables.push(vscode.commands.registerCommand(constants.Commands.Tests_Discover, () => {
// Ignore the exceptions returned
// This command will be invoked else where in the extension
discoverTests(true).catch(() => { return null; });
discoverTests(true, true).catch(() => { return null; });
}));
disposables.push(vscode.commands.registerCommand(constants.Commands.Tests_Run_Failed, () => runTestsImpl(true)));
disposables.push(vscode.commands.registerCommand(constants.Commands.Tests_Run, (testId) => runTestsImpl(testId)));
Expand Down Expand Up @@ -134,7 +134,7 @@ function selectAndRunTestMethod(debug?: boolean) {
if (!testManager) {
return displayTestFrameworkError(outChannel);
}
testManager.discoverTests(true, true).then(() => {
testManager.discoverTests(true, true, true).then(() => {
const tests = getDiscoveredTests();
testDisplay = testDisplay ? testDisplay : new TestDisplay();
testDisplay.selectTestFunction(getTestWorkingDirectory(), tests).then(testFn => {
Expand All @@ -147,7 +147,7 @@ function selectAndRunTestFile() {
if (!testManager) {
return displayTestFrameworkError(outChannel);
}
testManager.discoverTests(true, true).then(() => {
testManager.discoverTests(true, true, true).then(() => {
const tests = getDiscoveredTests();
testDisplay = testDisplay ? testDisplay : new TestDisplay();
testDisplay.selectTestFile(getTestWorkingDirectory(), tests).then(testFile => {
Expand All @@ -164,7 +164,7 @@ function runCurrentTestFile() {
if (!testManager) {
return displayTestFrameworkError(outChannel);
}
testManager.discoverTests(true, true).then(() => {
testManager.discoverTests(true, true, true).then(() => {
const tests = getDiscoveredTests();
const testFiles = tests.testFiles.filter(testFile => {
return testFile.fullPath === currentFilePath;
Expand Down Expand Up @@ -225,7 +225,7 @@ function onConfigChanged() {

// No need to display errors
if (settings.unitTest.nosetestsEnabled || settings.unitTest.pyTestEnabled || settings.unitTest.unittestEnabled) {
discoverTests(true);
discoverTests(true, false);
}
}
function getTestRunner() {
Expand All @@ -248,7 +248,7 @@ function stopTests() {
testManager.stop();
}
}
function discoverTests(ignoreCache?: boolean) {
function discoverTests(ignoreCache?: boolean, isUserInitiated?: boolean) {
let testManager = getTestRunner();
if (!testManager) {
displayTestFrameworkError(outChannel);
Expand All @@ -257,7 +257,7 @@ function discoverTests(ignoreCache?: boolean) {

if (testManager && (testManager.status !== TestStatus.Discovering && testManager.status !== TestStatus.Running)) {
testResultDisplay = testResultDisplay ? testResultDisplay : new TestResultDisplay(outChannel, onDidChange);
return testResultDisplay.DisplayDiscoverStatus(testManager.discoverTests(ignoreCache));
return testResultDisplay.DisplayDiscoverStatus(testManager.discoverTests(ignoreCache, false, isUserInitiated));
}
else {
return Promise.resolve(null);
Expand Down Expand Up @@ -317,4 +317,4 @@ function runTestsImpl(arg?: vscode.Uri | TestsToRun | boolean | FlattenedTestFun
});

testResultDisplay.DisplayProgressStatus(runPromise, debug);
}
}
4 changes: 2 additions & 2 deletions src/client/unittests/nosetest/main.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export class TestManager extends BaseTestManager {
}
discoverTestsImpl(ignoreCache: boolean): Promise<Tests> {
let args = settings.unitTest.nosetestArgs.slice(0);
return discoverTests(this.rootDirectory, args, this.cancellationToken, ignoreCache, this.outputChannel);
return discoverTests(this.rootDirectory, args, this.testDiscoveryCancellationToken, ignoreCache, this.outputChannel);
}
runTestImpl(tests: Tests, testsToRun?: TestsToRun, runFailedTests?: boolean, debug?: boolean): Promise<any> {
let args = settings.unitTest.nosetestArgs.slice(0);
Expand All @@ -26,6 +26,6 @@ export class TestManager extends BaseTestManager {
if (!runFailedTests && args.indexOf('--with-id') === -1) {
args.push('--with-id');
}
return runTest(this.rootDirectory, tests, args, testsToRun, this.cancellationToken, this.outputChannel, debug);
return runTest(this.rootDirectory, tests, args, testsToRun, this.testRunnerCancellationToken, this.outputChannel, debug);
}
}
4 changes: 2 additions & 2 deletions src/client/unittests/pytest/main.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@ export class TestManager extends BaseTestManager {
}
discoverTestsImpl(ignoreCache: boolean): Promise<Tests> {
let args = settings.unitTest.pyTestArgs.slice(0);
return discoverTests(this.rootDirectory, args, this.cancellationToken, ignoreCache, this.outputChannel);
return discoverTests(this.rootDirectory, args, this.testDiscoveryCancellationToken, ignoreCache, this.outputChannel);
}
runTestImpl(tests: Tests, testsToRun?: TestsToRun, runFailedTests?: boolean, debug?: boolean): Promise<any> {
let args = settings.unitTest.pyTestArgs.slice(0);
if (runFailedTests === true && args.indexOf('--lf') === -1 && args.indexOf('--last-failed') === -1) {
args.push('--last-failed');
}
return runTest(this.rootDirectory, tests, args, testsToRun, this.cancellationToken, this.outputChannel, debug);
return runTest(this.rootDirectory, tests, args, testsToRun, this.testRunnerCancellationToken, this.outputChannel, debug);
}
}
4 changes: 2 additions & 2 deletions src/client/unittests/unittest/main.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export class TestManager extends BaseTestManager {
}
discoverTestsImpl(ignoreCache: boolean): Promise<Tests> {
let args = settings.unitTest.unittestArgs.slice(0);
return discoverTests(this.rootDirectory, args, this.cancellationToken, ignoreCache, this.outputChannel);
return discoverTests(this.rootDirectory, args, this.testDiscoveryCancellationToken, ignoreCache, this.outputChannel);
}
runTestImpl(tests: Tests, testsToRun?: TestsToRun, runFailedTests?: boolean, debug?: boolean): Promise<any> {
let args = settings.unitTest.unittestArgs.slice(0);
Expand All @@ -26,6 +26,6 @@ export class TestManager extends BaseTestManager {
return fn.testFunction.status === TestStatus.Error || fn.testFunction.status === TestStatus.Fail;
}).map(fn => fn.testFunction);
}
return runTest(this, this.rootDirectory, tests, args, testsToRun, this.cancellationToken, this.outputChannel, debug);
return runTest(this, this.rootDirectory, tests, args, testsToRun, this.testRunnerCancellationToken, this.outputChannel, debug);
}
}

0 comments on commit 45a456b

Please sign in to comment.