Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Samsam 3 #49

Merged
merged 6 commits into from Dec 9, 2018
Merged

Samsam 3 #49

merged 6 commits into from Dec 9, 2018

Conversation

mantoni
Copy link
Member

@mantoni mantoni commented Dec 7, 2018

Purpose (TL;DR) - mandatory

This PR contains the set of changes for samsam 3 that have been collected on the next branch.

The changes should be downward compatible, but given the scope, I'd like to release this as a major. This will also allow to make a clean switch in Sinon.

How to verify - mandatory

  1. Check out this branch
  2. npm install
  3. npm t

Checklist for author

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

mroderick and others added 5 commits December 7, 2018 15:32
This has not been exported yet, currently, we're just wiring up the tests
* 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 mantoni added enhancement semver:major changes will cause a new major version labels Dec 7, 2018
@mantoni
Copy link
Member Author

mantoni commented Dec 7, 2018

This might need a little more work. A first attempt to integrate this with Sinon causes 6 tests to fail. We can keep this PR open for discussion and tracking the changes.

@mantoni
Copy link
Member Author

mantoni commented Dec 7, 2018

It turns out that Sinon calls samsam.deepEqual(expected, actual) instead of (actual, expected). With matcher integration, deepEqual is not symetric, so order matters. Swapping the arguments in all occurences of deepEqual in Sinon makes all tests pass.

I'm surprised how clean this went. I replace the entire lib/sinon/util/core/deep-equal.js with samsam.deepEqual and lib/sinon/match.js with samsam.createMatcher.

Given that referee uses (actual, expected) semantics, I'd like to keep this implementation here as is. It's a major release anyway and the changes in Sinon are straight forward.

Copy link
Member

@mroderick mroderick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

BREAKING: This brings the samsam.match implementation closer to how
sinon.match behaves. Nested object where recursively compared using
samsam.match itself. This change uses samsam.deepEqual for recursive
checks instead. If matching logic is desireable on nested objects,
samsam.createMatcher can be used which is now recognized.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 203

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+1.8%) to 96.891%

Files with Coverage Reduction New Missed Lines %
match.js 2 97.54%
deep-equal.js 3 95.98%
Totals Coverage Status
Change from base Build 185: 1.8%
Covered Lines: 439
Relevant Lines: 451

💛 - Coveralls

@mantoni mantoni merged commit 73908b6 into master Dec 9, 2018
@mantoni mantoni deleted the next branch December 9, 2018 14:24
@mantoni
Copy link
Member Author

mantoni commented Dec 9, 2018

Released in 3.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement semver:major changes will cause a new major version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants