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

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

Open
pastelmind opened this issue Jul 20, 2020 · 7 comments
Open

Comments

@pastelmind
Copy link

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
@mroderick
Copy link
Member

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

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
    function mirrorPropAsAssertion(name, method, message) {
    var msg = message;
    var meth = method;
    if (arguments.length === 2) {
    msg = method;
    meth = name;
    }
    assert[name] = function(fake) {
    verifyIsStub(fake);
    var args = arraySlice(arguments, 1);
    var failed = false;
    verifyIsValidAssertion(name, args);
    if (typeof meth === "function") {
    failed = !meth(fake);
    } else {
    failed = typeof fake[meth] === "function" ? !fake[meth].apply(fake, args) : !fake[meth];
    }
    if (failed) {
    failAssertion(this, (fake.printf || fake.proxy.printf).apply(fake, concat([msg], args)));
    } else {
    assert.pass(name);
    }
    };
    }

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.

@mantoni
Copy link
Member

mantoni commented Jul 24, 2020

// 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
Copy link
Member

mantoni commented Jul 24, 2020

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.

@dpmott
Copy link
Contributor

dpmott commented Jan 14, 2021

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

dpmott commented Jan 19, 2021

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?

@fatso83
Copy link
Contributor

fatso83 commented Jan 20, 2021

@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.

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

5 participants