Fix unhandled promise rejections #702

elliott-beach · 2018-02-04T05:21:00Z

The tests fail because I unearthed one test that was silently failing, 'Must strip company name from version info'.

Most uncaught rejections were from the promises of testManager.runTest and testManager.discoverTests getting killed when the test ended. They ought to be handled just to prevent this error, but otherwise there is not any code defect.

Fixes #479, I would hope.

Uncaught rejections were from the methods `testManager.runTest` and `testManager.discoverTests` that are killed when tests complete. They ought to be handled just to prevent this error, but otherwise there is not any code defect. I handled it using the same code as in https://github.com/Microsoft/vscode-python/blob/1cd98b2d3c044a1ed69ca2fe6d548998cc52aabf/src/test/unittests/debugger.test.ts#L150-L156. I had to disable tslint in a few places because tslint (from the pre-commit hook) is very strict with promises, see https://stackoverflow.com/q/43980188/5749914.

This was giving errors for me but not in Travis, probably because I did not have rope installed.

elliott-beach · 2018-02-04T05:22:21Z

src/test/interpreters/condaEnvFileService.test.ts

@@ -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 => {


forEach on an async function doesn't do anything.

Please see below for the fix to this entire test

elliott-beach · 2018-02-04T05:23:46Z

src/test/unittests/debugger.test.ts

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


With using deferred I am basically copying from https://github.com/Microsoft/vscode-python/blob/1cd98b2d3c044a1ed69ca2fe6d548998cc52aabf/src/test/unittests/debugger.test.ts#L150-L156

elliott-beach · 2018-02-04T05:26:49Z

src/test/unittests/stoppingDiscoverAndTest.test.ts

+        runningPromise
+            .then(() => 'Debugger stopped when it shouldn\'t have')
+            .catch(() => 'Debugger crashed when it shouldn\'t have')
+            // tslint:disable-next-line:no-floating-promises


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

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.

DonJayamanne · 2018-02-04T07:02:34Z

Thanks for cleaning up the tests, please could you create an issue for this so we can track the PR against an issue.

DonJayamanne · 2018-02-04T07:03:14Z

Also, travis is failing, please could you fix the tests where its failing, I cannot merge the PR without the tests passing.

elliott-beach · 2018-02-04T07:03:16Z

The issue for this is #479.

DonJayamanne · 2018-02-04T07:26:48Z

@elliott-beach oh awesome, here's the fix to the failing travis unit test.
Please include this into your changes, it should fix the test.

    test('Must strip company name from version info', async () => {
        const interpreterPaths = [
            path.join(environmentsPath, 'conda', 'envs', 'numpy')
        ];
        const pythonPath = path.join(interpreterPaths[0], 'pythonPath');
        condaService.setup(c => c.condaEnvironmentsFile).returns(() => environmentsFilePath);
        condaService.setup(c => c.getInterpreterPath(TypeMoq.It.isAny())).returns(() => pythonPath);
        fileSystem.setup(fs => fs.fileExistsAsync(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve(true));
        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)));

        for (const companyName of AnacondaCompanyNames) {
            const versionWithCompanyName = `Mock Version :: ${companyName}`;
            interpreterVersion.setup(c => c.getVersion(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(versionWithCompanyName));
            const interpreters = await condaFileProvider.getInterpreters();

            assert.equal(interpreters.length, 1, 'Incorrect number of entries');
            assert.equal(interpreters[0].displayName, `${AnacondaDisplayName} Mock Version`, 'Incorrect display name');
        }
    });

elliott-beach · 2018-02-04T07:28:33Z

I was hoping you should show me how to fix it, thanks!!!
Also I would never be offended if you wanted to commit on my pull request directly.

DonJayamanne · 2018-02-04T07:29:26Z

also I would never be offended

I don't want to muddy the waters. Thanks though

DonJayamanne · 2018-02-04T06:53:01Z

src/test/unittests/stoppingDiscoverAndTest.test.ts

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


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'))

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

DonJayamanne · 2018-02-04T06:55:36Z

src/test/unittests/stoppingDiscoverAndTest.test.ts

+        runningPromise
+            .then(() => 'Debugger stopped when it shouldn\'t have')
+            .catch(() => 'Debugger crashed when it shouldn\'t have')
+            // tslint:disable-next-line:no-floating-promises


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.

DonJayamanne · 2018-02-04T06:56:07Z

src/test/unittests/stoppingDiscoverAndTest.test.ts

