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

Finally swallows rejections #49

Closed
aiham opened this issue Jun 28, 2017 · 4 comments
Closed

Finally swallows rejections #49

aiham opened this issue Jun 28, 2017 · 4 comments
Labels

Comments

@aiham
Copy link

aiham commented Jun 28, 2017

If I call finally on a rejected promise it unexpectedly becomes fulfilled.

const Promise = require(process.argv[2]);

const p = new Promise((resolve, reject) => {
  setTimeout(() => reject(new Error('simulated error')));
});

p.finally(() => {
  console.log('Cleanup');
})
.then(result => {
  console.log('Unexpected success', result);
})
.catch(err => {
  console.error('Expected failure', err);
});

With yaku:

$ node index.js yaku
Cleanup
Unexpected success Error: simulated error

With bluebird:

$ node index.js bluebird
Cleanup
Expected failure Error: simulated error

Your finally test doesn't catch this because although you expect 'error' here: https://github.com/ysmood/yaku/blob/master/test/finally.js#L28

You do not confirm that its a rejection here: https://github.com/ysmood/yaku/blob/master/test/testSuit.js#L14

The cause is that you do not rethrow or wrap value with Promise.reject here:

return value;

@aiham
Copy link
Author

aiham commented Jun 28, 2017

I would submit a fix but I don't know how you want to update your custom test framework to also take into account the expected Promise result type.

@ysmood
Copy link
Owner

ysmood commented Jun 28, 2017

Thanks for your report. Sorry that I mistook how finally works.

try {
    try {
        throw 'err'
    } finally {
        console.log('ok')
    }
} catch (e) {
    console.log(e)
}

I thought the console.log(e) won't be executed, but I'm wrong. It's not catch. I'll fix it as soon as possible.

@ysmood ysmood closed this as completed in 7ee312d Jun 28, 2017
@ysmood
Copy link
Owner

ysmood commented Jun 28, 2017

v0.18.1 published. If there's any problem, I will reopen this issue.

@aiham
Copy link
Author

aiham commented Jun 28, 2017

@ysmood Thanks for the quick response!

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants