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

Alias 'to throw' as 'to throw ... satisfying' #535

Open
joelmukuthu opened this issue Dec 2, 2018 · 10 comments
Open

Alias 'to throw' as 'to throw ... satisfying' #535

joelmukuthu opened this issue Dec 2, 2018 · 10 comments

Comments

@joelmukuthu
Copy link
Member

No description provided.

@alexjeffburke
Copy link
Member

Aliases for "to throw" are already supported - see the documentation for the assertion here.

The word "satisfying" is not something defined for "to throw", but if we added it that wouldn't really be an alias but an entirely different assertion. However, you can already achieve exactly the semantics you want by using expect.it, for example:

expect(() => { throw new Error() }, 'to throw', expect.it('to have property', 'message'));
expect(() => { throw new Error() }, 'to throw error', expect.it('to have property', 'message'));
expect(() => { throw new Error() }, 'to throw exception', expect.it('to have property', 'message'));

@papandreou
Copy link
Member

The to throw... assertion already uses "to satisfy" semantics, though? In the light of that I think it would make sense to add the ...satisfying variant as an alias?

@alexjeffburke
Copy link
Member

My apologies, I think I misread the value type to mean it wouldn't compare the error using satisfy semantics - the assertion does something a little special for strings I believe (comparing the message against it) hence the expect.it suggestion.

@joelmukuthu joelmukuthu changed the title Alias 'to throw' as 'to throw (error|exception...etc) satisfying' Alias 'to throw' as 'to throw ... satisfying' Dec 3, 2018
@joelmukuthu
Copy link
Member Author

The issue title was also a bit confusing, I've updated it to make it a bit clearer. While I have your attention, what does <function> to throw (a|an) <function> do :)?

unexpected/lib/assertions.js

Lines 1030 to 1044 in 9b30831

expect.addAssertion(
'<function> to throw (a|an) <function>',
(expect, subject, value) => {
const constructorName = utils.getFunctionName(value);
if (constructorName) {
expect.argsOutput[0] = output => {
output.jsFunctionName(constructorName);
};
}
expect.errorMode = 'nested';
return expect(subject, 'to throw').then(error => {
expect(error, 'to be a', value);
});
}
);

@alexjeffburke
Copy link
Member

I've been thinking about this some more - I can definitely see the value in this alias but I'd like to raise one concern.

Using satisfying here would introduce something that behaves a little differently than for example "to have an item satisfying" primarily because of the matching of the error message to a string of its supplied. That would make this use of "satisfying" a little different to all the others. My concern is also that since (unfortunately) strings can be thrown at the language level this would create an annoying ambibugity. I might be over losing this though so keen to hear thoughts.

@joelmukuthu thanks for expanding on the description a bit :)

@joelmukuthu
Copy link
Member Author

@alexjeffburke I agree, but I've also always thought of to satisfy as an assertion that allows you to assert against different types which to me makes this an acceptable ambiguity aka "it's a feature" :D. Actually I think to throw error satisfying <string> reads better than to throw error <string>

@papandreou
Copy link
Member

I agree about to throw error satisfying <string> reading better. And to throw error satisfying <object> even more so!

<function> to throw (a|an) <function> checks that the thrown value is an instance of a particular class, eg.:

expect(myFunction, 'to throw an', httpErrors.Conflict);

@alexjeffburke
Copy link
Member

Agreed about that spelling for which I think means we've reached some broad agreement - I can have a look next time I'm near core.

@papandreou
Copy link
Member

the assertion does something a little special for strings I believe (comparing the message against it)

You're right, that's a bit of a wart in itself, but I think we can fix that so it reduces directly to <UnexpectedError> to satisfy...: #538.

@papandreou
Copy link
Member

@joelmukuthu, personally I'd welcome a PR for this now that the other oddity got resolved.

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

No branches or pull requests

4 participants