calledWith ignores symbols used as property keys

[BrandonE]

Describe the bug
Asserting calledWith ignores mismatched properties if they keys are JavaScript symbols.

To Reproduce
Steps to reproduce the behavior:

var sinon = require('sinon');
var mock = sinon.mock();

mock({
    [Symbol("a")]: true
});

sinon.assert.calledWith(
    mock,
    {
        [Symbol("a")]: false
    }
);

Expected behavior
The above script should throw an AssertError because the mock was called with an object containing a true value when we were expecting a false value. It should behave similarly to this example that uses non-symbolic keys:

var sinon = require('sinon');
var mock = sinon.mock();

mock({
    a: true
});

sinon.assert.calledWith(
    mock,
    {
        a: false
    }
);

Context (please complete the following information):

• Library version: 7.2.3
• Environment: Node.js v10.8.0
• Other libraries you are using: None; I reproduced this issue in a newly initialized npm project.

Additional context
I started working on a solution, but I wanted to raise an issue before opening pull requests like the contributing guide suggests to make sure I'm going down the right path. Using this change for samsam and this change for formatio, I get the behavior I expected for the script in the To Reproduce section:

Because symbols are unique, this:

var sinon = require('sinon');
var mock = sinon.mock();

mock({
    [Symbol("a")]: true
});

sinon.assert.calledWith(
    mock,
    {
        [Symbol("a")]: true
    }
);

will throw an AssertError (thanks @pettyalex for pointing this out):

This, however, will not throw an AssertError:

var sinon = require('sinon');
var mock = sinon.mock();

const symbol = Symbol("a");

mock({
    [symbol]: true
});

sinon.assert.calledWith(
    mock,
    {
        [symbol]: true
    }
);

Unfortunately, the samsam change breaks one unit test:

This is because arguments has a property symbol called iterator:

> function test() { return Object.getOwnPropertySymbols(arguments) } test();
[ Symbol(Symbol.iterator) ]

[mantoni]

Thank you for putting this detailed analysis and code changes together. We're definitely lacking proper Symbol support in Sinon and should do something about it.

Regarding your suggested solution, I think the fix should be slightly different: I'd say we should check the expectation for any symbol properties and only compare those to the actual. This means that additional symbol properties being added to the actual object do not break existing assertions. If we want to enforce strict matching of all symbols, then this would be a breaking change, because Sinon never looked at symbol properties before.

Any opinions or other ideas @sinonjs/sinon-core?

[BrandonE]

I like your suggestion. I'll put that solution together and open a PR for both samsam and formatio.

[mroderick]

That's some very detailed analysis, thank you ❤️

I like the proposed solution that doesn't break existing assertions 👍

[BrandonE]

@mantoni @mroderick samsam and formatio pull requests for your consideration. Although I implemented the proposed solution and all existing unit tests pass, I still have concerns about the backwards compatibility of the samsam change, which I detail in the solution section.

[BrandonE]

Resolved by sinonjs/samsam#64