Implemented new semantics for the 'was called with' assertion.

[papandreou]

• Require an array to be passed instead of supporting varargs. In my opinion that is much clearer. Also, it gets rid of this annoying edge zero params case: expect(spy, 'was called with');
• Removed the 'exactly' flag -- always require an exact match. Since expect.it is supported, there are better ways of asserting only on a prefix of the argument list. If it turns out there's a clear enough use case, we can consider implementing a was called with initial, or we can add support for <spy> was [always] called with <object> so you can specify exactly which arguments via numerical properties.

This is, of course, a very breaking change, but I think we should rip off the band-aid and do a major version bump. Most test suites that rely on the old convention will be caught by the type system.

[papandreou]

The coverage % is only decreasing because covered code is being removed.

[sunesimonsen]

Ohhhh noooo :-) I'm sorry that I'm so forgetful, but I don't remember that really hard argument for using an array over using varargs. From a users perspective I think varargs feel more natural, even if the implementation if more hacky. But that is just a matter of opinion on preferred style. I don't think this kind of breakage is worth it, just to change the coding style. Is it because we want to pave the way for something bigger? like <function> was called with <assertion>?

I can see that using an array would be preferable, if you build up the arguments programmatically. But I think most usage right now is just hardcoded.

This would be a very hurtful change - if we do it, we should probably ask the community their opinion. This is one of the first really breaking changes to enduser facing code.

Don't get depressed, I'm just trying to preserve stability.

[papandreou]

I knew this one wouldn't go down easily, I just took a shot anyway because we identified some problems with the current semantics in this old discussion: https://gitter.im/unexpectedjs/unexpected/archives/2015/11/26

Especially the fact that was called with only matches a prefix of the arguments unless you specify the exactly flag. It has surprised me a bunch of times and I think that in itself is something we need to fix so it's strict by default, and that's also a breaking change.

My aversion against varargs in this case is subjective to some extent, but it does give us the chance to extend the assertion to support things like these later:

expect(spy, 'was called with', {1: 'foobar'});
expect(spy, 'was called with', function (args) {
    expect(args[0], 'to equal', 'foo');
});

Also, the "was called with no arguments" case is ugly with varargs:

expect(spy, 'was called with exactly');

To me all that added up wanting to break backwards compatibility for those 3 users we currently have so that we can stop worrying about the weird semantics.

[sunesimonsen]

I agree that we should be strict by default, and that would be breaking.

This was the kind of argument I was hoping for:

expect(spy, 'was called with', {1: 'foobar'});
expect(spy, 'was called with', function (args) {
    expect(args[0], 'to equal', 'foo');
});

That would be neat. We need another release cycle to introduce that functionality, so the breakage will be clearer for the removal of the varargs.

What I'm afraid of is that we have more than 3 users :-) But okay let's roll with this, but we should make it really clear that we are breaking things on Gitter, Twitter, and the changelog.

I'm such a pushover ;-)

[sunesimonsen]

We need a changelog for this change with a big warning. 👍

[papandreou]

Thanks for reconsidering it. And yes, we have to shout it from the roofs.

I'd like to be sure that the new semantics are sane -- it would be pretty bad if we had to make further adjustments to it because it isn't thought through.

Some open questions for me:

• Is it OK that was called with uses to satisfy semantics without having "satisfy" or "satisfying" in its name?
• Do we need to reintroduce an assertion that matches a prefix of the arguments, ie. the old non-exactly variant of was called with. If so, it should of course have a better name so it's obvious what it does. (I'm hoping we won't need it)

// cc @gertsonderby @joelmukuthu @gustavnikolaj @Munter

[bruderstein]

Hi, very regular user of unexpected-sinon here (with lots of uses of was called with)

For the record:
We use was called with explicitly to mean to have at least one call with the following arguments. There are many calls, but we only care that it was called at least once with these arguments.

I'm a bit confused about the change being suggested now, so I'll summarise my understanding to ensure I've not misunderstood something.

Point 1

The current suggestion is to stick with varargs, but not allow prefix arguments, so if the call was spy(123, 'abc', 5), expect(spy, 'was called with', 123, 'abc') would fail now, where it would pass before.

I think this is fine (I don't actually think we've ever used it like that anyway). Can understand some people might though.

We could even add an extra check - if expected args are a prefix of the actual args, add a note on the diff that the behaviour has changed.

