callCount property is incorrect

[kbtknr]

• Sinon version : 1.17.7
• Environment : node v6.9.2
• Example URL : None
• Other libraries you are using: mocha

What did you expect to happen?
I would expect that spy.callCount is number of times the target function was called.
And, spy.withArgs(arg1).callCount is number of times the target function was called with specific arguments.
But, the callCount property is often incorrect.
What actually happens
First of all, f(1) and f(1, 1) and f(1, 2) is calling.
As this time, spy.withArgs(1).callCount is 3,
But spy.withArgs(1, 1).callCount is often 0.

This may be affected by calling order spy.withArgs().
Following test code has 2 case pattern, and change the order in which withArgs(1) and withArgs(1, 1) are called.

How to reproduce

const sinon = require('sinon'); // [email protected]
const assert = require('referee').assert;

describe("#my-issue", function () {
  it("case1", function () {
    var obj = {};
    obj.f = function () {};

    var spy = sinon.spy(obj, "f");

    // withArgs(1) => withArgs(1, 1)
    assert.equals(spy.callCount, 0);
    assert.equals(spy.withArgs(1).callCount, 0);
    assert.equals(spy.withArgs(1, 1).callCount, 0);

    obj.f();
    obj.f(1);
    obj.f(1, 1);
    obj.f(1, 2);

    assert.equals(spy.callCount, 4);
    assert.equals(spy.withArgs(1).callCount, 3); // passed
    assert.equals(spy.withArgs(1, 1).callCount, 1); // assertion-error
    assert.equals(spy.withArgs(1, 2).callCount, 1); // passed
  });
  it("case2", function () {
    var obj = {};
    obj.f = function () {};

    var spy = sinon.spy(obj, "f");

    // change order, withArgs(1, 1) => withArgs(1)
    assert.equals(spy.callCount, 0);
    assert.equals(spy.withArgs(1, 1).callCount, 0);
    assert.equals(spy.withArgs(1).callCount, 0);

    obj.f();
    obj.f(1);
    obj.f(1, 1);
    obj.f(1, 2);

    assert.equals(spy.callCount, 4);
    assert.equals(spy.withArgs(1).callCount, 3); // assertion error
    assert.equals(spy.withArgs(1, 1).callCount, 1); // passed
    assert.equals(spy.withArgs(1, 2).callCount, 1); // passed
  });
});

Result of running above code.

% mocha test.js

#my-issue
1) case1
2) case2

0 passing (21ms)
2 failing

1. #my-issue case1:
   AssertionError: [assert.equals] 0 expected to be equal to 1
   at referee.fail (node_modules/referee/lib/referee.js:193:25)
   at Object.ctx.fail (node_modules/referee/lib/referee.js:90:21)
   at assertion (node_modules/referee/lib/referee.js:98:21)
   at Function.referee.(anonymous function).(anonymous function) [as equals] (node_modules/referee/lib/referee.js:118:23)
   at Context. (test.js:23:12)

2. #my-issue case2:
   AssertionError: [assert.equals] 2 expected to be equal to 3
   at referee.fail (node_modules/referee/lib/referee.js:193:25)
   at Object.ctx.fail (node_modules/referee/lib/referee.js:90:21)
   at assertion (node_modules/referee/lib/referee.js:98:21)
   at Function.referee.(anonymous function).(anonymous function) [as equals] (node_modules/referee/lib/referee.js:118:23)
   at Context. (test.js:43:12)

[kbtknr]

If write the following hack before the assertion, this test will pass.

spy.fakes = [];

With the matchingFake function in spy.js, I think only the last fake (pop()) in fakes matched may be incremented.

[kbtknr]

I think that stub is also affected by this issues.
Because stub depend on spy api, and this is similarly affected.
The following code is able to reproduce this bug.

it("case3", function() {
  var stub = sinon.stub();
  stub.returns(0);
  stub.withArgs(1).returns(1);
  stub.withArgs(1, 1).returns(2);

  assert.equals(stub(), 0);
  assert.equals(stub(1), 1);
  assert.equals(stub(1, 1), 2); // assertion error, stub(1, 1) returns 1 actually.
  assert.equals(stub(2), 0);
});

[kbtknr]

This issues also happen in v2.0.0-pre.6.

Could I send a PR to resolve this issues?

[fatso83]

Yes, please!

[kbtknr]

I investigate the part of matchingFake, in particular pop().
It seems the feature of using withArgs function with passing sinon-matcher don't work.

For example:

it("case4", function () {
  var obj = {};
  obj.f = function () {};
  var spy = sinon.spy(obj, "f");

  assert.equals(spy.withArgs(2, 1).callCount, 0);
  assert.equals(spy.withArgs(sinon.match.any, 1).callCount, 0);
  obj.f(2, 1);
  obj.f(3, 1);
  obj.f(4, 1);

  assert.equals(spy.callCount, 3);
  assert.equals(spy.withArgs(2, 1).callCount, 1);
  assert.equals(spy.withArgs(3, 1).callCount, 1); // assertion-error,
                                                  // seems like, return fake(any, 1)
  assert.equals(spy.withArgs(sinon.match.any, 1).callCount, 3);
});
it("case5", function () {
  var stub = sinon.stub();
  stub.returns(0);
  stub.withArgs(2, 1).returns(1);
  stub.withArgs(sinon.match.any, 1).returns(2);

  assert.equals(stub(), 0);
  assert.equals(stub(1, 1), 2); // ?
  assert.equals(stub(2, 1), 1); // ?
  assert.equals(stub(1, 2), 0);
});

I think case4 should prioritize non-matcher over matcher, and return its fake.
Current behavior is returning last matched fake (pop()).
So, the returning fake is undefined because of calling order withArgs() and making order fakes.
In case5, stub do likewise.

But, this behavior is better to decide this strictly.

[ fake(1, any), fake(any, 1) ] => withArgs(1, 1) => How to prioritize? or conventional pop()
[ fake(sinon.match.any), fake(sinon.match.number) ] => withArgs(1) => Should be any types of simon-matcher prioritized?

Seems like, case4 and case5 is different from root of issue, so should I open the new issue?