Use last defined withArgs match when multiple conditions are met

[jishi]

This hopefully fixes #1053

[coveralls]

Coverage increased (+0.004%) to 96.303% when pulling 69e2c2c on jishi:master into 35fe7d8 on sinonjs:master.

[mroderick]

I agree with the changes in this file, but they add noise. In the future, someone is going to land on this commit using git bisect, and the clearer each commit is, the faster they can get on with their day.

Would you mind splitting this commit, and put the white-space only changes in this file into a separate commit?

[jishi]

I splitted the linting fix into individual commit as suggested.

[mroderick]

👍

[coveralls]

Coverage increased (+0.004%) to 96.303% when pulling 5c79afd on jishi:master into 35fe7d8 on sinonjs:master.

[mroderick]

We don't yet have style and compatibility guidelines for the effort of creating v2.x. I'll try to get that discussion going in a separate thread.

For this PR, I would suggest using Array.prototype.filter instead of a while loop. filter is defined in ES5.1, and we're already using forEach, so we have committed ourselves it ES5.1 by now.

function matchingFake(fakes, args, strict) {
    if (!fakes || fakes.length === 0) {
        return undefined;
    }

    var matchingFakes = fakes.filter(function (f) {
        return f.matches(args, strict);
    });

    return matchingFakes.pop() || undefined;
}

I am aware that the solution iterates over ALL the fakes ... but there's not likely to be so many that we can actually measure the difference under anything other than truly extraordinary circumstances.

I think this refactoring of your changes works (all the tests pass). What do you think?

[jishi]

Yeah, the filter is probably more intuitive, I wasn't aware which target we we're aiming for so I stuck with a known approach since I knew it supposed to run in a browser.

The last || undefined seems unecessary though since pop() returns undefined by default.

Do you want me to make those changes and update the PR?

[mroderick]

You're right, || undefined is not necessary.

It'd be great to get the changes in, thanks!

[mroderick]

Try to avoid using literal values for tests, but label them using function names and variable names. This removes a lot of confusion situations and questions, like "is this 2 value the same 2 value as above?".

I realise that it's not practiced throughout the Sinon.JS codebase much yet, it's something I am hoping to change, so that it'll be easier for people to do more casual contributions.

Using v4 UUID values for return values, where we don't care about the exact value has the benefit of making it trivial to search for them in a large codebase, unlike values like 2, "username", etc.

I've tried applying the advice to the first test.

it("ensure stub uses last matching arguments", function () {
    var unmatchedValue = "d3ada6a0-8dac-4136-956d-033b5f23eadf";
    var firstMatchedValue = "68128619-a639-4b32-a4a0-6519165bf301";
    var secondMatchedValue = "4ac2dc8f-3f3f-4648-9838-a2825fd94c9a";
    var expectedArgument = "3e1ed1ec-c377-4432-a33e-3c937f1406d1";

    var stub = createStub().returns(unmatchedValue);

    stub.withArgs(expectedArgument).returns(firstMatchedValue);
    stub.withArgs(expectedArgument).returns(secondMatchedValue);

     assert.equals(stub(), unmatchedValue);
     assert.equals(stub(expectedArgument), secondMatchedValue);
});

What do you think?

[jishi]

makes sense to me.

[mroderick]

👍

[mroderick]

Thank you for your pull request! I just have a few of minor comments

[coveralls]

Coverage increased (+0.006%) to 96.305% when pulling aa6f3b6 on jishi:master into 35fe7d8 on sinonjs:master.

[mroderick]

Good stuff, thanks! 👍

[BitForger]

Came across this trying to figure out why my chained .withArgs statements were always returning the last one. I'm using sinon/mocha with typescript and have a model that i'm trying to mock with static methods. The problem is i need to switch the value returned based on the arguments passed.

public static findOne = sinon.stub()
    .withArgs({slug: 'test'}).returns({
      exec: sinon.stub().resolves(new MockFeature()),
    });

How would I do that, seeing that it's been changed in this PR?

[fatso83]

Not sure I understand the problem. In any case, this is a usage problem. could you please post this to the sinon tag on the Stackoverflow forum? Lots more people following that than this issue tracker