support .withArgs() on expect(fn) #756

Open
blockloop opened this Issue Jul 22, 2016 · 13 comments

Projects

None yet

4 participants

@blockloop

expect.js has a feature to pass arguments to a function using the following syntax

expect(fn).withArgs(some, args).to.throw();

Right now I'm having to write the following

expect(() => fn(some, args)).to.throw();
@lucasfcosta
Member

Hi @blockloop, thanks for your issue!

That makes total sense to me and looks like a relevant feature (I'm not sure we can call that a "feature" since it may be implemented just as syntax sugar, but let's keep going). Its syntax is clear and concise enough for me.

I'm just wondering how we would implement that on the should interface if everyone agreed upon this feature. Maybe we could change the order of the assertion words, for example:

fn.should.throw.withArgs(some, args);

What do you guys think?
cc @meeber @keithamus

@keithamus
Member
keithamus commented Jul 22, 2016 edited
fn.should.throw.with.args(some, args);

Would probably fit more with the rest of the code. Having args as the assertion actually makes for some interesting developments:

addNumbers.should
  .throw.with.args(1, 'a')
  .and.args('a', 2)
  .but.not.throw.with.args(1, 2)

It becomes challenging though - because we can't assert on the error that is being thrown in a sensible way:

addNumbers.should.throw/* a typeerror, but I can't assert this */.with.args(1, 'a')

Whereas if the args was in the chain, then we can:

addNumbers.should.with.args(1, 'a').throw(TypeError)
@lucasfcosta
Member
lucasfcosta commented Jul 22, 2016 edited

@keithamus Whoa!
That's what I call heavy code ninjitsu. That looks awesome.
It's like writing in plain english and having your computer to understand it.
I totally agree with this proposal.

@keithamus
Member

Which one? .throw.with.args() or .with.args().throw()?

@keithamus
Member

We could also have args() propagate the error like throw() does, which loses flexibility on the first example I had, but you get to assert on the error which is probably more preferable:

addNumbers.should
  .throw.with.args(1, 'a')
  .and.be.an.instanceof(TypeError)
@lucasfcosta
Member
lucasfcosta commented Jul 22, 2016 edited

Oh I have seen a different version of this, probably before you edited it.
Anyway, considering your new ideas I guess that having args to propagate the error seems more useful, however it is not as semantically correct as this:

addNumbers.should
  .throw.with.args(1, 'a')
  .and.args('a', 2)
  .but.not.throw.with.args(1, 2)

If I was a user I would not expect args() to change the assertion's subject, even if it is useful it doesn't seem to be much obvious.

@keithamus
Member

Yeah sorry, was attempting to ninja edit my comment as I realised it was more complex than I thought.

I agree with your comment, that's our challenge - finding a good balance between useful and expected. My guess is that users will want to more precisely specify why those calls were throwing - and so having .args() change the subject to the error is useful, even if it makes less sense.

We should wait for more people to chime in, like @blockloop and @meeber

@blockloop

I agree with the verbiage. Maybe it's just a style thing but I don't like this

addNumbers.should
  .throw.with.args(1, 'a')
  .and.args('a', 2)
  .but.not.throw.with.args(1, 2)

Not because of the verbiage but because it's multiple assertions in one statement.

@keithamus
Member

Not because of the verbiage but because it's multiple assertions in one statement.

Yeah, most of our chained assertions just drill down into the same assertion, e.g. expect(fn).to.throw().and.be.an('error').that.has.property('message').that.matches(/foo/), whereas the chaining of multiple args would be multiple individual assertions.

@blockloop

What about this

fn.calledWith(1, 'a').should.throw()
@lucasfcosta
Member

@blockloop IMO that is not a viable solution because it adds another property to the Object.prototype.
I think that having should is already enough. If we did that it would be like a new Assertion interface.

@blockloop

True. That's actually one of the reasons I prefer the expect() interface :). That being said, addNumbers.should.throw.with.args(1, 'a') appears to be the most semantic for the should interface.

@meeber
Contributor
meeber commented Jul 22, 2016 edited

This is a great feature request. With proper documentation, it should help cut down on instances of this problem.

I prefer styles that allow the arguments to be positioned as close as possible to the function, instead of having them be separated by the throw keyword.

An alternative or alias to args in that spirit:

expect(myFn).given(arg1, arg2).to.throw(TypeError);
myFn.should.given(arg1, arg2).throw(TypeError);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment