Add in matcher #1811

mhmoudgmal · 2018-05-26T13:07:44Z

Purpose (TL;DR)

Add sinon.match.in matcher

Background (Problem in detail)

I had a case where I wanted to match a computed value (a value that I don't know exactly what it is), but I knew the possibilities of that value.

so I had to do something like this:
sinon.match.same('something').or(sinon.match.same('something_else'))

const expectedPayload = {
    experiment_uuid: 'target1',
    uuid: 'fake-uuid',
    variation: sinon.match.same('control').or(sinon.match.same('group1'))
};

I know also there is array.contains matcher.. but the case above, that payload is used in different places where I check if a function called with it, and I thought it would be much easier if I have something like this: sinon.match.in([v1, v2, v3])

Solution

sinon.match.in matcher.
Allows to match a value to one of the elements of an array.
And it can be useful when the the value is not known,
but could be one of few possibilities provided as an array.

Usage:
sinon.match.in([p1, p2, p3, ..])

How to verify - mandatory

Check out this branch
npm install
npm test

Checklist for author

npm run lint passes
References to standard library functions are cached.

Allows to match a value to one of the elements of an array. And it can be useful when the the value is not known, but could be one of few possibilities provided as an array. Usage: sinon.match.in([p1, p2, p3, ..])

mroderick

Thank you for contributing to Sinon.

I only have a few comments for this pull request.

Remember to add documentation for the new feature in docs/release-source.

mroderick · 2018-05-29T05:40:42Z

lib/sinon/match.js

@@ -178,6 +178,18 @@ match.same = function (expectation) {
    }, "same(" + valueToString(expectation) + ")");
 };

+match.in = function (arrayOfExpectations) {
+    if (!(arrayOfExpectations instanceof Array)) {


Please use Array.isArray to accurately detect instances of Array.

See http://web.mit.edu/jwalden/www/isArray.html for a detailed explanation.

mroderick · 2018-05-29T05:43:50Z

test/match-test.js

+
+                assert.exception(function () {
+                    sinonMatch.in(arg);
+                }, {name: "TypeError"});


It would be good to also test for the message, so we can be sure that it's actually expected error. Otherwise, another error might go undetected by this test for a long time.

mroderick · 2018-05-29T05:47:47Z

test/match-test.js

+                it("returns true", function () {
+                    arrays.forEach(function (array) {
+                        var inMatcher = sinonMatch.in(array);
+                        assert(inMatcher.test(array[0]));


Referee's assert checks for truthy, not true.

You'll want to use assert.isTrue to test for true.

mroderick · 2018-05-29T05:48:34Z

test/match-test.js

+            });
+
+            context("and none of the elements is the same as the actual", function () {
+                it("returns true", function () {


Copy-pasta, this should read it("returns false", function () {

Assert for the exception message. Correct an example description. Assert for true instead of truthy.

mhmoudgmal · 2018-05-29T08:49:01Z

@mroderick Thanks a lot 👍
I updated the code and added the documentation, please rephrase it if it's not clear.

fearphage

Just a few simplifications for the tests

fearphage · 2018-05-29T15:17:07Z

test/match-test.js

+            assert(sinonMatch.isMatcher(inMatcher));
+        });
+
+        context("when the test is called with non array argument", function () {


2 things here:

Use describe instead when necessary

Only nest tests when there is additional/unique setup

In this instance, the nesting is not warranted. Just change the label/name on the test below.

fearphage · 2018-05-29T15:18:16Z

test/match-test.js

+            });
+        });
+
+        context("when the test is called with an array", function () {


Use describe.

fearphage · 2018-05-29T15:18:47Z

test/match-test.js

+            ];
+
+            context("and one of the elements is the same as the actual", function () {
+                it("returns true", function () {


Rename the tests instead of nesting them here and on the following tests.

fatso83 · 2018-06-05T20:52:15Z

docs/release-source/release/matchers.md

+#### `sinon.match.in(array)`
+
+Requires the value to be in the `array`.
+
 *Since `sinon@2.0.0`*


The line about Sinon 2 belongs to the symbol matcher above. I'll fix it in my browser, if my crappy Moto G5 can take the load 😁

Was the odd man out. - wasn't done for any of the other matchers. Use the change log for history instead (or got blame).

mroderick · 2018-06-06T06:46:56Z

This has been published to NPM as sinon@5.1.0

Add in matcher

01f6aef

Allows to match a value to one of the elements of an array. And it can be useful when the the value is not known, but could be one of few possibilities provided as an array. Usage: sinon.match.in([p1, p2, p3, ..])

mroderick requested changes May 29, 2018

View reviewed changes

mhmoudgmal added 2 commits May 29, 2018 10:20

Use isArray to accurately check for arrays

8e2bd7c

Improve the tests

61959f5

Assert for the exception message. Correct an example description. Assert for true instead of truthy.

Add documentation for sinon.match.in matcher

722465e

mhmoudgmal force-pushed the feature/in-matcher branch from 9155895 to 722465e Compare May 29, 2018 11:24

fearphage requested changes May 29, 2018

View reviewed changes

Improve tests readability

81a87d5

fearphage approved these changes May 29, 2018

View reviewed changes

fatso83 reviewed Jun 5, 2018

View reviewed changes

Remove notice

09f849c

Was the odd man out. - wasn't done for any of the other matchers. Use the change log for history instead (or got blame).

fatso83 merged commit 53169e3 into sinonjs:master Jun 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add in matcher #1811

Add in matcher #1811

mhmoudgmal commented May 26, 2018 •

edited by mroderick

mroderick left a comment

mroderick May 29, 2018

mroderick May 29, 2018

mroderick May 29, 2018

mroderick May 29, 2018

mhmoudgmal commented May 29, 2018

fearphage left a comment

fearphage May 29, 2018

fearphage May 29, 2018

fearphage May 29, 2018

fatso83 Jun 5, 2018

mroderick commented Jun 6, 2018

Add in matcher #1811

Add in matcher #1811

Conversation

mhmoudgmal commented May 26, 2018 • edited by mroderick

Purpose (TL;DR)

Background (Problem in detail)

Solution

How to verify - mandatory

Checklist for author

mroderick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhmoudgmal commented May 29, 2018

fearphage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroderick commented Jun 6, 2018

mhmoudgmal commented May 26, 2018 •

edited by mroderick