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

Make the test fail on a Promise unhandled rejection #649

Closed
DanReyLop opened this issue Oct 19, 2016 · 6 comments
Assignees
Labels
Milestone

Comments

@DanReyLop
Copy link
Contributor

@DanReyLop DanReyLop commented Oct 19, 2016

We've recently migrated from NodeJS 4.x.x to 6.7.0 and we found that in some cases our Promise-based code was failing silently. In recent versions of NodeJS, when a Promise is rejected but it doesn't have a .catch() handler, a warning is emitted in the console. But that warning was the only way to notice it, and it was easy to overlook, because the tests didn't fail.

I propose that lab subscribes to the process.on('unhandledRejection') event (documented here, available since NodeJS 1.4.1) and make the test fail if any Promise was rejected and not handled.

Even though what I propose is a breaking change, I think it's worth going that way. 99% of time, if your code under test has a Promise being rejected and fail silently, there's something wrong with that code that ought to be fixed.

Any opinions on this? I'll be happy to make a PR, but I want to have an idea beforehand that the solution isn't gonna be rejected completely :)

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Oct 20, 2016

@DanReyLop I like the idea, but I don't want to break applications that already have an unhandledRejection handler. Instead, it would be nice if the warning was backported in Node.js itself, but that's not likely to happen.

An alternative is for anyone using promises to add

process.on('unhandledRejection', (reason, promise) => {
  console.error(reason);
  throw reason;
});

Honestly, my bigger concern is that once we add this change people will start opening issues in lab saying that it broke their promises since we will be throwing when people don't handle rejections.

@DanReyLop

This comment has been minimized.

Copy link
Contributor Author

@DanReyLop DanReyLop commented Oct 21, 2016

I like the idea, but I don't want to break applications that already have an unhandledRejection handler.

Good point. I've yet to see any of those applications in the wild, but they may exist :)
We could check, when the unhandledRejection event is fired, there is another handler apart from lab's, and don't fail the test in that case. I think this is starting to sound more complex than it should...

Instead, it would be nice if the warning was backported in Node.js itself, but that's not likely to happen.

The problem with the warning is that it's very easy to miss. In a lab run, you normally see a fast scrolling list of checkmarks and, at the end, either it passes or it fails. If the test suite doesn't fail, very few people would scroll back to find some warnings.

Honestly, my bigger concern is that once we add this change people will start opening issues in lab saying that it broke their promises since we will be throwing when people don't handle rejections.

I understand that this, if implemented, would be a major semver change. That and clearly stating that the error is caused by an unhandled promise may be enough.

An alternative is for anyone using promises to add [...]

I thought about something like that, but it would need to be included (directly or through a require) in every single test file. That's easy to forget. We can do this for our project (maybe encapsulate it in a file like configure-lab.js), but it doesn't change the fact that it would be better if it was baked-in in the test runner :)

@blacksun1

This comment has been minimized.

Copy link
Contributor

@blacksun1 blacksun1 commented Oct 23, 2016

What about a command line flag to enable or disable this functionality? Or an option on an experiment or test?

By default it could be turned off. If this was the case then we would skip the needless requirement of a major semver as it would only be turned on when the user has specifically requested it, it would not cause issues from users saying that it broke their tests.

@blacksun1

This comment has been minimized.

Copy link
Contributor

@blacksun1 blacksun1 commented Oct 23, 2016

By the way, it sounds like a great idea to me.

@DanReyLop

This comment has been minimized.

Copy link
Contributor Author

@DanReyLop DanReyLop commented Oct 24, 2016

What about a command line flag to enable or disable this functionality? [...] By default it could be turned off.

Sound reasonable. An option per experiment/test kinda defeats the purpose of this, but a global flag seems like the best approach to not break anything and still be useful.

I'll hack something together on the next few days :)

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Oct 24, 2016

@DanReyLop @blacksun1 a global flag sounds good to me, I am definitely happy to add that change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.