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

Sinon no longer throws when stubbing non-existent properties #1508

Closed
schreifels opened this issue Aug 2, 2017 · 6 comments
Closed

Sinon no longer throws when stubbing non-existent properties #1508

schreifels opened this issue Aug 2, 2017 · 6 comments

Comments

@schreifels
Copy link

schreifels commented Aug 2, 2017

  • Sinon version : 2.3.7
  • Environment : OSX, Node 6 (Jest)
  • Other libraries you are using: Mocha

What did you expect to happen?

When calling sinon.stub(obj, 'nonexistentProperty');, I would expect it to throw an error, e.g. "Attempted to wrap undefined property".

What actually happens

It does not throw (nor does it create a stub on the object at that property).

How to reproduce

Interestingly, if you use the deprecated 3rd argument to sinon.stub, it does still work, as shown in this example:

'use strict';

const assert=require('assert');
// const sinon = require('./lib/sinon'); // if checking out the sinon project
const sinon = require('sinon');

describe('sinon', () => {
  context('with the standard syntax', () => {
    it('throws when stubbing a non-existent property', () => {
      const obj = {};

      // FAIL: expected [Function] to throw an error
      assert.throws(() => {
        sinon.stub(obj, 'nonexistentProperty');
      });
    });
  });

  context('with the deprecated syntax', () => {
    it('throws when stubbing a non-existent property', () => {
      const obj = {};

      // PASS
      assert.throws(() => {
        sinon.stub(obj, 'nonexistentProperty', () => {});
      }, 'Attempted to wrap undefined property');
    });
  });
});

(I would have expected both tests to pass.)

@fatso83 fatso83 self-assigned this Aug 8, 2017
@fatso83
Copy link
Contributor

fatso83 commented Aug 8, 2017

Thanks to your test I was able to automate the search for this using git bisect. This behavior has been consistent throughout the entire 2.x series and was introduced by PR #1297, and the change is mentioned in the list of changes.

From the discussion, I am not totally sure how it was decided we should go from throwing on non-existent properties to support stubbing them, but maybe @lucasfcosta remembers more? Anyway, this change was intentional and can't be reversed now (if so, use 1.x). The only thing I feel is an issue, is that this is a breaking change (as you show), that is not covered in the migration guide from 1.x to 2.x. That should be remedied.

@fatso83 fatso83 closed this as completed Aug 8, 2017
@lucasfcosta
Copy link
Member

lucasfcosta commented Aug 8, 2017

Hi @schreifels, thanks for your issue!

As @fatso83 commented, this has been an intentional change in order to allow stubbing values, if I'm not mistaken.

Also, the reason why you are not getting a stub into the object itself when doing this is because you are not creating any behavior for it. By behavior I mean creating a fake getter or setter or even changing the descriptor's value by using the value behavior. However, IMO this is something that should be changed.

When creating stubs, even when we're not creating stubs for methods, they must replace the original property on the object.


TL;DR:

  • IMO we should not throw for undefined properties because we do support stubbing them
  • IMO we should indeed replace properties for the stubs being created for them even though they're not functions

I have this change ready right here and all tests pass. I will reopen this issue to get feedback on this new behavior and if everyone agrees I can just push this change and open a PR, it would take me only a few minutes.

@lucasfcosta lucasfcosta reopened this Aug 8, 2017
@schreifels
Copy link
Author

This seems to reasonable to me, thanks for the explanation @lucasfcosta and @fatso83!

@fatso83
Copy link
Contributor

fatso83 commented Aug 23, 2017

This is related to #1537, and we should close this issue if the conclusion is to fix that regression behavior.

@fatso83 fatso83 removed their assignment Aug 23, 2017
fatso83 added a commit to fatso83/git-bisect-scripts that referenced this issue Sep 11, 2017
@fatso83
Copy link
Contributor

fatso83 commented Sep 11, 2017

@lucasfcosta I have now digged down in this and it turns out we have never supported stubbing undefined properties. It always resulted in an exception, up until commit 278c2ce (which is when you added getters/setters). After this commit we still don't support it in a way, as no stub is created, but this is a regression and a bug in any case.

Now knowing this, this directly affects #1552 and friends, and I am now inline with the rest of the team that we should not support this in any way (sandbox or not). The bug introduced in 278c2ce should be fixed in any case.

Full testcase for verification:
https://github.com/fatso83/git-bisect-scripts/blob/master/sinon-1552-1508-git-bisect-script.sh

fatso83 added a commit to fatso83/sinon that referenced this issue Sep 12, 2017
This was a longer discussion that can be found in the
issues sinonjs#1508, sinonjs#1552 and sinonjs#1537 - and internally amongst the core team.

Support for stubbing props using get()/set() was added in sinonjs#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 added a commit to fatso83/sinon that referenced this issue Sep 13, 2017
This was a longer discussion that can be found in the
issues sinonjs#1508, sinonjs#1552 and sinonjs#1537 - and internally amongst the core team.

Support for stubbing props using get()/set() was added in sinonjs#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 added a commit that referenced this issue Sep 18, 2017
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
Copy link
Contributor

fatso83 commented Sep 18, 2017

Fixed in #1557. To be released shortly.

@fatso83 fatso83 closed this as completed Sep 18, 2017
franck-romano pushed a commit to franck-romano/sinon that referenced this issue Oct 1, 2019
This was a longer discussion that can be found in the
issues sinonjs#1508, sinonjs#1552 and sinonjs#1537 - and internally amongst the core team.

Support for stubbing props using get()/set() was added in sinonjs#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants