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

Fail test if a falsy value is thrown by the test #813

Merged
merged 1 commit into from Feb 25, 2018

Conversation

@rankida
Copy link
Contributor

rankida commented Feb 22, 2018

This fixes #812 awaiting a rejected promise can still pass test. All the info can be found here.

@@ -578,7 +578,7 @@ internals.protect = function (item, state) {
finish();
}
catch (ex) {
return finish(ex);
return finish(ex || 'reject');

This comment has been minimized.

Copy link
@Marsup

Marsup Feb 22, 2018

Member

Shouldn't it be a new Error() ?

This comment has been minimized.

Copy link
@rankida

rankida Feb 22, 2018

Author Contributor

If it is an Error then it will flow the full way through and report an empty string as the error, whereas setting it as a string results in the standard Non Error object received or caught message.

I am happy enough with either (my priority is that the test fail) so really depends on what feedback makes most sense

with 'reject'

Failed tests:

  27) Executor jobs throw:

      Non Error object received or caught

      at finish...

with new Error()

   27) Executor jobs throw:



      at Immediate.setImmediate...

This comment has been minimized.

Copy link
@geek

geek Feb 22, 2018

Member

This seems reasonable to me, @Marsup any objections to the current implementation?

This comment has been minimized.

Copy link
@Marsup

Marsup Feb 22, 2018

Member

OK, didn't investigate enough to tell, I was just surprised that it could end with an error or a string. If you're happy I'm happy.

This comment has been minimized.

Copy link
@lerouxb

lerouxb Feb 23, 2018

What's wrong with new Error('Empty promise rejection.')?

This comment has been minimized.

Copy link
@rankida

rankida Feb 23, 2018

Author Contributor

As I said above, the "problem" with using an genuine Error is that you follow the same path as any errors that come from the test.

So you could see the error message 'Empty promise rejection.' and think your test was actually throwing that.

The reason I chose a non-Error value is that I think the existing message Non Error object received or caught is the most appropriate and fits in nicely with the existing behaviour.

That said, as long as it is set to something I do not care too much. My priority is that it fails the test.

This comment has been minimized.

Copy link
@rankida

rankida Feb 23, 2018

Author Contributor

@geek if you want me to change anything just let me know

@geek geek added the bug label Feb 22, 2018
@ORESoftware

This comment has been minimized.

Copy link

ORESoftware commented Feb 22, 2018

maybe dis better?

return finish(ex || 'dummy hapijs reject');

or

return finish(ex || new Error('dummy hapijs reject'));
@rankida

This comment has been minimized.

Copy link
Contributor Author

rankida commented Feb 23, 2018

@ORESoftware thanks for your comment.

The string is used only internally to lab so I don't think it matters too much. If it does need changed I would prefer something that does not mention hapi as you might think this si something coming from the webserver.

I have addressed the comments about using Error in the thread above

@geek geek added this to the 15.3.0 milestone Feb 25, 2018
@geek geek merged commit 35cc159 into hapijs:master Feb 25, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rankida rankida deleted the rankida:falsy-throws branch Feb 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.