Feature: Unify comparison logic with matcher support in Sinon projects

[mantoni]

Motivation:

It would be totally awesome if this was possible:

const { assert, match } = require('@sinonjs/referee');

const object = { my: 'thingy' };

assert.equals(object, { my: match.string });

Suggestion

A newly exposed match function could essentially behave like sinon.match. I think the matchers actually belong in samsam and the deepEqual logic in samsam and sinon should be unified with matcher support. This way sinon, @sinonjs/referee and @sinonjs/referee-sinon can expose it and have the same semantics everywhere.

It would then be possible to pass the result of calling match({ some: 42 }) to both, assert.equals and spy.calledWith.

[mroderick]

That would be very cool indeed.

[mroderick]

This is related to sinonjs/sinon#1764 (comment), where we discuss the responsibilities of match and deepEqual.

I'll try to do some analysis of this and make a proposal for a way forward that can address most of these.

[mroderick]

@mantoni I've opened sinonjs/sinon#1853 to fix a problem with cyclic objects and deepEqual.

If we can get this merged, then I think we'll have a better starting point for future improvements

[mroderick]

I took a stab at consolidating the deepEqual in sinon and samsam.

1. sinonjs/samsam@master...mroderick:improve-deep-equal
2. sinonjs/sinon@master...mroderick:hollow-out-deepEqual

To verify

1. Check out the samsam branch
2. npm test
3. Observe that there are more tests, all green
4. npm link
5. Check out the sinon branch
6. npm link @sinonjs/samsam
7. npm test
8. Observe that there are still tests for deepEqual, and that all tests are still green.

Thoughts

• The easy parts are over, I think the rest of the challenges are around using match
• I think that we need to untangle match from deepEqual, maybe I should do that first, to make this easier
• Some things just can't be mapped between them, sinon does not allow comparing arguments with Object or Array. samsam does.
• Where there are conflicts, I think we should go with what samsam does, as that seems to be much better tested.
• We should make new major versions of both sinon and samsam as there is likely to be things we haven't yet tested for

What are your thoughts @mantoni?

[mantoni]

This looks very good, thank you! Here are my thoughts so far:

• The a === b check could also move into samsam, maybe as the first thing. This leaves only the matcher logic in sinon.
• When using the matcher aware sinon deepEquals implementation, it now runs recursively through the tree using samsam.deepEqual and then does the same recursively for each value. I think the way forward is to move Sinons deepEqual.use implemention onto Samsams deepEqual.
• I agree that samsam has the better tests and it seems right to be able to compare arguments with Array. Here is a quick test that shows an issue with Sinon here:

var fake = sinon.fake()
(function () { fake(arguments) })(1, 2, 3)

sinon.assert.calledWith(fake, [1, 2, 3]) // fails
sinon.assert.calledWith(fake, { 0: 1, 1: 2, 2: 3 }) // also fails, so cannot verify

samsam.deepEqual(fake.firstCall.args[0], [1, 2, 3]) // true

• Yes, this whole thing should be released as a new major. With the large user base we have now, I'm sure the nuanced differences in the implementations will break someones tests.

The difficult part will be to get sinon.match extracted. If we want to move that to samsam, there are quite a few "core" dependencies there. The easiest would be to duplicate them for now. Otherwise we'd have to extract a @sinonjs/core module that exposes the common utilities. I find things with the name "core" or "common" a little smelly though 🤔

[mroderick]

I've added sinonjs/samsam#33, which makes it easier to further improve samsam.

[mroderick]

I think we will want to keep assert.equals as it is:

Compares actual to expected property by property. If the property count does not match, or if any of actual‘s properties do not match the corresponding property in expected, the assertion fails. Object properties are verified recursively.

From https://github.com/sinonjs/referee/blob/master/docs/index.md#equals

If you agree that this is the desired state, then I'll to further improve samsam to keep that goal in mind.

[mroderick]

The difficult part will be to get sinon.match extracted. If we want to move that to samsam, there are quite a few "core" dependencies there. The easiest would be to duplicate them for now. Otherwise we'd have to extract a @sinonjs/core module that exposes the common utilities. I find things with the name "core" or "common" a little smelly though 🤔

Time for a mono-repo?
https://lernajs.io

[mantoni]

