Integrate with sinon match #48

mantoni · 2018-12-06T16:28:40Z

This is a solution for sinonjs/referee#29, merging samsam.match with sinon.match and integrating the whole thing with samsam.deepEqual. This is based on the work done in #36.

I was dancing around this for a few days now trying a few approaches that lead to huge changes. It turns out to be quite straight forward to integrate everything without even breaking any existing behavior (look ma, no test cases changed).

How this works:

sinon.match is not a matching function like samsam.match, it's a matcher factory function. It is therefore now exported as a new API samsam.createMatcher.
samsam.createMatcher still exposes all sinon.match.* APIs, so this can be imported by Sinon and shouldn't require any further changes.
samsam.match is kept as is and all sinon.match.* functions are assigned to it. This means that sinon.match.string and samsam.match.string will be the same function.
samsam.deepEqual is integrated with samsam.match through the deepEqual.use(...) API. For the sinon matchers to work, a small code change was needed to check if the second argument is a matcher. ~~I believe~~ This can be simplified by changing deepEqual calls in sinon-match by swapping the arguments (deepEqual(actual, expectation) instead of deepEqual(expectation, actual)).

The last point is actually optional. We could decide that calling libraries like referee could integrate deepEqual and match. Opinions anyone?

How to verify - mandatory

Check out this branch
npm install
npm t

Checklist for author

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

fearphage

I had a few questions and suggestions. I also didn't see the value in a few of the changes.

fearphage · 2018-12-06T17:37:56Z

lib/deep-equal.js

                return obj1 === obj2;
            }
-            return obj1.test(obj2);
+            return obj2.test(obj1);


Were any changes in this file necessary? It seems like you just switched variable names, but I'm not seeing the why.

Good question. It's easier to see if you review each commit individually. I had to introduce this check on top of the previous one to make these new test cases pass:

samsam/lib/sinon-match-integration.test.js

Lines 50 to 62 in 2760e75

context("samsam.deepEqual", function() {

it("returns true if matching boolean", function() {

assert.isTrue(samsam.deepEqual(false, match.bool));

});

it("returns true when matching nested matcher", function() {

assert.isTrue(samsam.deepEqual({ x: 1 }, { x: match.number }));

});

it("returns false when not matching nested matcher", function() {

assert.isFalse(samsam.deepEqual({ x: 1 }, { x: match.string }));

});

});

It then occurred to me, that we can have the same simple solution as before with the tests still passing if we swapped the variable names.

lib/samsam.js

lib/sinon-match-integration.test.js

lib/samsam.js

fearphage · 2018-12-06T17:46:28Z

lib/sinon-match.js

@@ -54,7 +54,7 @@ function isMatcher(object) {
    return isPrototypeOf(matcher, object);
 }

-function matchObject(expectation, actual) {
+function matchObject(actual, expectation) {


Why was this change made? Was it necessary?

No, it wasn't strictly necessary, but I had to change the order of actual / expected in other places and I thought it should be consistent.

mroderick

sinonMatch was created as stepping stone to get us to where we needed to go. This PR tidies up everything nicely, but still leaves that behind. Would you mind renaming the file to something that no longer references sinon?

mantoni · 2018-12-07T09:41:58Z

I renamed sinon-match to matcher and changed variable and test case names accordingly.

mantoni · 2018-12-07T09:44:03Z

If we're happy, we can either just squash the whole PR, or I can rebase to clean it up a little – if you think some refactoring history makes it easier to follow.

mantoni · 2018-12-07T10:58:43Z

Kind of important question: The behavior of deepEqual may be different now, because of the matcher awareness. Do we consider this a breaking change?

* Integrate sinon.match with samsam.match * Integrate samsam.match with samsam.deepEqual * Simplify deepEqualCyclic by swapping actual and expected * Order exported functions alphabetically * More consistent formatting in test case * Group positive and negative tests * Rename sinon-match.js to matcher.js

fearphage · 2018-12-07T17:01:35Z

Do we consider this a breaking change?

I thought changing the order of the params was a breaking change, no?

mantoni · 2018-12-07T17:03:10Z

@fearphage Yes. It became very obvious when I tried to integrate with Sinon :) (see here)

fearphage · 2018-12-07T21:08:37Z

@mantoni Does switching the order of the params here provide some value that I'm not seeing? It seems to make more work and provide zero additional value from my external point of view. Please feel free to correct me though.

mantoni · 2018-12-07T21:18:12Z

@fearphage The only value that I can see myself is consistency with referee.equals semantics. During development I got confused because sometimes the expectation was passed in first (on the Sinon side) and sometimes last (on the samsam side). I can revert this change and we keep it as it was. I'd prefer it to be consistent though.

I agreed with Morgan that we'll be releasing these changes as a major anyway, regardless of the parameter ordering. Otherwise we might run into issues when integrating this with Sinon. Also, the matcher awareness of samsam.deepEqual is potentially breaking.

All of this work was merged into next and can still be easily changed. However, I've already integrated the changes with Sinon locally. It looks like it's going to be a smooth transition :)