(Just to through into the mix, a change to an array would be doable - I'd write a codemod to change the calls if we went that way, and make that available. Generally though, I'd be against changing the syntax for 'was called with')

Point 2

The two suggestions from @papandreou above

expect(spy, 'was called with', {1: 'foobar'});
expect(spy, 'was called with', function (args) {
    expect(args[0], 'to equal', 'foo');
});

These wouldn't be supported under the varargs scheme, would they? I've been bitten by unexpected calling a function that I'm passing as an expected value before, and it confused me for a while.

What's to stop a new assertion using an array, or object with index keys, or a function as above?

expect(spy, 'was called with', expect.it('to be a string'), 'foobar');
expect(spy, 'was called with arguments satisfying', [ expect.it('to be a string'), 'foobar'])
expect(spy, 'was called with arguments satisfying', function (args) {
    expect(args[0], 'to equal', 'foo');
});
expect(spy, 'was called with arguments satisfying', { 1: 'foobar' });

[papandreou]

@bruderstein, thanks for weighing in, I would have cced you if I knew you were a user.

The current suggestion is to stick with varargs, but not allow prefix arguments, so if the call was spy(123, 'abc', 5), expect(spy, 'was called with', 123, 'abc') would fail now, where it would pass before.

No, what I'm suggesting here (and have implemented) is to drop varargs and require the expected arguments to be specified as an array so it becomes:

spy(123, 'abc', 5);
expect(spy, 'was called with', [ 123, 'abc', 5 ]);

Passing a non-array as the third parameter would cause a "missing assertion" error -- although at a later point we can also entertain the idea of implementing <spy> was [always] called with <object> which could work like regular <array-like> to satisfy <object> for the partial argument match case.

I guess the confusion comes from the fact that I want to change two things at once. You're right that just fixing the really surprising partial-match-by-default thing and sticking with varargs would be less invasive as most users probably specify all the params anyway. And adding such a notice to the error message would offer a nice way to let users know "in context" what's going on -- great idea.

Introducing a was called with arguments satisfying assertion that takes an array/object/etc. is also a good option. That would allow us to implement all the desired features at once instead of waiting a while to introduce <spy> was called with <object> because it might lead to obscure results for users that haven't "migrated" to the new semantics yet. A name of was called with arguments satisfying is also nicer per the convention that assertions that use to satisfy semantics should have some conjugation of the word "satisfy" in their name. And currently was called with breaks that rule, which is also a consistency problem that my proposal doesn't fix. For one thing, spy({foo: 123}); expect(spy, 'was called with', {}) currently passes because of the non-exhaustive to satisfy semantics, and that's also a bit of a footgun.

How about this instead:

• Add <spy> to have call [exhaustively] satisfying <array|object|function>
• Remove <spy> was [always] called with [exactly] <any*> (or change it to always fail with a notice that you should switch to to have calls satisfying)

[papandreou]

// cc @alexjeffburke -- just in case you're also using unexpected-sinon

[bruderstein]

Thanks for the extra explanation, sorry for the confusion - I thought the direction had changed mid-issue.

I think just changing the syntax to require an array could irritate people.

spy([123, 456, 789]);

expect(spy, 'was called with', [123, 456, 789]);
// expected spy was called with [123, 456, 789] but was called with [[123, 456, 789]]

If I haven't seen this issue/announcement etc, I'm now confused and irritated.

I really like your last suggestion:

Add to have call [exhaustively] satisfying <array|object|function>
Remove was [always] called with [exactly] <any*> (or change it to always fail with a notice that you should switch to to have calls satisfying)

And yes, change it (at least until the next major) to fail with a message stating the new assertion.

That still means changing tests, but is at least explicit, in that I get a message telling me what to do. I'd write a codemod for this, and if we could host a docs with a description of the change, and provide a link to the codemod, then I think we're making the change responsibly.

[bruderstein]

Ah, just had another thought on this...

We already have 'to have calls satisfying', which takes an array of objects with args properties, one object for each call. To add an assertion that takes a different format, but is only different in the calls vs call might cause confusion.

I still like the idea of having a new assertion so nobody gets weird errors, just not sure that to have calls satisfying and to have call satisfying should behave in wildly different ways.

Maybe this is bikeshedding too much, but how about adding an a - to have a call satisfying

expect(spy, 'to have a call satisfying', ['foo', 123])

expect(otherSpy, 'to have calls satisfying', [ { args: ['foo', 123] }, { args: ['bar', 234] }])

As an alternative, and I accept this is longer, but we could make 'to have [a] call satisfying' work in the same way as 'to have calls satisfiying', meaning an object with args, and optionally supporting the returned property, which is currently only possible with the multi-calls variant.

expect(spy, 'to have a call satisfying', { args: ['foo', 123], returned: 42 })

expect(otherSpy, 'to have calls satisfying', [ { args: ['foo', 123] }, { args: ['bar', 234] }])

An array as parameter to 'to have a call satisfying' could be a shorthand form for { args: [...] }, but at least then they both accept the same formats (especially if 'to have calls satisfying' was extended to allow an array in place of { args: [...] }.

[alexjeffburke]

See, I'm not completely sure about removing the varargs - but for a bit of context, I struggled with naming and behaviour of a very similar interface when doing -events.

The thing about array is it is synonymous with more than one. However, there are two different things you have here:

1. arguments to an existing call
2. arguments to multiple calls

The more I think about it, the more I am tempted to reserve arrays for the multiple individual calls. That'd mean arrays of arrays yes - but I like the fact you won't be confused what you're asserting. Then arguments to a single call could be specified using varargs, somehow like that.

Now, I kinda like the suggestion instead of having arrays of arrays you have an array of { args: [] }. For me that little extra verbosity is actually a win in test land if it makes what these things are crystal clear - I don't wanna be confused by the tests of a piece of compiles functionality.

Other random thoughts for things on the RHS of these expressions:
, { calls: [...] }
That's the removing ambiguity thing again.

Hope that's helpful (:

[sunesimonsen]

@papandreou maybe we should only change the prefix behaviour of was called with and require at least one argument. Then introduce the following assertions:

expect(spy, 'to have call satisfying', { args: ['foo', 123] })

If you want to satisfy against the arguments there is this option:

expect(spy, 'to have call satisfying', function (spy) {
  spy('foo', 123);
});

and one for asserting that there is should be no arguments:

expect(spy, 'was called with no args')

The problem about satisfying against an empty object seems to be a general one. That should probably always use to equal semantics for that case, but that is another store.

[papandreou]

@bruderstein Thanks for the thorough response. You're nowhere near bikeshedding in my book. I really want us to get this right!

I think you're right about the confusion and irritation if we change the existing was called with to require an array. Let's take that off the table. I've also noted that I seem to be the one in this conversation that dislikes varargs, so I'm withdrawing that as well, except for when the assertion has arguments satisfying in its name so that users will naturally assume that they have to pass an array or an object with the spec.

I agree about to have a call satisfying. It's more explicit than to have call satisfying and harder to type by accident. Since we can easily introduce that in a minor release, let's branch that discussion out. I implemented what seems to be the consensus here, including @sunesimonsen's ideas: #31

As for was called with we still need to work out:

1. whether to deprecate or maybe even remove it
2. if it stays, whether to force the always mode to remove the element of surprise
3. if it stays, how to cope with the fact that it uses to satisfy semantics for the individual arguments without advertising it in the assertion name

My vote is on 1)

[papandreou]

Echoing @sunesimonsen's comment on #31:

Maybe we should just not touch was called with at all and write a deprecation notice in the documentation.

That would work for me. How's that with everyone else? I'm thinking the plan could be something like:

• add deprecation notice to was called with
• feature to have a call satisfying on the front page and replace
• release a new major (or at least minor) version

And then after a while:

• remove was called with from the documentation
• release a new major version

And then even later:

• remove the was called with assertion
• release a new major version

@bruderstein, do you feel like transitioning to to have a call satisfying for all your use cases?

[sunesimonsen]

That sound like a plan, but instead of just removing was called with when the time comes, we should throw a nice descriptive error pointing the users in the right direction.

[bruderstein]

@papandreou I really like how this turned out too. And yes, I'll
transition. I'll probably make codemod for it and post it somewhere, might
be useful for others.

I think as the last step, rather than remove it, it should just fail with a
pointer to the docs.

Great stuff 🎉

On Tue, 26 Jan 2016 17:38 Sune Simonsen notifications@github.com wrote:

That sound like a plan, but instead of just removing was called with when
the time comes, we should throw a nice descriptive error pointing the users
in the right direction.

—
Reply to this email directly or view it on GitHub
#29 (comment)
.

[papandreou]

Released 10.1.0 with to have a call satisfying etc. and the first 3 steps of the checklist.

Let's keep track of the remaining steps in a new issue: #32