Sinon: Arrays matched with deepEqual

[Elberet]

Consider:

var actual = ['foo'];
actual.hasCustomProperty = true.

var match = sinon.match(['foo']);
match.test(actual); // => false

According to the docs, this test should succeed: actual contains and matches all of the properties defined on the expectation object, yet the matcher uses deepEqual to compare arrays and stumbles over the additional property on actual.

PR #316 to compare arrays without deepEqual had been accepted way back when in 2013, but that behaviour is missing from the current code. Is this a regression or was this reverted intentionally?

[mantoni]

Can you please state which Sinon version you're using? 1.17.5 or 2.0.0-pre?

[Elberet]

I've just upgraded from v1.17.4 to v1.17.5, but the behaviour is the same as before (as expected from a quick glance at the commits between those two releases).

[fatso83]

@Elberet: looked into this now. That test case started failing with the code from #316 (e4fdd41). It worked before that. I automated a test to find this, if you want to verify.

[fatso83]

@Elberet : To which documentation are you referring? The Sinon homepage does not seem to mention either deepEquals nor matcher#test, so I am curious to find out where you got this impression from.

PR #316 does not avoid using deepEquals on arrays, it _makes sure_ deepEquals is being used for _both_ array elements _and_ attributes. Both the code, behavior and tests that were introduced then are still present.

deepEquals is a two-way equality, which you can see from the fact that the amount of props need to be equal (after checking equality one-way first). If b has props that a does not have the length will simply not match. It as been like that since the first usable version of Sinon back in April 2010 (84dbb22).

So it seems like what you want is to have matcher#test be a one-way equality testing of sorts. That seems more like a feature request than a bug, unless I am misunderstanding you somehow?

[Elberet]

You are right about PR #316. I was too hasty and only noticed that a call to deepEqual got removed.

Considering the doc:

sinon.match(object)
Requires the value to be not null or undefined and have at least the same properties as expectation.

The use of "at least" indicates that the matcher makes no assertions regarding "additional" properties, yet the test is designed differently. It's probably unwise to change the code now if it's been behaving this way for the last 6 years, but then the documentation needs clarifying.

So yes, this is essentially a feature request. Something like sinon.match.array.contains([array]), which tests that actual is an array and contains the expected values, ignoring any other properties?

[fatso83]

Those docs refer to object, not array, so you are relying on undocumented features (although, of course every type is an object of sorts, and adding props to an array makes the confusion complete). I totally agree with you on the documentation part. The semantics for how sinon.match should work for arrays is undefined in the docs, and in the tests the semantics are only defined for arrays that are _properties of an object_ ("returns true if array is equal" and "returns false if array is not equal").

deepEqual should not change, since that is just a fallback in the match logic when we cannot find a match in our type mapping (string, object, number or regexp):

if (type in TYPE_MAP) {
        TYPE_MAP[type](m, expectation, message);
    } else {
        m.test = function (actual) {
            return deepEqual(expectation, actual);
        };
    }

What could change is sinon.match: since the behavior for arrays is currently not defined in the docs we could add 'array' to our TYPE_MAP and add semantics that matched that of 'object'.

Something like

case 'array':
    m.test = function(
        var expectationProps = extractProperties(expected);        
        return match(expectationProps).test(actual) && arrayContains(expected, actual);
    }

_But_ after playing with this for the latest hour there needs to be a definition of how the array contents are to be matched (the arrayContains part). If we look at how sinon.match(object) works these should all return true, I think:

sinon.match([1,2,3]).test([1,2,3]);
sinon.match([2,3]).test([1,2,3]);
sinon.match([2,3]).test([100,1,2,3,1000]);

Currently, only the first would return true. People might rely on this behavior, but it _is not mentioned how it should work in the docs_ (thus undefined - so we should be free to change it) and since we _are_ releasing a new major version this could be part of the breaking changes. What do you think, @sinonjs/sinon-core?

[Elberet]

I heavily disagree with the suggestion, that sinon.match([2,3]).test([1,2,3]) should be true; adding an element to a list fundamentally changes the information represented by that list, either because the element's position in the list matters or because the list is iterated over.

[Elberet]

P.S.: I've monkeypatched a helper into Sinon to provide the match I suggested earlier.

[mantoni]

I agree with @Elberet here. It might surprise people that arrays are matches in parts. I also like @Elberet approach of adding array specific matchers. However, it is already possible to check only specific array entries like this:

sinon.match({
  0: 'foo',
  2: 'bar'
})

This would assert that the given array has ’foo’ at index 0 and ’bar’ at index 2.

[fatso83]

I have absolutely no problem with keeping the default of using deepEqual. The important thing is just that it's documented, and that users know that deepEqual is the default for anything else than string,number,regex and object. And that one should use either (the as of yet not existing) array specific matchers or something like @mantoni proposed.

@Elberet: I actually modeled the implementation on the _name_ of your helper, since contains usually concerns itself with _subsets_ (one known example being domElem.classList.contains('myClass')). I'd rather have see such a helper as array.equals in a family of helpers like

• match.array.equals(): matches when the arrays are equal (unstrict)
• match.array.startWith(): matches sequence against start of array
• match.array.endsWith(): matches sequence against end of array
• match.array.contains(): matches sequence anywhere in the array

[Elberet]

@fatso83 yeah, at this point the method names are pretty much up to taste and the quirks of one's native language. As long as it's either startsWith or endWith, those names are fine. :+1:

[mroderick]

To make it easier for people wanting to contribute to figure out what to put into a PR, let me try to summarise this thread.

There's a feature request to implement more matchers, with the following names and meanings:

• match.array.equals(): matches when the arrays are equal (unstrict)
• match.array.startWith(): matches sequence against start of array
• match.array.endsWith(): matches sequence against end of array
• match.array.contains(): matches sequence anywhere in the array

Is that the consensus @Elberet @fatso83 @mantoni?

[Elberet]

Looks good. More expressive predicates are possible (containsSequence instead of contains above, containsAll where array contains given elements in any order, containsInOrder where array contains given elements in order but optionally interspersed with other elements), but might not be necessary.

[fearphage]

I'd vote match.array.deepEquals() as being more explicit than match.array.equals().

[lucasfcosta]

Hey friends, I think this one can be closed already.