fearphage · 2018-12-08T01:06:19Z

Ok, that makes sense.

coveralls · 2018-12-08T06:37:21Z

Pull Request Test Coverage Report for Build 192

0 of 0 changed or added relevant lines in 0 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.02%) to 96.887%

Files with Coverage Reduction	New Missed Lines	%
match.js	2	97.52%

Totals
Change from base Build 186:	0.02%
Covered Lines:	438
Relevant Lines:	450

💛 - Coveralls

* Integrate sinon.match with samsam.match * Integrate samsam.match with samsam.deepEqual * Simplify deepEqualCyclic by swapping actual and expected * Order exported functions alphabetically * More consistent formatting in test case * Group positive and negative tests * Rename sinon-match.js to matcher.js

mantoni added 2 commits December 6, 2018 16:55

Integrate sinon.match with samsam.match

964f8bc

Integrate samsam.match with samsam.deepEqual

df8dbc9

mantoni requested a review from mroderick December 6, 2018 16:28

Simplify deepEqualCyclic by swapping actual and expected

2760e75

fearphage requested changes Dec 6, 2018

View reviewed changes

mantoni added 3 commits December 6, 2018 19:33

Order exported functions alphabetically

b401218

More consistent formatting in test case

a8371b9

Group positive and negative tests

99c796c

mroderick requested changes Dec 7, 2018

View reviewed changes

Rename sinon-match.js to matcher.js

dcc0e30

mantoni force-pushed the integrate-with-sinon-match branch from 6d840c8 to dcc0e30 Compare December 7, 2018 09:40

mroderick approved these changes Dec 7, 2018

View reviewed changes

mantoni merged commit b61c686 into next Dec 7, 2018

mantoni deleted the integrate-with-sinon-match branch December 7, 2018 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate with sinon match #48

Integrate with sinon match #48

mantoni commented Dec 6, 2018 •

edited

Loading

fearphage left a comment

fearphage Dec 6, 2018

mantoni Dec 6, 2018

fearphage Dec 6, 2018

mantoni Dec 6, 2018

mroderick left a comment

mantoni commented Dec 7, 2018

mantoni commented Dec 7, 2018

mantoni commented Dec 7, 2018

fearphage commented Dec 7, 2018

mantoni commented Dec 7, 2018

fearphage commented Dec 7, 2018

mantoni commented Dec 7, 2018

fearphage commented Dec 8, 2018

coveralls commented Dec 8, 2018 •

edited

Loading

	context("samsam.deepEqual", function() {
	it("returns true if matching boolean", function() {
	assert.isTrue(samsam.deepEqual(false, match.bool));
	});

	it("returns true when matching nested matcher", function() {
	assert.isTrue(samsam.deepEqual({ x: 1 }, { x: match.number }));
	});

	it("returns false when not matching nested matcher", function() {
	assert.isFalse(samsam.deepEqual({ x: 1 }, { x: match.string }));
	});
	});

Integrate with sinon match #48

Integrate with sinon match #48

Conversation

mantoni commented Dec 6, 2018 • edited Loading

How to verify - mandatory

Checklist for author

fearphage left a comment

Choose a reason for hiding this comment

fearphage Dec 6, 2018

Choose a reason for hiding this comment

mantoni Dec 6, 2018

Choose a reason for hiding this comment

fearphage Dec 6, 2018

Choose a reason for hiding this comment

mantoni Dec 6, 2018

Choose a reason for hiding this comment

mroderick left a comment

Choose a reason for hiding this comment

mantoni commented Dec 7, 2018

mantoni commented Dec 7, 2018

mantoni commented Dec 7, 2018

fearphage commented Dec 7, 2018

mantoni commented Dec 7, 2018

fearphage commented Dec 7, 2018

mantoni commented Dec 7, 2018

fearphage commented Dec 8, 2018

coveralls commented Dec 8, 2018 • edited Loading

Pull Request Test Coverage Report for Build 192

💛 - Coveralls

mantoni commented Dec 6, 2018 •

edited

Loading

coveralls commented Dec 8, 2018 •

edited

Loading