stub.withArgs() with matchers breaks subsequent stubbing

[rluba]

• Sinon version : 3.2.1
• Environment : node 6.9.5

Here’s a minimal test that shows the issue

const stub = sinon.stub();
stub.withArgs(sinon.match.any).returns('old');
stub.withArgs(1).returns('new');

const result1 = stub(1);
const result2 = stub(2);

assert.equal(result1, 'new');
assert.equal(result2, 'old');

What did you expect to happen? result2 should be 'old';

What actually happens The sample code throws because result2 is set to new

This happens because Sinon is too lenient when checking whether subsequent withArgs calls match an existing fake. Therefore, .withArgs(1).returns('new'); modifies the behavior of the fake established with withArgs(sinon.match.any) instead of creating a new fake.

But it’s even worse: The bug persists across reset(). This code also throws:

const stub = sinon.stub();
stub.withArgs(sinon.match.any).returns('old');
stub.reset();
stub.withArgs(1).returns('new');

const result1 = stub(1);
const result2 = stub(2);

assert.equal(result1, 'new');
assert.equal(result2, undefined);

… because resetBehavior retains all fakes (to keep their history).

[rluba]

The bug still exists.

[mroderick]

I've created #1654 with your tests to show the continued existence of this bug.

Do you want to take a stab at fixing it?

[rgroothuijsen]

Not sure what a good solution is to this one. deepEqual checks its arguments for matchers and concludes that "any" matches "1" (since the "any" matcher always returns true), and the additional checks on the objects' own properties afterwards also yield no differences, since they have the same amount of own properties.

[jrnail23]

@rgroothuijsen, It seems like stub.withArgs() is just generally broken -- the following is a failing test in v7.3.2, running on node v10.15.3 (LTS):

const sinon = require("sinon");
const assert = require("assert");

const service = {
    find: () => {}    
};

service.find = sinon.stub()
    .withArgs("A")
    .returns("HELLO");

const result = service.find("B");

assert.notEqual(result, "HELLO");

I expect service.find to return "HELLO" when called with "A", and to return undefined when called with "B". However, when called with "B", it returns "HELLO".

[fatso83]

@jrnail23 I've answered you in #2025. It's not broken and it's covered by tests. Please see the docs for correct usage of this method. The example (unfortunately) showcases the problems of chained interfaces.

See these related issues for background: https://github.com/sinonjs/sinon/issues/1748#issuecomment-376861414, https://github.com/sinonjs/sinon/issues/1909#issuecomment-424321291, #1551

[rluba]

@fatso83 Why have you closed this issue? @jrnail23’s comment was incorrect, but the original issue still remains.

[fatso83]

@rluba 💯 Thank you for noticing that! I tagged and closed the wrong issue. That's what you get with too many issues open ...

[tatarin0]

So the issue exists in 4.2.2, the workaround if anyone needs is instead of using .withArgs(arg1, arg2) use .callsArgOn(index, context) and chain for multiple:

stub.callsArgOn(1, arg1).callsArgOn(2, arg2)
...
stub.restore();

[jlurena]

The same issue occurs when stubbing across different contexts ('describe').

describe('first', () => {
  let myMock;
  before(() => {
    myMock = sinon.stub(Bar, 'greet');
    myMock.withArgs('Hello').returns("Earth!");
  });
 
  after(() => myMock.restore());

  it('greets the Earth', () => {
    expect(Bar.greet('Hello')).to.equal('Hello Earth!')
  });
});

describe('second', () => {
  let myMock;
   // It breaks here with a 'Attempted to wrap greet which is already wrapped' error
  before(() => {
    myMock = sinon.stub(Bar, 'greet');
    myMock.withArgs('Hello').returns("World!");
  });

  after(() => myMock.restore());

  it('greets the World', () => {
    expect(Bar.greet('Hello')).to.equal('Hello World!)
  });
});

Work around is to use sandboxes for me.

[tobslob]

I am stubbing a method configureWorkspace and I want to able to be specific with the argument, by passing an id. But it return undefined, and works fine if I am matching with sinon.match.any. I am not sure why it's not working with an argument id

const configureWorkspace = sinon.stub(Workspaces, "configureWorkspace");

export function mockConfigureWorkspace(id: string, data: Authorisation) { return configureWorkspace.withArgs(sinon.match.any, id, sinon.match.any).resolves(data); }

[flickz]

Any solution to this yet?

[fatso83]

@flickz The inherent mutable nature of the Stubs API makes it prone to bugs like this and makes it a nightmare to handle stuff like this. I guess that is why no one has had a go, yet (feel free, though!). Try the Fakes API for an experience that does not cause as much confusion, it's simpler API covers all the scenarios without the confusion.

[m-radzikowski]

As a workaround, the original problem seems to not occur if subsequent stub.withArgs() also use matchers instead of the raw value:

const stub = sinon.stub();
stub.withArgs(sinon.match.any).returns('old');
stub.withArgs(sinon.match.in([1])).returns('new');
// stub.withArgs(1).returns('new'); // <- would make stub(2) return "new"

expect(stub(1)).toBe('new');
expect(stub(2)).toBe('old');
expect(stub('qq')).toBe('old');

The sinon.match.in([1]) could be simplified with some helper function like const exact = x => sinon.match.in([x]).

However, this is still an issue with stub.reset():

const stub = sinon.stub();
stub.withArgs(sinon.match.any).returns('old');
stub.withArgs(sinon.match.in([1])).returns('new');
stub.reset();
stub.withArgs(sinon.match.any).returns('newer');

expect(stub(1)).toBe('newer'); // fails, is undefined
expect(stub(2)).toBe('newer');
expect(stub('qq')).toBe('newer');

The only workaround I can see is to replace the stub with a fresh new sinon.stub(); instead of calling reset().