sinonjs / sinon

Test spies, stubs and mocks for JavaScript.
https://sinonjs.org/
Other
9.6k stars 769 forks source link

`returns` clears fake's `callArgAt` #2572

Closed fpascutti closed 2 months ago

fpascutti commented 8 months ago

Describe the bug Starting from sinon@17.0.1, using returns clears fake's callArgAt. This means that the callback will not be called.

To Reproduce Run the following:

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

const stub = sinon.stub();
stub.callsArgWith(0, "Hello").returns("World");

const cb = sinon.stub();
const ret = stub(cb);

assert.strictEqual(ret, "World");
assert(cb.calledOnce);
assert.deepEqual(cb.firstCall.args, ["Hello"]);

Expected behavior The callback should be properly invoked.

Context (please complete the following information):

Additional context

Bug introduced by the fix for #2566 (see #2567). Might be fixed by removing this line but this needs validation.

+@rluvaton

rluvaton commented 8 months ago

Will try to create a fix later today

But what do we define the behavior should be?

Should returns only override the things that affect returning/throwing values?

rluvaton commented 8 months ago

Unfortunately, I won't be able to get to that soon, do you want to create a PR to fix it?

zmknox commented 4 months ago

I seem to be experiencing this same bug, but in reverse. I have a .returns() followed later by a callsArgWith(), and the callsArgWith is clearing the return, breaking my tests.

In terms of what the behavior should be, I feel like there isn't really a conflict here. I don't see any reason that the stub couldn't both call one of its args and return some value.

fatso83 commented 4 months ago

Yeah, no conflict AFAI can see either. This should be a really small fix, if someone is interested in getting it working. Just remember to document it using a regression test 😉

fatso83 commented 2 months ago

Should be fixed by #2593