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

Stub of getters & setters not working with sandbox #781

Closed
vitalets opened this issue Jul 14, 2015 · 10 comments
Closed

Stub of getters & setters not working with sandbox #781

vitalets opened this issue Jul 14, 2015 · 10 comments

Comments

@vitalets
Copy link

When I stub getter using sinon.stub() everything works fine.
But when I use sandbox.stub() it does not work.
Example:

var o = {
    get foo() { return 'foo' },
    get bar() { return 'bar' }
};

// via sinon
sinon.stub(o, 'foo', { get: function () { return 'foo stubbed' }});
console.log(o.foo); // foo stubbed

// via sandbox
var sandbox = sinon.sandbox.create();
sandbox.stub(o, 'bar', { get: function () { return 'bar stubbed' } });
console.log(o.bar); // Uncaught TypeError: Cannot set property bar of #<Object> which has only a getter

jsfiddle: http://jsfiddle.net/exzc5m3e/

Relates to: #692

Is it possible to support getters & setters in sandboxes?

Thanks!

@mroderick
Copy link
Member

Thank you for reporting this.

Is it possible to support getters & setters in sandboxes?

That's a good question. We'll need more investigation to determine if that's possible.

@mikedaly
Copy link

mikedaly commented Nov 5, 2015

I recently added sinon to a project, to augment Jasmine, specifically because it supported mocking properties. I would use this feature as soon as it was available.

@nunofgs
Copy link

nunofgs commented Dec 17, 2015

I just hit this issue as well.

@kuraga
Copy link

kuraga commented Jan 8, 2016

The same issue

@i2infinity
Copy link

Just ran into this issue

@fatso83
Copy link
Contributor

fatso83 commented Mar 10, 2016

As the label says: help is much appreciated. If someone deeply craves this feature do know that we are open for pull requests ;-)

@mmoss
Copy link

mmoss commented Mar 13, 2016

@fatso83 I'll give adding some unit tests and fixing this a go... fingers crossed I can get it working 😄

@mantoni
Copy link
Member

mantoni commented Mar 13, 2016

@mmoss Make sure to pull again. The unit tests have been converted to Mocha.

@lucasfcosta
Copy link
Member

Hi everyone, I have been investigating this and I've found inconsistencies with the request and how it currently works.

According to the docs, the sandbox.stub method can be used to stub both values and functions:

The sandbox stub method can also be used to stub any kind of property.

Due to this fact it's not viable to make it accept property descriptors as values, because then we wouldn't be able to know whether the user wants to pass a property descriptor or an simple object to replace that property. For example:

var myModule = {
  doubles: {
    get: function get(x) {
      return x * 2;
    }
  }
};

var sandbox = sinon.sandbox.create();

// If we were able to do this, how can we know if the third argument is a property descriptor or if it should simply replace the property `get`?
sandbox.stub(myModule, 'doubles', {
  get: function get() {
    return -1
  }
});

// Now should this return -1 or should this return an object with a `get` function?
console.log(myModule.doubles);

I think that by adding this feature we would end up causing confusion, even though we check for the names in the object passed, someone may have names which end up clashing with property that do exist on property descriptors. We can't just arbitrarily infer what the user wants.

I can think of three ways to solve this problem:

  • Add another method called stubDescriptor or something like that (IMO this would be bad 👎 )
  • Allow stubbing functions only (very breaking change and reduces the number of use cases for sandboxes 👎 )
  • Add another argument which allows the user to pass a property descriptor and then use it whenever it is present (Best choice IMO 👍 )

Let me know what you'd like and I'll make a PR for that. Also, if I misunderstood something or said anything wrong please let me know.

Thanks!

@mroderick
Copy link
Member

This is implemented with #1416

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

10 participants