Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

returns clears fake's callArgAt #2572

Closed
fpascutti opened this issue Nov 6, 2023 · 5 comments
Closed

returns clears fake's callArgAt #2572

fpascutti opened this issue Nov 6, 2023 · 5 comments

Comments

@fpascutti
Copy link

fpascutti commented Nov 6, 2023

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):

  • Library version: 17.0.1
  • Environment: Windows / Node.JS

Additional context

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

+@rluvaton

@rluvaton
Copy link
Contributor

rluvaton commented Nov 6, 2023

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
Copy link
Contributor

rluvaton commented Nov 6, 2023

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

@zmknox
Copy link

zmknox commented Feb 14, 2024

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
Copy link
Contributor

fatso83 commented Feb 15, 2024

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
Copy link
Contributor

fatso83 commented Apr 25, 2024

Should be fixed by #2593

fatso83 added a commit that referenced this issue Apr 25, 2024
* Add regression test for #2572

* Partially revert "fix returns does not override call through (#2567)"

This partially reverts commit 5fde5ae:
- we keep the tests
- revert to the old manual clearing of props

The clearing of callThrough has then been manually added to what I
deem are the relevant spots. Tests are unaltered.
@fatso83 fatso83 closed this as completed Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants