Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unhandled promise rejections #702

Merged
merged 6 commits into from
Feb 5, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
4 changes: 2 additions & 2 deletions src/test/interpreters/condaEnvFileService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,11 @@ suite('Interpreters from Conda Environments Text File', () => {
fileSystem.setup(fs => fs.fileExistsAsync(TypeMoq.It.isValue(environmentsFilePath))).returns(() => Promise.resolve(true));
fileSystem.setup(fs => fs.readFile(TypeMoq.It.isValue(environmentsFilePath))).returns(() => Promise.resolve(interpreterPaths.join(EOL)));

AnacondaCompanyNames.forEach(async companyDisplayName => {
Copy link
Author

@elliott-beach elliott-beach Feb 4, 2018

Choose a reason for hiding this comment

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

forEach on an async function doesn't do anything.

Choose a reason for hiding this comment

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

Please see below for the fix to this entire test

for (const _ of AnacondaCompanyNames){
const interpreters = await condaFileProvider.getInterpreters();

assert.equal(interpreters.length, 1, 'Incorrect number of entries');
assert.equal(interpreters[0].displayName, `${AnacondaDisplayName} Mock Version (numpy)`, 'Incorrect display name');
});
}
});
});
4 changes: 2 additions & 2 deletions src/test/refactor/extension.refactor.extract.var.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,13 +101,13 @@ suite('Variable Extraction', () => {
test('Extract Variable', async () => {
const startPos = new vscode.Position(234, 29);
const endPos = new vscode.Position(234, 38);
testingVariableExtraction(false, startPos, endPos);
await testingVariableExtraction(false, startPos, endPos);
});

test('Extract Variable fails if whole string not selected', async () => {
const startPos = new vscode.Position(234, 20);
const endPos = new vscode.Position(234, 38);
testingVariableExtraction(true, startPos, endPos);
await testingVariableExtraction(true, startPos, endPos);
});

function testingVariableExtractionEndToEnd(shouldError: boolean, startPos: Position, endPos: Position) {
Expand Down
31 changes: 26 additions & 5 deletions src/test/unittests/debugger.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,31 @@ suite('Unit Tests - debugging', () => {
assert.equal(tests.testFunctions.length, 2, 'Incorrect number of test functions');
assert.equal(tests.testSuites.length, 2, 'Incorrect number of test suites');

const deferred = createDeferred<string>();
Copy link
Author

@elliott-beach elliott-beach Feb 4, 2018

Choose a reason for hiding this comment

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

const testFunction = [tests.testFunctions[0].testFunction];
testManager.runTest(CommandSource.commandPalette, { testFunction }, false, true);
const launched = await mockDebugLauncher.launched;
assert.isTrue(launched, 'Debugger not launched');
const runningPromise = testManager.runTest(CommandSource.commandPalette, { testFunction }, false, true);

// This promise should never resolve nor reject.
runningPromise
.then(() => 'Debugger stopped when it shouldn\'t have')
.catch(() => 'Debugger crashed when it shouldn\'t have')
// tslint:disable-next-line:no-floating-promises
.then(error => {
deferred.reject(error);
});

mockDebugLauncher.launched
// tslint:disable-next-line:no-unsafe-any
.then((launched) => {
if (launched) {
deferred.resolve('');
} else {
deferred.reject('Debugger not launched');
}
})
// tslint:disable-next-line:no-unsafe-any
.catch(ex => deferred.reject(ex));
return await deferred.promise;
}

test('Debugger should start (unittest)', async () => {
Expand Down Expand Up @@ -105,8 +126,7 @@ suite('Unit Tests - debugging', () => {
const launched = await mockDebugLauncher.launched;
assert.isTrue(launched, 'Debugger not launched');

testManager.discoverTests(CommandSource.commandPalette, true, true, true);

await testManager.discoverTests(CommandSource.commandPalette, true, true, true);
await expect(runningPromise).to.be.rejectedWith(CANCELLATION_REASON, 'Incorrect reason for ending the debugger');
}

Expand Down Expand Up @@ -151,6 +171,7 @@ suite('Unit Tests - debugging', () => {
runningPromise
.then(() => 'Debugger stopped when it shouldn\'t have')
.catch(() => 'Debugger crashed when it shouldn\'t have')
// tslint:disable-next-line: no-floating-promises
.then(error => {
deferred.reject(error);
});
Expand Down
26 changes: 23 additions & 3 deletions src/test/unittests/stoppingDiscoverAndTest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { expect, use } from 'chai';
import * as chaiAsPromised from 'chai-as-promised';
import * as path from 'path';
import { Uri } from 'vscode';
import {createDeferred} from '../../client/common/helpers';
import { Product } from '../../client/common/types';
import { CANCELLATION_REASON, CommandSource, UNITTEST_PROVIDER } from '../../client/unittests/common/constants';
import { ITestDiscoveryService } from '../../client/unittests/common/types';
Expand Down Expand Up @@ -60,9 +61,28 @@ suite('Unit Tests Stopping Discovery and Runner', () => {

const discoveryPromise = mockTestManager.discoverTests(CommandSource.auto);
mockTestManager.discoveryDeferred.resolve(EmptyTests);
mockTestManager.runTest(CommandSource.ui);
const runningPromise = mockTestManager.runTest(CommandSource.ui);
const deferred = createDeferred<string>();

await expect(discoveryPromise).to.eventually.equal(EmptyTests);
// This promise should never resolve nor reject.
runningPromise
.then(() => 'Debugger stopped when it shouldn\'t have')

Choose a reason for hiding this comment

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

This won't do anything, if this isn't necessary, then you need to change this line as follows:
.then(() => Promise.reject('Debugger stopped when it shouldn\'t have'))

This will cause it to fail, then the catch will do what's necessary

or
Just fail early
.then(() => deferred.reject('Debugger stopped when it shouldn\'t have'))

Copy link
Author

@elliott-beach elliott-beach Feb 4, 2018

Choose a reason for hiding this comment

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

Sure, makes sense. It is 1:30AM my time and I will fix this tomorrow. Thanks for the review!

.catch(() => 'Debugger crashed when it shouldn\'t have')
// tslint:disable-next-line:no-floating-promises
Copy link
Author

Choose a reason for hiding this comment

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

See https://stackoverflow.com/a/48603963/5749914 for my take on why this is necessary.

Choose a reason for hiding this comment

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

You should change this again as follows:
.catch(() => deferred.reject('Debugger crashed when it shouldn\'t have'))

This way, whether the promise resolves or rejects, we're capturing it and accordingly rejecting the deferred promise.

.then(error => {
deferred.reject(error);

Choose a reason for hiding this comment

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

This .then won't be necessary.

});

discoveryPromise.then(result => {
if (result === EmptyTests) {
deferred.resolve('');
} else {
deferred.reject('tests not empty');
}
}).catch(error => {
deferred.reject(error);
});
await deferred.promise;
});

test('Discovering tests should stop running tests', async () => {
Expand All @@ -75,7 +95,7 @@ suite('Unit Tests Stopping Discovery and Runner', () => {
await new Promise(resolve => setTimeout(resolve, 1000));

// User manually discovering tests will kill the existing test runner.
mockTestManager.discoverTests(CommandSource.ui, true, false, true);
await mockTestManager.discoverTests(CommandSource.ui, true, false, true);
Copy link
Author

@elliott-beach elliott-beach Feb 4, 2018

Choose a reason for hiding this comment

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

This caused the coverage to go down because, without the await, discoverTests would eventually receive a cancellation request, triggering handler code. Edit: that's wrong, it's somewhere else

await expect(runPromise).to.eventually.be.rejectedWith(CANCELLATION_REASON);
});
});