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

Remove support for stubbing undefined props #1557

Merged
merged 1 commit into from Sep 18, 2017

Conversation

Projects
None yet
3 participants
@fatso83
Contributor

fatso83 commented Sep 12, 2017

Purpose

This removes support for stubbing undefined properties, aligning the behavior with how sandbox stubbing works. The reasoning behind not allowing this is

stubbing non-existing own properties is likely to lead to mistakes in tests, where the test passes because the author mistyped the name of the existing property they were aiming to stub

Background

This was a longer discussion that can be found in the
issues #1508, #1552 and #1537 - and internally amongst the core team.

Support for stubbing props using get()/set() was added in #1237
and also worked for undefined properties.

This type of behavior (stubbing undefined props) has been
explicitly disabled for sandboxes, so in order for Sinon to
behave in a consistent way, we should do the same here.

This change will bring normal stubs back to how it has
used to be historically, and will make sandbox stubs
and normal sinon stubs behave the same.

It might break things for people relying on the behavior
that has been present since Sinon 2.0, but it should
make things more reliable going forward.

@fatso83 fatso83 requested a review from mroderick Sep 12, 2017

@fatso83

This comment has been minimized.

Show comment
Hide comment
@fatso83

fatso83 Sep 12, 2017

Contributor

The alternative to removing this functionality is the inverse of this:

  • remove the restriction introduced for sandboxes in 3.1 (#1537)
  • fix the stub that is being produced (the reason for #1552 is that the stub is only produced at the point we call set(...) - this could be done earlier)
Contributor

fatso83 commented Sep 12, 2017

The alternative to removing this functionality is the inverse of this:

  • remove the restriction introduced for sandboxes in 3.1 (#1537)
  • fix the stub that is being produced (the reason for #1552 is that the stub is only produced at the point we call set(...) - this could be done earlier)
@mroderick

This comment has been minimized.

Show comment
Hide comment
@mroderick

mroderick Sep 13, 2017

Member

You can count on me to pretty much always err on the side of stricter use of languages and apis, and this is another one of those cases.

I vote 👍

Member

mroderick commented Sep 13, 2017

You can count on me to pretty much always err on the side of stricter use of languages and apis, and this is another one of those cases.

I vote 👍

Remove support for stubbing undefined props
This was a longer discussion that can be found in the
issues #1508, #1552 and #1537 - and internally amongst the core team.

Support for stubbing props using get()/set() was added in #1237
and also worked for undefined properties.

This type of behavior (stubbing undefined props) has been
explicitly disabled for sandboxes, so in order for Sinon to
behave in a consistent way, we should do the same here.

This change will bring normal stubs back to how it has
used to be historically, and will make sandbox stubs
and normal sinon stubs behave the same.

It might break things for people relying on the behavior
that has been present since Sinon 2.0, but it should
make things more reliable going forward.
@mroderick

This comment has been minimized.

Show comment
Hide comment
@mroderick

mroderick Sep 18, 2017

Member

@sinonjs/sinon-core any objections to merging this?

Member

mroderick commented Sep 18, 2017

@sinonjs/sinon-core any objections to merging this?

@fatso83

This comment has been minimized.

Show comment
Hide comment
@fatso83

fatso83 Sep 18, 2017

Contributor

@mroderick Only the objections raised in this other issue. Thought it might be a nuisance to have two major releases in short succession, but it's not a big thing, so you might as well. There's not like there is a PR out there for the factory function refactor.

Contributor

fatso83 commented Sep 18, 2017

@mroderick Only the objections raised in this other issue. Thought it might be a nuisance to have two major releases in short succession, but it's not a big thing, so you might as well. There's not like there is a PR out there for the factory function refactor.

@mroderick

This comment has been minimized.

Show comment
Hide comment
@mroderick

mroderick Sep 18, 2017

Member

@mroderick Only the objections raised in this other issue. Thought it might be a nuisance to have two major releases in short succession, but it's not a big thing, so you might as well. There's not like there is a PR out there for the factory function refactor.

I don't see having several MAJOR releases as a big deal, as long as their changelogs are small at least :)

Member

mroderick commented Sep 18, 2017

@mroderick Only the objections raised in this other issue. Thought it might be a nuisance to have two major releases in short succession, but it's not a big thing, so you might as well. There's not like there is a PR out there for the factory function refactor.

I don't see having several MAJOR releases as a big deal, as long as their changelogs are small at least :)

@fatso83 fatso83 merged commit 34fc2ba into sinonjs:master Sep 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fatso83 fatso83 deleted the fatso83:fix-1508 branch Sep 18, 2017

@Glogo

This comment has been minimized.

Show comment
Hide comment
@Glogo

Glogo Apr 23, 2018

This may be unrelated to this issue, but for those that have the same problem as me with version 4 now.
Instead of doing this:

sinon.stub(obj, 'yourMethod', () => Promise.resolve('whatever'))

you do:

sinon.stub(obj, 'yourMethod')
  .returns(() => Promise.resolve('whatever'));

Glogo commented Apr 23, 2018

This may be unrelated to this issue, but for those that have the same problem as me with version 4 now.
Instead of doing this:

sinon.stub(obj, 'yourMethod', () => Promise.resolve('whatever'))

you do:

sinon.stub(obj, 'yourMethod')
  .returns(() => Promise.resolve('whatever'));
@fatso83

This comment has been minimized.

Show comment
Hide comment
@fatso83

fatso83 Apr 23, 2018

Contributor

@Glogo, actually we have had promise support for several versions, so it is even shorter:

sinon.stub(obj, 'yourMethod').resolves('whatever');
Contributor

fatso83 commented Apr 23, 2018

@Glogo, actually we have had promise support for several versions, so it is even shorter:

sinon.stub(obj, 'yourMethod').resolves('whatever');
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment