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 an object property with getter/setter doesn't work #1018

Closed
elado opened this issue Mar 31, 2016 · 8 comments
Closed

Stub of an object property with getter/setter doesn't work #1018

elado opened this issue Mar 31, 2016 · 8 comments

Comments

@elado
Copy link

elado commented Mar 31, 2016

If there's a method that was defined with defineProperty with a value it works, but not with get/set.

Error is "Attempted to wrap undefined property fnGetterSetter as function".

const sinon = require('sinon')

function factory() {
  let object = {}

  Object.defineProperty(object, 'fnValue', { configurable: true, value() { return 'fnValue' } })
  let fnGetterSetter = () => 'fnGetterSetter'
  Object.defineProperty(object, 'fnGetterSetter', {
    get() { return fnGetterSetter },
    set(newValue) { fnGetterSetter = newValue }
  })

  return object
}

let o1 = factory()

console.log('o1.fnValue: ' + o1.fnValue()) // => "fnValue"
console.log('o1.fnGetterSetter: ' + o1.fnGetterSetter()) // => "fnGetterSetter"
o1.fnGetterSetter = () => 'new fn'
console.log('o1.fnGetterSetter: ' + o1.fnGetterSetter()) // => "new fn"

let o2 = factory()

sinon.stub(o2, 'fnValue').returns('stubbed fnValue')
console.log('o2.fnValue: ' + o2.fnValue()) // works, => "stubbed fnValue"

sinon.stub(o2, 'fnGetterSetter').returns('stubbed fnGetterSetter') // fails - Attempted to wrap undefined property fnGetterSetter as function
console.log('o2.fnValue: ' + o2.fnGetterSetter())

Runnable code: https://tonicdev.com/56e1cbdb104098110026e06c/56fd90d539b35a13000ede79

@mantoni
Copy link
Member

mantoni commented Jul 6, 2016

Already fixed in #1059. sinon@next will be updated tonight, but you can also depend on a commit as a workaround. Also reported in #1036.

@fatso83
Copy link
Contributor

fatso83 commented Jul 7, 2016

npm show sinon dist-tags.next
2.0.0-pre.2

@fatso83 fatso83 closed this as completed Jul 7, 2016
@hedgeday
Copy link

hedgeday commented Sep 1, 2016

Hey, can someone confirm whether this works with the new version?

I'm running @elado's code snippet with sinon@2.0.0-pre.2 and I'm still seeing the error Attempted to wrap undefined property fnGetterSetter as function.

@rclark
Copy link

rclark commented Sep 10, 2016

@mantoni Was your commit dropped somehow? You point to f7cba98 as the solution to the problem, but I can't find that commit in master (could be failing at git-fu), and the problem is not fixed when I install sinon in either of these ways:

A gist showing what I'm still seeing in either version

@mantoni
Copy link
Member

mantoni commented Sep 11, 2016

@rclark I can find the commit in master when I go through the commit history, and the important change is still in current HEAD:

iterator.call(context, k, target);

So I'm wondering if this was either re-introduced by some other change or if there is another issue.

@rclark
Copy link

rclark commented Sep 11, 2016

Thanks for tracking that down @mantoni -- I would have to agree that either there was a regression or the commit you've indicated solved something other than this specific issue. It looks to me (having never worked on sinon before) like the issue is entirely in the wrap-method.js script.

In master, the problems starts here

var methodDesc = (typeof method === "function") ? {value: method} : method;
, where methodDesc is defined as an object with a value property.

Then, here:

wrappedMethod = wrappedMethodDesc[types[i]];
the descriptor for the getter/setter only property that you intend to mock is asked for a .value property, which returns undefined.

Now the checkWrappedMethod() function will throw the error being reported, since it has been passed undefined:

error = new TypeError("Attempted to wrap " + (typeof wrappedMethod) + " property " +
.

A very simple failing test case indicating that the issue is not fixed:

var sinon = require('sinon');

var val;
var obj = {
  get foo() { return val; },
  set foo(v) { val = v; }
};

var stub = sinon.stub(obj, 'foo');

@mantoni
Copy link
Member

mantoni commented Sep 12, 2016

Thanks for tracking it down. Your research tells me that this is different issue than the one reported here, however with the same outcome. Reopening the issue.

Can you send a pull request adding the failing test? Or even have a stab at fixing it?

@mantoni mantoni reopened this Sep 12, 2016
@mroderick
Copy link
Member

I've created a repository of demos with the following branches:

  • master using sinon@v2.0.0-pre.3
  • fail-in-1.17 using sinon@1.17.6

With sinon@v2.0.0-pre.3 both spy() and stub() works for accessor methods
With sinon@1.17.6, only stub() works for acessor methods. spy() fails with the following error message:

throw error;
                    ^

TypeError: Attempted to wrap undefined property myProperty as function

You can try it out yourself in this branch: https://github.com/mroderick/sinon-demo-getter-setter/tree/fail-in-1.17

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

6 participants