+            .catch(() => 'Debugger crashed when it shouldn\'t have')
+            // tslint:disable-next-line:no-floating-promises
+            .then(error => {
+                deferred.reject(error);


This .then won't be necessary.

codecov · 2018-02-04T19:08:40Z

Codecov Report

Merging #702 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #702      +/-   ##
=========================================
+ Coverage   62.39%   62.4%   +<.01%     
=========================================
  Files         241     241              
  Lines       11109   11109              
  Branches     2001    2001              
=========================================
+ Hits         6932    6933       +1     
+ Misses       4169    4168       -1     
  Partials        8       8

Impacted Files	Coverage Δ
...terpreter/locators/services/condaEnvFileService.ts	`94.54% <0%> (+1.81%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cd98b2...ff9e916. Read the comment docs.

elliott-beach · 2018-02-04T20:32:06Z

src/test/unittests/stoppingDiscoverAndTest.test.ts

@@ -75,7 +90,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);


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

DonJayamanne · 2018-02-04T21:25:51Z

This caused the coverage to go down

In this case (for this PR), as they're fixes in tests, that's fine

elliott-beach · 2018-02-04T21:26:33Z

Oh well I just fixed it, and we might as well cover test cancellation, which is actually code that's shipped, not simply internal test code. So let's see if the CI passes.

elliott-beach · 2018-02-04T22:06:01Z

Hooray, coverage passed!

elliott-beach added 3 commits February 3, 2018 19:23

fix uncaught promise rejections due to forOf

07af7fb

fix possible uncaught promise from var extraction

916a138

This was giving errors for me but not in Travis, probably because I did not have rope installed.

elliott-beach commented Feb 4, 2018

View reviewed changes

DonJayamanne requested changes Feb 4, 2018

View reviewed changes

elliott-beach added 2 commits February 4, 2018 12:32

add DonJayamanne's code

85a9fa5

cleanup

1607174

elliott-beach commented Feb 4, 2018

View reviewed changes

cover cancelling test discovery

ff9e916

DonJayamanne approved these changes Feb 5, 2018

View reviewed changes

DonJayamanne merged commit 896b588 into microsoft:master Feb 5, 2018

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unhandled promise rejections #702

Fix unhandled promise rejections #702

elliott-beach commented Feb 4, 2018 •

edited

Loading

elliott-beach Feb 4, 2018 •

edited

Loading

DonJayamanne Feb 4, 2018

elliott-beach Feb 4, 2018 •

edited

Loading

elliott-beach Feb 4, 2018

DonJayamanne Feb 4, 2018

DonJayamanne commented Feb 4, 2018

DonJayamanne commented Feb 4, 2018

elliott-beach commented Feb 4, 2018 •

edited

Loading

DonJayamanne commented Feb 4, 2018 •

edited

Loading

elliott-beach commented Feb 4, 2018 •

edited

Loading

DonJayamanne commented Feb 4, 2018

DonJayamanne Feb 4, 2018

elliott-beach Feb 4, 2018 •

edited

Loading

DonJayamanne Feb 4, 2018

DonJayamanne Feb 4, 2018

codecov bot commented Feb 4, 2018 •

edited

Loading

elliott-beach Feb 4, 2018 •

edited

Loading

DonJayamanne commented Feb 4, 2018

elliott-beach commented Feb 4, 2018 •

edited

Loading

elliott-beach commented Feb 4, 2018

Fix unhandled promise rejections #702

Fix unhandled promise rejections #702

Conversation

elliott-beach commented Feb 4, 2018 • edited Loading

elliott-beach Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

DonJayamanne Feb 4, 2018

Choose a reason for hiding this comment

elliott-beach Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

elliott-beach Feb 4, 2018

Choose a reason for hiding this comment

DonJayamanne Feb 4, 2018

Choose a reason for hiding this comment

DonJayamanne commented Feb 4, 2018

DonJayamanne commented Feb 4, 2018

elliott-beach commented Feb 4, 2018 • edited Loading

DonJayamanne commented Feb 4, 2018 • edited Loading

elliott-beach commented Feb 4, 2018 • edited Loading

DonJayamanne commented Feb 4, 2018

DonJayamanne Feb 4, 2018

Choose a reason for hiding this comment

elliott-beach Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

DonJayamanne Feb 4, 2018

Choose a reason for hiding this comment

DonJayamanne Feb 4, 2018

Choose a reason for hiding this comment

codecov bot commented Feb 4, 2018 • edited Loading

Codecov Report

elliott-beach Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

DonJayamanne commented Feb 4, 2018

elliott-beach commented Feb 4, 2018 • edited Loading

elliott-beach commented Feb 4, 2018

elliott-beach commented Feb 4, 2018 •

edited

Loading

elliott-beach Feb 4, 2018 •

edited

Loading

elliott-beach Feb 4, 2018 •

edited

Loading

elliott-beach commented Feb 4, 2018 •

edited

Loading

DonJayamanne commented Feb 4, 2018 •

edited

Loading

elliott-beach commented Feb 4, 2018 •

edited

Loading

elliott-beach Feb 4, 2018 •

edited

Loading

codecov bot commented Feb 4, 2018 •

edited

Loading

elliott-beach Feb 4, 2018 •

edited

Loading

elliott-beach commented Feb 4, 2018 •

edited

Loading