Yes, I agree with the "mission statement" for assert.equals and I don't want to change it's semantics either. However, we'll have to slightly enhance it if we want matcher support to work the same in all frameworks. Or do you maybe have a different refactoring path in mind?

Time for a mono-repo?

Please no. We'd force the code, tooling and tests of referee, samsam, lolex, referee-sinon and nise on everyone who wants to contribut in any of those projects. I quite like small projects, because they come with less weight by themselves and are easier to use in different contexts. Yes, I know, you can still depend on individual projects in a mono-repo. But it's not becoming easier to maintain things. It's just easier to avoid difficult decisions like the one at hand. The mono-repo tooling is also another layer in the release process that has be understood, learned and maintained.

[mroderick]

Yes, I agree with the "mission statement" for assert.equals and I don't want to change it's semantics either. However, we'll have to slightly enhance it if we want matcher support to work the same in all frameworks. Or do you maybe have a different refactoring path in mind?

Let me summarise my current understanding, so we can see if you have the same understanding

1. sinon has the best match implementation, we want it to replace samsam.match
2. assert.equals in referee should be upgraded to also support matchers
3. assert.match in referee should (continue to) use samsam.match

Is that roughly your understanding too?

[mantoni]

Yes, we have the same goal in mind. I though the path there would mean to essentially replace the internal deepEquals implementation of Sinon and Referee with the one provided by samsam. Is that what you're planing to do? Or do you want to keep the samsam.deepEquals implementation as is and have the matcher integration in Sinon and Referee separately?

[mroderick]

ok, then I think we have the same understanding.

A rough outline of the plan could be:

1. Improve samsam
   1. Improve samsam.deepEqual be good enough for use in all libraries
   2. Figure out how to port deepEqual.use to samsam in a meaningful way
   3. Replace samsam.match with implementation from sinon.match (which relies on deepEqual.use)
2. Update dependents
   • Replace sinon.match with samsam.match
   • Use updated samsam.match in referee.match
   • Expand referee.assert.equals to understand matchers, in the same way that sinon.calledWith and friends do
3. Bump major version on all affected libraries

We should probably expand the documentation for assert.equals considerably, to make it easier to understand what is going on, and also make it clear that it now supports matchers.

[mroderick]

The difficult part will be to get sinon.match extracted. If we want to move that to samsam, there are quite a few "core" dependencies there. The easiest would be to duplicate them for now. Otherwise we'd have to extract a @sinonjs/core module that exposes the common utilities. I find things with the name "core" or "common" a little smelly though 🤔

I had a look at extracting match tonight. I think you're right with your assessment, it's going to be tricky with all the dependencies.

In order to extract match we need to find a solution for these files:

lib/sinon/util/core
|-- function-name.js
`-- value-to-string.js

lib/sinon/var/
|-- array.js
|-- copy-prototype.js
|-- function.js
|-- object.js
`-- string.js

Instead of having drifting implementations of these (and other useful functions) I'd be ok with having a @sinonjs/commons package. Perhaps with rules like:

• Follows the Sinon compatibility and coding guidelines
• Only accepts code when it is needed by more than one of the other packages
• No side effects welcome! (only accepts pure functions)
• No platform specific functions
• All functions must have 100% test coverage (preferably using property based testing, using jsverify). I already do this at work, am happy to do much more of this.
• One function per file (nice side effect: even browserify can do tree shaking)
• JSDoc is probably a good thing, I don't want to maintain documentation for yet another library

I think we can do this, while still avoiding creating another meta package like https://github.com/busterjs/buster#core-modules-in-alphabetical-order

If you're happy with that approach, then I'll extract exactly those functions into a new repo and prepare a release that we can then wire back into sinon.

[mantoni]

That sounds like a decent plan. I like the strict set of rules you suggested. I don’t know about property based testing and jsverify yet, so there’s something new for me to learn. Let’s do it this way 🙂

[mroderick]

I've started working on this: sinonjs/commons#1

[fearphage]

I think this proposal is confusing and a bit magical. I find it confusing to assert that something is equal and all it does is a type check instead. That's not equivalence.

[mantoni]

@fearphage Yes, the above example is maybe too reduced. If I wanted to just assert the type of my I'd rather write assert.isString(object.my).

Consider something like this:

assert.equals(record, {
  id: match.number,
  name: "Max",
  role: "Developer"
});

[mantoni]

This is implemented in #76 and #78.