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

Make all properties non-enumerable in spies, stubs, mocks and fakes #1975

Merged
merged 1 commit into from Feb 18, 2019
Merged

Make all properties non-enumerable in spies, stubs, mocks and fakes #1975

merged 1 commit into from Feb 18, 2019

Conversation

ehmicky
Copy link
Contributor

@ehmicky ehmicky commented Feb 7, 2019

Purpose (TL;DR) - mandatory

Fix issue #1936 by making all properties non-enumerable in spies, stubs, mocks and fakes.

How to verify - mandatory

  1. Check out this branch
  2. npm install
  3. sinon.spy(), sinon.stub(), sinon.fake(), sinon.mock() should not return non-enumerable properties.

Very few properties had to be left because removing them was breaking the tests.

Also this is a breaking change (major version) because some users might rely on spy/stub/fake/mock properties being enumerable.

CircleCI on Node 10 fails but this does not seem related to this PR, as it failed on other commits/PRs.

Checklist for author

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

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2789

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 15 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.4%) to 94.118%

Files with Coverage Reduction New Missed Lines %
sinon/mock.js 1 96.62%
sinon/spy.js 2 89.59%
sinon/stub.js 3 91.02%
sinon/util/core/wrap-method.js 9 79.41%
Totals Coverage Status
Change from base Build 2788: -0.4%
Covered Lines: 1635
Relevant Lines: 1703

💛 - Coveralls

Copy link
Contributor

@fatso83 fatso83 left a comment

Choose a reason for hiding this comment

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

Very nice! This looks fine to me, as the diff is very understandable. Haven't tested it, though.

Not sure this change validates a test or not, as this should be a non-functional change.

@fatso83 fatso83 merged commit fc2a32a into sinonjs:master Feb 18, 2019
@mroderick
Copy link
Member

Thank you ❤️

papandreou added a commit to unexpectedjs/unexpected-sinon that referenced this pull request Feb 19, 2019
They aren't as of sinonjs/sinon#1975, which landed in sinon 7.2.4

Fixes #92
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