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.stub().withArgs() matches too often #1882

Closed
tbiesemann opened this issue Aug 22, 2018 · 7 comments
Closed

sinon.stub().withArgs() matches too often #1882

tbiesemann opened this issue Aug 22, 2018 · 7 comments

Comments

@tbiesemann
Copy link

tbiesemann commented Aug 22, 2018

Sinon 6.1.5
This test fails:

it('sinon use constructor name when checking deepEquality', function() {
    class ClassWithoutProps {};
    const arg1 = new ClassWithoutProps(); //arg1.constructor.name === ClassWithoutProps
    const arg2 = new Proxy({}, {});	//arg2.constructor.name === Object
    const stub = sinon.stub();
    stub.withArgs(arg1).returns(5);
    const result = stub(arg2);
    assert.strictEqual(result, undefined); //[ERR_ASSERTION]: 5 === undefined
});

Describe the bug
withArgs() has too many matches.
Empty objects, instances of empty classes or proxies are all treated the same.
The deepEqual function in sinon returns true when comparing those.

One could use the constructor.name for further comparison in deepEqual, to solve some of these false matches.

@mgred
Copy link
Contributor

mgred commented Sep 9, 2018

@tbiesemann, @mroderick
Just found out that the samsam.deepEqual() is the function in charge here that returns true.
I opened an issue in the samsam repo sinonjs/samsam#37

@mroderick
Copy link
Member

@mgred I don't think we're quite done yet

  • npm install @sinonjs/samsam@2.1.0
  • Adding a slightly modified version if the provided test to issues-test.js
describe("#1882", function () {
    it("should use constructor name when checking deepEquality", function () {
        class ClassWithoutProps {};
        const arg1 = new ClassWithoutProps(); //arg1.constructor.name === ClassWithoutProps
        const arg2 = new Proxy({}, {}); //arg2.constructor.name === Object
        const stub = sinon.stub();
        stub.withArgs(arg1).returns(5);
        const result = stub(arg2);
        assert.same(result, undefined); //[ERR_ASSERTION]: 5 === undefined
    });
});

Produces AssertionError: [assert.same] 5 expected to be the same object as undefined.

Would you take another look at this one?

@mgred
Copy link
Contributor

mgred commented Sep 14, 2018

@mroderick Sure I will.

Also: sinonjs/samsam#39

mgred added a commit to mgred/sinon that referenced this issue Sep 16, 2018
* add test case provided in sinonjs#1882
* change some eslint config to match test:
** class definitions are not enabled in ecmaVersion 5,
** addint Proxy as global

See: sinonjs#1882, sinonjs/samsam#37
@mgred mgred mentioned this issue Sep 16, 2018
2 tasks
@mgred
Copy link
Contributor

mgred commented Sep 16, 2018

@mroderick, In the #1905 I added your test and it passes for me.

mroderick pushed a commit that referenced this issue Sep 21, 2018
* test(1882): add test case

* add test case provided in #1882
* change some eslint config to match test:
** class definitions are not enabled in ecmaVersion 5,
** addint Proxy as global

See: #1882, sinonjs/samsam#37

* fix(1882): changes according to review

* Rewert config to support IE11 again
* Adopt changes to test to be ES5
@mroderick
Copy link
Member

@tbiesemann can you verify that the issue has been fixed in latest version of Sinon?

@fatso83
Copy link
Contributor

fatso83 commented Sep 24, 2018

This is failing on Internet Explorer 11. Two different runs:

@tbiesemann
Copy link
Author

@mroderick Confirmed - Fix works with Sinon 6.3.4

@mgred mgred closed this as completed Oct 14, 2018
franck-romano pushed a commit to franck-romano/sinon that referenced this issue Oct 1, 2019
* test(1882): add test case

* add test case provided in sinonjs#1882
* change some eslint config to match test:
** class definitions are not enabled in ecmaVersion 5,
** addint Proxy as global

See: sinonjs#1882, sinonjs/samsam#37

* fix(1882): changes according to review

* Rewert config to support IE11 again
* Adopt changes to test to be ES5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants