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

Added a flag (-R, --rejections) to make the test fail if an unhandled Promise rejection happened during its execution #658

Merged
merged 4 commits into from Nov 7, 2016

Conversation

@DanReyLop
Copy link
Contributor

DanReyLop commented Nov 4, 2016

Fixes #649

I'm open to suggestions, specially about naming.

All the relevant discussion is on the #649 issue. To sum up, I've added a flag to Lab, called --rejections (alias -R). If that flag is set, and an unhandled Promise rejection happens during a test, that test will be failed.

Without this flag, the behaviour will be exactly the same as before: The test pass, and in versions of Node.JS 6.6.0+, a warning is printed on the console (but the test still passes).

An unhandled Promise rejection looks like this:

function foo() {
    functionThatReturnsPromise()
        .then((res) =>{
            throw new Error('This error will be absolutely swallowed.');
        })
        // No .catch() block here
}

Initially, the behaviour of unhandled rejections was to be silenced and totally ignored, by design. It looks like everyone is changing their minds, because in the latest versions of Chrome and Node.JS (maybe more engines, I don't know more examples) the unhandled rejections have started to display console warnings. In 99% an unhandled rejection means that something is terribly bad with the code, and the fact that doesn't crash the entire process is just a questionable design choice. So I think it should definitely make a test fail.

@DanReyLop

This comment has been minimized.

Copy link
Contributor Author

DanReyLop commented Nov 4, 2016

Ouch. I've added tests to cover this feature in test/cli.js, but the coverage isn't counting those tests (I assume because they spawn child processes). I haven't realized it because as a Windows user I couldn't get npm test to pass 😄

Do you have any suggestion on how to handle this? I thought about adding tests to test/runner.js directly, but since what's being tested here is a CLI flag, it made more sense to test it in test/cli.js

test/cli.js Outdated

RunCli(['test/cli_reject_promise/reject_promise.js', '-R'], (error, result) => {

if (error) {

This comment has been minimized.

Copy link
@geek

geek Nov 5, 2016

Member

instead:

expect(error).to.not.exist();
@geek

This comment has been minimized.

Copy link
Member

geek commented Nov 5, 2016

@DanReyLop The naming looks good. For improving the code coverage, you are likely going to need to emit the rejection event manually on the process, and perhaps monkey-patch other areas.

@DanReyLop

This comment has been minimized.

Copy link
Contributor Author

DanReyLop commented Nov 6, 2016

@geek I've ended up adding a test to tests/runner.js to cover the new code and actually make it count for the coverage. Coverage is at 100% again 😄

"posttest": "npm run lint-md",
"test-cov-html": "./bin/_lab -fL -r html -m 3000 -o coverage.html",
"lint": "./bin/_lab -d -f -L",
"test-cov-html": "node ./bin/_lab -fL -r html -m 3000 -o coverage.html",

This comment has been minimized.

Copy link
@geek

geek Nov 6, 2016

Member

Is there a reason why this needed to change?

This comment has been minimized.

Copy link
@DanReyLop

DanReyLop Nov 7, 2016

Author Contributor

Not directly related to this PR, but the previous command didn't work on Windows, because the #! /bin/node trick obviously doesn't work on Windows.

Some tests keep failing on my Windows 10 machine, but with this change at least I can run them.

@geek geek added the feature label Nov 6, 2016
@geek geek added this to the 11.2.0 milestone Nov 6, 2016
@geek geek self-assigned this Nov 6, 2016
@geek geek merged commit f1094c9 into hapijs:master Nov 7, 2016
2 checks passed
2 checks passed
Node Security No known vulnerabilities found
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.