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

calledWith reporting a match with completely different arguments (promises) #2345

Closed
NoxWings opened this issue Mar 5, 2021 · 8 comments
Closed
Labels

Comments

@NoxWings
Copy link

NoxWings commented Mar 5, 2021

Describe the bug
Sinon spy calledWith doesn't work with promises

To Reproduce
Steps to reproduce the behavior:

const mySpy = sinon.spy();
mySpy(Promise.resolve("a"));
const output = mySpy.calledWith(Promise.resolve("b"));
console.log(output); // true when it should be false

Expected behavior
calledWith should fail when comparing obviously different arguments than the one it was previously called with.
I guess the deepEqual function used inside this method is not correctly comparing promises.
The example above should be able to fail for either of these 2 options:

  1. The promise instance provided as an argument of the spy call and the calledWith expectation are different
  2. The promises resolve to a different value

I guess option 1 would be the expected behaviour by most users and it would probably be the easiest to implement.

Context (please complete the following information):

  • Library version: 9.2.4
  • Environment: node v14.15.1
  • Other libraries you are using: can be replicated just with sinon
@mantoni
Copy link
Member

mantoni commented Mar 5, 2021

I agree that your example should return false since the promise instances are different. This is a bug.

However, I think we can only do a strict equality check on promises since it's impossible to inspect the state and value of a promise synchronously.

To write an assertion on the resolved promise value, you'd depend on your assertion library to support that, e.g. with referee I would write it like this:

await assert.resolves(mySpy.lastCall.args[0], 'b');

@mantoni mantoni added the Bug label Mar 5, 2021
@NoxWings
Copy link
Author

NoxWings commented Mar 5, 2021

I am personally using chai so what I am currently doing right now is pretty much the same:

expect(await mySpy.getCall(0).args[0]).to.deel.equal('a');

It looks really ugly though :(

But sometimes I have the specific Promise instance right on the test so that would look like

expect(mySpy.calledWith(myPromise)).to.equal(true)

or even prettier with sinonChai

expect(mySpy).to.have.been.calledWith(myPromise);

@mantoni
Copy link
Member

mantoni commented Mar 5, 2021

This will need to be fixed in @sinonjs/samsam (see sinonjs/samsam#216). @NoxWings Would you like to look into it yourself?

@NoxWings
Copy link
Author

NoxWings commented Mar 5, 2021

I've uploaded a PR. Just let me know if you need anything else there.

@srknzl
Copy link
Contributor

srknzl commented Apr 19, 2021

I wonder why calledWith does not check for strict equality (===). I am using this type of assertions as a workaround:

const mySpy = sinon.spy();

const anObject = {};
const anotherObject = {};

mySpy(anObject);
const output = mySpy.calledWith(anotherObject);
console.log(output); // true, I expect it to be false

// My workaround with chai

expect(mySpy.calledOnce).to.be.true;
expect(mySpy.firstCall.args[0]).to.be.eq(anObject);

if calledWith checked for strict equality, I would just say:

expect(mySpy.calledWith(anObject)).to.be.true;

This becomes even harder when there are multiple object arguments.

@mantoni
Copy link
Member

mantoni commented Apr 19, 2021

@srknzl You can solve that with expect(mySpy.calledWith(sinon.match.same(anObject))).

@srknzl
Copy link
Contributor

srknzl commented Apr 19, 2021

Thanks a lot, I checked matchers but looks like I missed that one

@fatso83
Copy link
Contributor

fatso83 commented Apr 26, 2021

PR is pending a small update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants