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

Upgrade to samsam 3 #1955

Merged
merged 2 commits into from Dec 10, 2018
Merged

Upgrade to samsam 3 #1955

merged 2 commits into from Dec 10, 2018

Conversation

mantoni
Copy link
Member

@mantoni mantoni commented Dec 7, 2018

Purpose (TL;DR) - mandatory

WORK IN PROGRESS! DO NOT MERGE

This PR integrates the changes from sinonjs/samsam#49 with Sinon. Since samsam 3 is not yet release, the build is failing.

See intructions below on how to verify locally.

Changes

  • lib/sinon/util/core/deep-equal.js is replaced with samsam.deepEqual.
  • lib/match.js now simply exposes samsam.createMatcher.
  • Removed obsolete files:
    • lib/sinon/util/core/every.js
    • lib/sinon/util/core/iterable-to-string.js
    • lib/sinon/util/core/typeOf.js
  • Removed obsolete dependencies:
    • lodash.get
    • type-detect
  • Parameter ordering of samsam.deepEqual has changed from (expected, actual) to (actual, expected)

How to verify - mandatory

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

Checklist for author

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

@mroderick
Copy link
Member

I love PRs like these!

@coveralls
Copy link

coveralls commented Dec 9, 2018

Pull Request Test Coverage Report for Build 2766

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 22 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.7%) to 94.517%

Files with Coverage Reduction New Missed Lines %
sinon/mock.js 1 96.64%
sinon/spy.js 21 90.05%
Totals Coverage Status
Change from base Build 2758: -0.7%
Covered Lines: 1663
Relevant Lines: 1729

💛 - Coveralls

@mantoni mantoni changed the title [WIP] Upgrade to samsam 3 (next) Upgrade to samsam 3 Dec 9, 2018
@mantoni
Copy link
Member Author

mantoni commented Dec 9, 2018

Samsam 3 was released and this is passing all tests in all environments. Ready for review.

It was required to update formatio and nise to avoid having two versions of samsam in the "production" dependency tree.

We could also wait for the next major version of referee to only have samsam 3 in the development dependency tree. However, all tests work fine with the current version too.

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.

I have verified this locally

Copy link
Member

@fearphage fearphage left a comment

Choose a reason for hiding this comment

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

lib/sinon/match.js seems like a candidate for removal.

lib/sinon/call.js Outdated Show resolved Hide resolved
lib/sinon/match.js Outdated Show resolved Hide resolved
test/util/core/deep-equal-test.js Outdated Show resolved Hide resolved
@mantoni
Copy link
Member Author

mantoni commented Dec 10, 2018

@fearphage Thank you for reviewing. You're absolutely right. I kept the tests and match.js file initially to show that these changes do not break any tests. Now that the change is complete and all tests pass, I feel confident removing them.

@mantoni mantoni merged commit c16bce4 into master Dec 10, 2018
@mantoni mantoni deleted the samsam-v3 branch December 10, 2018 07:50
franck-romano pushed a commit to franck-romano/sinon that referenced this pull request Oct 1, 2019
* Upgrade to samsam 3

* Remove obsolete tests and match.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants