Matching multiple arguments at once

[reimund]

These two commits makes it possible to match multiple arguments with a single matcher.

The first commit makes the following scenario work:

var obj = { foo: function() { }};
var ex = sinon.mock(obj).expects('foo');

obj.foo('bar', 'abc', 123);

sinon.assert.calledWith(ex, sinon.match(function(arg1, arg2, arg3) {
    return 'bar' == arg1
            && 'abc' == arg2
            && 123 == arg3;
}));

The second commit makes this, slighly less verbose, example work:

var obj = { foo: function() { }};
var mock = sinon.mock(obj);

mock.expects('foo').once()
    .withArgs(sinon.match.all(function(arg1, arg2, arg3) {
            return 'bar' == arg1 && 'abc' == arg2 && 123 == arg3;
    }));

obj.foo('bar', 'abc', 123);

[mantoni]

sinon.match(function (val) {}) creates a custom matcher. Your commit overrides that behavior.

[reimund]

Care to elaborate?

[mantoni]

Ah, sorry. Now I get it. You are overriding it only in the specific case of a single argument which also happens to be a matcher.

I'll have a closer look at your proposal later today.

[reimund]

Exactly.

To clarify further, the reason I added a new matcher (sinon.match.all) was to avoid changing behaviour of existing matchers (sinon.match.number, etc). Since sinon.match are used to create them, they would also receive all arguments had I used sinon.match instead of sinon.match.all (in case of a single argument).

Thinking about it some more, I'm not sure that would be a problem. What do you think?

[reimund]

Did you have a chance to look at it?

[mantoni]

Sorry. There is something I don't like about it, but I can't really say why.
I'd rather leave this one to @cjohansen.

[reimund]

You don't like the new feature, or the implementation?

If it's the latter I would be glad if you can improve my implementation. The important part here is to be able to make a custom matcher that gets passed all arguments, which is currently not possible.

[cjohansen]

@mantoni you know the matchers better than me. I think that "a custom matcher that gets passed all arguments" sounds like a reasonable request. What options do we have to solve this problem, and what do you see as the trade-offs, @mantoni ?

[mantoni]

Ok, letting it sink in a while did help in this case.

There are three things:

• I don't feel comfortable assessing the changes made in mock.js, because I know very little about it.
• the addition to the matchers is actually fine, but the name is not very descriptive. Maybe args?
• if we have this feature, wouldn't it make sense to make it a little more generic, so that any remaining arguments of a function call can be matched? Think calledWith('foo', 42, sinon.match.args(function (args) { ... })).

[mroderick]

This needs a rebase

[fearphage]

Should we swap out the yoda conditions? They aren't present anywhere else.

Those should be caught by ESLint also.

if we have this feature, wouldn't it make sense to make it a little more generic

One thing to think about, your suggested solution and the one in this PR don't allow for partial matches: match(first3Args), match(last3Args)

[fatso83]

This has lingered far too long. Let's get this out for Christmas 🎁

1. No one on the maintainer team has much interest in the mocks. I'd say let's get it merged if the test passes. On the way to Sinon 3 we will be splitting out the mocks into a separate project anyway.
2. I can do whatever rebasing needs to be done.
3. I'll rename all to args
4. We can do partial argument matching anyways using arguments (or rest parameters in ES2015).
5. I'll fix the Yoda statements and add an ESLint rule.

Not sure what @fearphage means with the partial matching example, as I don't understand the code snippet without context.

Anyone opposed?

[fearphage]

It doesn't seem like a way to not match all or the rest of the arguments. It's "greedy" in regular expression terms.

And to explain using code, there's no way to do this as I understand it:

calledWith(42, true, sinon.match.args(matchNext3Args), 'catpants', 'test');

The sample function call would match: fn(42, true, 'foo', 'bar', 'baz', 'catpants', 'test')

[fatso83]

@fearphage I practically never use matchers, so I feel a bit out on thin ice here, but that use case seems awfully narrow too me. Is partial matching something we feel is useful, and if so, is there a way of modifying this to support it? I have freed up some time and I am simply trying to find a way of moving this forward 🙂

[mroderick]

The original author has not participated in this PR for nearly three years. I am closing this