sinon icon indicating copy to clipboard operation
sinon copied to clipboard

sinon.assert.calledOnceWithExactly() does not accept a single spy call

Open pastelmind opened this issue 5 years ago • 7 comments

Describe the bug According to the v9.0.2 documentation, sinon.assert.calledOnceWithExactly() can accept a single, dedicated spy call, instead of a spy. However, it throws an error instead:

const sinon = require("sinon");

let add = sinon.fake((a, b) => a + b);
add(5, 100);

// Passes
sinon.assert.calledOnceWithExactly(add, 5, 100);
// Passes
sinon.assert.calledWithExactly(add.getCall(0), 5, 100);
// Should pass, but throws in v9.0.2
sinon.assert.calledOnceWithExactly(add.getCall(0), 5, 100);

The exception message:

C:\Users\<username>\Documents\GitHub\test-proj\node_modules\sinon\lib\sinon\assert.js:110
        throw error;
        ^

Error [AssertError]: expected fake(5, 100) => 105 at Object.<anonymous> (C:\Users\<username>\Documents\GitHub\test-proj\index.js:4:1) to be called once and with exact arguments
    at Object.fail (C:\Users\<username>\Documents\GitHub\test-proj\node_modules\sinon\lib\sinon\assert.js:107:21)
    at failAssertion (C:\Users\<username>\Documents\GitHub\test-proj\node_modules\sinon\lib\sinon\assert.js:66:16)
    at Object.assert.<computed> [as calledOnceWithExactly] (C:\Users\<username>\Documents\GitHub\test-proj\node_modules\sinon\lib\sinon\assert.js:92:13)
    at Object.<anonymous> (C:\Users\<username>\Documents\GitHub\test-proj\index.js:7:14)
    at Module._compile (internal/modules/cjs/loader.js:1138:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1158:10)
    at Module.load (internal/modules/cjs/loader.js:986:32)
    at Function.Module._load (internal/modules/cjs/loader.js:879:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47

To Reproduce Steps to reproduce the behavior:

  1. Create a fake method with sinon.fake()
  2. Call the fake method a number of times
  3. Call sinon.assert.calledOnceWithExactly(), passing a dedicated spy call using getCall(), and specify the correct arguments.
  4. Assertion error is raised

Expected behavior sinon.assert.calledOnceWithExactly() should pass it normally.

Context (please complete the following information):

  • Library version: v9.0.2
  • Environment: Node.js 12.18.2 / Windows 10 (x86-64)
  • Example URL: (none)
  • Other libraries you are using: Mocha

pastelmind avatar Jul 20 '20 05:07 pastelmind

I have verified the existence locally. Don't yet know if it is a recent addition, or if it was always failing.

I'll try to find some time for a git bisect session soon, unless someone beats me to it

mroderick avatar Jul 21 '20 17:07 mroderick

I've managed to construct a smaller test case to produce the error

const sinon = require("sinon");

let spy = sinon.spy();
spy(5, 100);

// passes
spy.calledOnceWithExactly(spy.getCall(0), 5, 100)

// throws
sinon.assert.calledOnceWithExactly(spy.getCall(0), 5, 100);

This teaches me two things:

  1. the problem originates in spy, fakeis using spy, which is why the error also manifests there
  2. there's an error in the area where the assertions are constructed, namely https://github.com/sinonjs/sinon/blob/f34a13ec8c878188f805a57a0c1c1de4176c2765/lib/sinon/assert.js#L69-L97

For the second, on line 88 failed is set to false because !fake[meth] evaluates to false, because fake[meth] is undefined. That means that the matcher doesn't have a calledOnceWithExactly property on it, which kind of makes sense.

I guess the next step to investigate, is how calledWithExactly is different from calledOnceWithExactly, since that passes just fine.

But, it's past midnight here, so that'll have to wait.

mroderick avatar Jul 21 '20 22:07 mroderick

// passes spy.calledOnceWithExactly(spy.getCall(0), 5, 100)

There are two things wrong with this:

  1. The function never throws, it returns true or false, so it always "passes".
  2. The function is called on the spy and is not supposed to receive a spy or a call as the first argument, unlike sinon.assert.* calls.

My observation is that

  • spy.getCall(0).calledWithExactly(5, 100) returns true
  • spy.getCall(0).calledOnceWithExactly(5, 100) is not a function, as you figured out from debugging as well :)

the problem originates in spy, fake is using spy

This isn't the case anymore. In our last hackathon I've separated the implementations and the common interface is now proxy.

mantoni avatar Jul 24 '20 07:07 mantoni

Anyway, I think this is a copy-paste bug in the documentation. It makes not much sense to check that a single call was called once. If the call wasn't made at all, the result of sinon.getCall(0) is null. It's basically a variation of sinon.assert.calledOnce wich does not claim to support a call argument in the documentation.

I'd suggest to fix it in the documentation and throw a meaningful exception if sinon.assert.calledOnce[WithExactly] receives a call as the first argument.

mantoni avatar Jul 24 '20 07:07 mantoni

Interesting.

I was going to write a big post about how I disagreed that it doesn't make much sense, but then I discovered that called() and calledOnce() do not accept a SpyCall type, and will not compile when given such in typescript per the type definitions in @types/sinon.

The type of the first argument for calledOnceWithExactly() (for both the sinon and sandbox interfaces) should probably be updated to accept only SinonSpy<TArgs> instead of SinonSpy<TArgs> | SinonSpyCall<TArgs>.

I'll see if I can put together a PR to address this issue.

dpmott avatar Jan 14 '21 20:01 dpmott

I've looked into this, and I can't get my head wrapped around what in the sinon code base would convey typing hints to the good folk maintaining @types/sinon. I can update the documentation, but I'm unclear what other changes could/should be made in the sinon code base. What am I missing?

Also, should I just open a related issue in the @types/sinon project to request a typing change there?

dpmott avatar Jan 19 '21 22:01 dpmott

@dpmott While we are pondering of adding TypeScript definitions generated through JSDoc annotations ourselves, like we have done in @sinonjs/fake-timers, we are not there yet, so I would open an issue in the DefinitelyTyped project.

fatso83 avatar Jan 20 '21 08:01 fatso83