Cannot spy on property getters, and stubbed getters do not record calls

[RoystonS]

• Sinon version : 4.4.6
• Environment : Chrome 65.0.3325.162 on Mac
• Example URL : N/A
• Other libraries you are using: N/A

What did you expect to happen?

I'm attempting to stub out - and monitor - a property getter,
using the stubbing approach recommended in the documentation at http://sinonjs.org/releases/v4.4.6/stubs/#stubgetgetterfn

I was hoping that the resulting stub would report when the getter was actually invoked, but no such reports are logged.

(In fact, I don't actually want to stub the getter: I merely want to spy on it, but Sinon provides no property getter spy API right now afaik.)

What actually happens

I can successfully stub out the getter, and control what it does, but Sinon's invocation recording reports no calls.

(This is also related to issue #1545.)

I do understand that, for a stubbed property, there are really two sets of function calls going on - the getter and the setter - and so really there should be two sets of recordings going on. But right now, there are none: when stubbing a property, as recommended in the docs above, no recording happens.

How to reproduce

There's a small example at https://codesandbox.io/s/l5m9loy21l; output is in the console.

[mroderick]

Thank you for your contribution!

This is a curious issue. I too was under the impression that calls were being recorded, but something seems amiss. Your example shows the issue clearly 👍

I think this one will require some digging to correctly diagnose it. A good start would be to use git bisect and a custom test, to see if Sinon was ever able to do this.

[RoystonS]

I just found another bit of discussion about it in #1606, but if I read it right, that API was only in the Sinon 1.7 stream (pull #1205)?

The current status is a little confusing.

[RoystonS]

@mroderick, I can help out with this.

I'll take a look, as you suggest, to see if there was previous support for this.

[mroderick]

@mroderick, I can help out with this.

That would be great, thank you!

[RoystonS]

Support for getters + setters was introduced in commit 278c2ce, just before the v2.0.0 release. The get() and set() functions added to stubs merely override the get and set parts of the property descriptor, allowing custom get and set logic to be applied, and don't affect the recording ability of the stub.

Prior to that commit, attempts to stub out a property were met with Attempted to wrap undefined property XXX as function in wrap-method.js.

Property getter + setter stubbing was introduced in Sinon v1.14.0, in commit bb467e6 and was fixed up in commit 8dace57 for v1.17. That facility was lost sometime between there and v2, but I haven't tracked that down.

@lucasfcosta previously mentioned the lack of recording ability in issue #1392, but that was closed after a while, with no changes made.

---

The API approach taken with v1 was that the user specifically requested, at stubbing time, whether get+set were to be stubbed.

The API approach with v2 is that extra .get() and .set() functions were added to stub, but they only provide behaviour, not recording. They do return a stub object, but it's the same stub object they're called on.

It would be great to have both stubbing and spying supported on getters and setters, but it would probably require additional API.

I can see a couple of options initially:

1. Modify .get() and .set() to return their own new stub/spy objects, which provide their own recording. This captures the fact that the getter functions and setter functions are their own separate functions, with independently-configurable behaviour and recordings. e.g. you could record calls to get independently of set, and calls like .throws() could apply independently to the two. It could possibly also make the API slightly more consistent: stub.get().callsFake(fn) instead of stub.get(fn).

   The downsides would be that

   • the original stub on a property would basically be a useless shell (although it is right now, tbh), existing only as a factory for the get + set stubs
   • code currently depending on .get() and .set() returning the original stub object would get different behaviour.

   I'm not sure either of those are actually real problems.

2. Always have stub(obj, 'prop') replace getter and setter, and always record, onto the same stub. Very simple API. Downsides: you'd have a mixture of get + set calls intermixed. The getter calls and setter calls could be distinguished as some would have arguments and some wouldn't, but you wouldn't be able to configure, say, the getter to throw but the setter not to throw using the standard Sinon APIs.

[RoystonS]

Any opinions on the two API options I suggested, or any other suggestions?

[JakobJingleheimer]

Meantime, for those looking for a workaround, this is possible with a combination of stub.get/set + spy: #1545 comment-385262212

[fatso83]

@RoystonS If pressed, I would go for #1, but it does pave the way for (even more) confused questions on why things don't act the expected way. This is due to how fluent interfaces often end up with confusing behaviours.

In #1545 I mentioned an example with sinon.stub(objects, prop).get(getter).set(setter). Changing the API to return a different (new) stub for each get/set call makes sense (I proposed it at one point), but it comes with downsides of its own, and lands us in an unfortunate setting where the above probably still will run, but it won't make any sense. I am not sure of a way to improve that situation. I am also not sure changing the API for direct spy capacity outweighs the negatives.

My current take (not set in stone) is that what we in general lack the most are good examples on how to do stuff. For instance, spying on property getters/setters is perfectly doable today, as I suggested in #1545 (pass in a spy as the getterfn), but as this issue shows that knowledge is too hidden away for anyone to find it.

[fatso83]

There is, btw, another take on the API that wouldn't change existing code

• make the getters/setters spies by default
• expose them as properties on the stub

spy.getter.calledOnce, etc

[fatso83]

PS. This issue isn't a bug, as it's willed, intended behaviour, though poorly documented. Previous volunteers for documenting it never got back with a PR .. As it's not a bug, but a feature request in disguise, I am tempted to close it, but maybe we should just change the title and label it as a feature request to save the noise of creating (yet) another issue?

[JakobJingleheimer]

spy.getter.calledOnce, etc

@fatso83 that's exactly what I would expect (and was the first or second thing I originally tried).

More robust examples in the docs would be very appreciated. I get a lot of questions from my colleagues that could be answered by that. I'd be happy to contribute to that.

[fatso83]

It's clear we need to look at property accessors again (see also #1762 for a discussion on proposals to API changes and why there's no clear "best" path), but having more docs is always welcome. Feel free to provide us with what you would have liked to be in the how-to section.

[JakobJingleheimer]

@fatso83 the description for Stubs is confusing:

Test stubs are functions (spies) with pre-programmed behavior.

When first starting with Sinon, I, and several others, understood that to mean that the behaviour was already set, not that the implementer/developer determines the behaviour (or doesn't). That's also not entirely accurate, because if I don't attach any behaviours to the stub (via .returns(), .value(), etc), it does nothing during execution, which could be what is actually desired, ex stubbing console methods to keep the output clean (I know there are better ways to handle that, but we don't control 3rd-party libs).

Some Getting Started documentation would be very appreciated by newcomers. Ex I didn't realise how incredibly useful sandbox is until a while down the line; that sort of documentation could have saved me quite a few _.each(spies, spy => spy.resetHistory())s and _.each(stubs, stub => stub.restore())s, and some accidental cross-contamination on my specs.

Some recommended approaches and tips would also be great. Ex Spying on getters and setters of a stubbed object is quite difficult to figure out (once you figure it out, it makes sense, but before, it's a pretty big wtf).

The last thing I can think of are examples that are less abstract and more practical/real-world to help the reader understand how/when a particular method is useful. Ex The appropriate use-case for stub.onCall(1) seems extremely limited to me (it seems very brittle): A seemingly insignificant change in code could easily break this, when what it's trying to test is still valid. It seems the only appropriate use-case is when sequence is actually important and needs to be maintained (first call must happen before the second, for whatever reason). I could easily see an unwary beginner not understanding that implication, and using it when call sequence is not important just because the sequence is currently first then second (but a change in sequence only matters to the spec, which inappropriately starts failing).

So a warning would be nice:

onCall(n)
⚠️ Danger, Will Robinson
Use this method only when call sequence is important (where first call must precede second call, etc); otherwise the spec could be broken by unimportant changes.

Potential real-world example for the docs:

const fetchStub = sandbox.stub(window, 'fetch');
fetchStub
    .resolves(optionalResponse);
fetchStub
    .withArgs('/a/bad/path').rejects(optionalResponse);

window.fetch('/a/good/path'); // resolves
window.fetch('/a/bad/path'); // rejects

fetchStub
    .withArgs('/a/bad/path').resolves(optionalResponse); // overwrites previous behaviour

window.fetch('/a/bad/path'); // now resolves
window.fetch('/a/good/path'); // unchanged: still resolves

[fatso83]

An official "Getting started" would be great and is sorely needed. Right now, there are lots intros on the web, but no one that we control and officially support, although the Articles elsewhere on the web are some of the ones we recommend.

[JakobJingleheimer]

@fatso83 i happen to have already written one for my department. You're welcome to have it if you'd like. It's currently in slide-form. I can convert it to markdown or whatever.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[JakobJingleheimer]

@fatso83 did you want that getting started stuff? I'd happily lend a hand.

[mroderick]

@jshado1 that would be great, thank you

[0xdevalias]

Just wanted to throw in my /2c+1 on the 'unexpected effect of getters not being spies', would love to see this feature included, though @jshado1 's workaround is also how I was thinking of solving it in the interim.

Though currently, not sure how to get it working. A cut down example:

export const mockTaskThatRunsXTimes = times => {
  const isRunningStub = stub()

  // Call X
  for (let i = 0; i < times; i++) {
    isRunningStub.onCall(i).returns(() => true)
  }

  // Default
  isRunningStub.returns(() => false)

  const task = mockTask()
  stub(task, 'isRunning').get(isRunningStub)

  return task
}

I would expect I should be able to do the following:

t.is(mockedTask.isRunning.callCount, 1)

But that returns undefined

Looking at the console.log output it seems it is a Getter, which I assume relates to this thread and how it "isn't spy'able"?:

{ isRunning: [Getter],
  cancel:
   { [Function: proxy]

My workaround:

- const task = mockTask()
+ const task = { ...mockTask(), __isRunningStub: isRunningStub }

- t.is(mockedTask.isRunning.callCount, 1)
+ t.is(mockedTask.__isRunningStub.callCount, 1)

[fatso83]

@0xdevalias Not trying to sell you short, but we are trying to keep the GitHub issues list tidy and focused on bugs and feature discussions, and your looks like a better fit forStackOverflow, as there are people literally waiting for someone to tag it with sinon, so that they can earn points. I don't have the time to inspect what's wrong, at least.

[RoystonS]

Been away from this one for a while as I mostly just got used to the limitation. I can take a look at implementing the suggestion in #1741 (comment) if somebody (@fatso83 ?) would be willing to give a hand with reviewing?

[fatso83]

@RoystonS This is my second check-in since April (parental leave), so I am not that guy ATM ...

[kasamachenkaow]

Hello

I found that in the wrap-mothod.js file here it's trying to get the method by using the value field from the property descriptor which doesn't exist for the accessor property descriptor (getter/setter)

Could we make a change to make it aware of what type of the property (accesstor or data) it's dealing with and get the method accordingly?

I know that we could use the sinon.spy(obj, 'method', ['get']) right now but it would be nice if it would support without doing that since from Typescript 3.9+ they make any exported module a getter

And when you're writing TS module you might forget that all of your module would be getters since TS does that behind the scene with transpilation

Also it would make writing assertion simpler too (instead of sinon.assert.calledOnce(spy.get))

[fatso83]

since from Typescript 3.9+ they make any exported module a getter
Is that not entirely dependant on the tooling used? Did not think this was a language spec thing. We had similar issues with Webpack in the past, which changed its ES2015+ -> ES5 transpilation to use getters starting with Webpack 4 or 5.

The sinon.spy(obj, 'method', ['get']) has always irked me a bit, but getting this right is from what I can remember a proper wormhole of potential bugs/snags/corner cases. I thought it was something along the lines of it being difficult to know/detect what kind of property or getter you were looking at any given time.

Feel free to look into this, though! I would love if we could treat getters and values in the same way, but I am not entirely sure if it is feasible using the current APIs. You have the old stubs API and also the newer replace* methods

[kasamachenkaow]

@fatso83 Thanks for the reply, I can try playing with it a bit!

[fatso83]

So annoying that we never reached closure on this one. There were too many suggestions in all kinds of directions.

The original issue was perhaps never a real issue to begin with, when looking at it in hindsight:

Sinon provides no property getter spy API right now afaik.

AFAIK we have had that spy API all the time 🤔

sinon.spy(obj, 'method', ['get'])

What irks me is that we have different APIs for spies and stubs to deal with accessors. At least the consistency is better with the newer replace* APIs:

const o = {
  get foo() {
    return 100
  },
  set bar(value) {
    this.value = value
  },
}

const setter = sinon.fake(function (val) {
  this.value = val * 10
})
sinon.replaceSetter(o, 'bar', setter)
const getter = sinon.fake.returns(42)
sinon.replaceGetter(o, 'foo', getter)

o.bar = 200
assert(o.value === 2000)
o.bar = 1
o.bar = 2
assert(setter.callCount === 3)
o.foo
assert(getter.callCount === 1)

But that does not fix up the handle chainable APIs by itself. Auto-wrapping the replacements for the .get and .set calls for stubs would absolutely make sense, so if anyone would come up with a tiny PR for that we could merge it ASAP.

[fatso83]

@kasamachenkaow I think #2531 and #2533 could improve on the situation, of informing users what is wrong, without doing anything too magical.