From 45a456bf90643648fcc01f7654bdafde8aa8238d Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Mon, 30 Oct 2017 15:15:43 -0700 Subject: [PATCH] fix #1041 when debugging a test do not cancel it when re-discovering 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 --- .../unittests/common/baseTestManager.ts | 81 +++++++++++++------ src/client/unittests/main.ts | 22 ++--- src/client/unittests/nosetest/main.ts | 4 +- src/client/unittests/pytest/main.ts | 4 +- src/client/unittests/unittest/main.ts | 4 +- 5 files changed, 72 insertions(+), 43 deletions(-) diff --git a/src/client/unittests/common/baseTestManager.ts b/src/client/unittests/common/baseTestManager.ts index e6c31d31a0b..c31935f377c 100644 --- a/src/client/unittests/common/baseTestManager.ts +++ b/src/client/unittests/common/baseTestManager.ts @@ -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() { @@ -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) { @@ -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; - discoverTests(ignoreCache: boolean = false, quietMode: boolean = false): Promise { + discoverTests(ignoreCache: boolean = false, quietMode: boolean = false, isUserInitiated: boolean = true): Promise { if (this.discoverTestsPromise) { return this.discoverTestsPromise; } @@ -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; @@ -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 => { @@ -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; } @@ -105,7 +132,7 @@ export abstract class BaseTestManager { this.outputChannel.appendLine('' + reason); } storeDiscoveredTests(null); - this.disposeCancellationToken(); + this.disposeCancellationToken(CancellationTokenType.testDicovery); return Promise.reject(reason); }); } @@ -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(reason); } displayTestErrorMessage('Errors in discovering tests, continuing with tests'); @@ -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(reason); }); } abstract runTestImpl(tests: Tests, testsToRun?: TestsToRun, runFailedTests?: boolean, debug?: boolean): Promise; -} \ No newline at end of file +} diff --git a/src/client/unittests/main.ts b/src/client/unittests/main.ts index 6ab1f22b2e7..2f6eb9b0bd2 100644 --- a/src/client/unittests/main.ts +++ b/src/client/unittests/main.ts @@ -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 }); } @@ -61,7 +61,7 @@ async function onDocumentSaved(doc: vscode.TextDocument): Promise { 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; } @@ -72,7 +72,7 @@ async function onDocumentSaved(doc: vscode.TextDocument): Promise { if (timeoutId) { clearTimeout(timeoutId); } - timeoutId = setTimeout(() => { discoverTests(true); }, 1000); + timeoutId = setTimeout(() => { discoverTests(true, false); }, 1000); } function dispose() { @@ -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))); @@ -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 => { @@ -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 => { @@ -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; @@ -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() { @@ -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); @@ -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); @@ -317,4 +317,4 @@ function runTestsImpl(arg?: vscode.Uri | TestsToRun | boolean | FlattenedTestFun }); testResultDisplay.DisplayProgressStatus(runPromise, debug); -} \ No newline at end of file +} diff --git a/src/client/unittests/nosetest/main.ts b/src/client/unittests/nosetest/main.ts index 1f3def92cfb..47c1423cfc6 100644 --- a/src/client/unittests/nosetest/main.ts +++ b/src/client/unittests/nosetest/main.ts @@ -16,7 +16,7 @@ export class TestManager extends BaseTestManager { } discoverTestsImpl(ignoreCache: boolean): Promise { 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 { let args = settings.unitTest.nosetestArgs.slice(0); @@ -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); } } diff --git a/src/client/unittests/pytest/main.ts b/src/client/unittests/pytest/main.ts index 8be3477c1b8..4cb56a3eeb0 100644 --- a/src/client/unittests/pytest/main.ts +++ b/src/client/unittests/pytest/main.ts @@ -14,13 +14,13 @@ export class TestManager extends BaseTestManager { } discoverTestsImpl(ignoreCache: boolean): Promise { 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 { 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); } } diff --git a/src/client/unittests/unittest/main.ts b/src/client/unittests/unittest/main.ts index 51cee1b32ac..d48776a338f 100644 --- a/src/client/unittests/unittest/main.ts +++ b/src/client/unittests/unittest/main.ts @@ -16,7 +16,7 @@ export class TestManager extends BaseTestManager { } discoverTestsImpl(ignoreCache: boolean): Promise { 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 { let args = settings.unitTest.unittestArgs.slice(0); @@ -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); } }