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

current state of setter/getter spies #2160

Closed
43081j opened this issue Nov 20, 2019 · 6 comments · Fixed by #2171
Closed

current state of setter/getter spies #2160

43081j opened this issue Nov 20, 2019 · 6 comments · Fixed by #2171

Comments

@43081j
Copy link
Contributor

43081j commented Nov 20, 2019

7.2.3:
https://sinonjs.org/releases/v7.2.3/spies/#using-a-spy-to-wrap-property-getter-and-setter

7.2.4 and onwards don't have this section of the docs.

For a patch release to remove a piece of functionality would be very questionable in terms of versioning so i assume the logic is still there in the source.

However, can someone please clear up what the situation is?

  • Can we still spy on setters/getters this way?
  • Where did the docs go?
  • What is returned, if it works? a property descriptor? so it has a special signature different to all other spy calls?

Maybe the docs just went walkies by accident?

@fatso83
Copy link
Contributor

fatso83 commented Nov 20, 2019

Well spotted. If you have any chance to do a forensic of this by using git blame and git log -p on the affected files, you might spot the actual reason in the log descriptions (and/or PR text).

I don't know/remember, but you might be right that it's accidental.

@43081j
Copy link
Contributor Author

43081j commented Nov 20, 2019

714911e#diff-2969afd226e75d3e2d257a1ab427d503

but no obvious reason why

also, do you know what the return type is? a spy? or a descriptor containing spies?

edit:

looks like spy(obj, method) returns the wrapped function
spy(obj, method, [get]) returns the descriptor containing potentially wrapped functions

@fatso83
Copy link
Contributor

fatso83 commented Dec 4, 2019

Something mysterious has been going on. I have hunch to what it is. The link you posted to the commit shows the diff for the generated docs for the latest version. That is always generated from release-source/release/spied.md. Looking at that, I see no mention of getters and spies:
https://github.com/sinonjs/sinon/commits/master/docs/release-source/release/spies.md

@fatso83
Copy link
Contributor

fatso83 commented Dec 4, 2019

And here it is: https://github.com/sinonjs/sinon/commits/714911ee19f237f7b5ef10d04ba3afb4b1c705a5/docs/_releases/latest/spies.md.

It shows the PR #1976 by @salomvary added this section to the generated docs, not the source, and we were unable to catch it.

Would you like to reintroduce them, @43081j?

@fatso83
Copy link
Contributor

fatso83 commented Dec 4, 2019

And with regards to return types you seem to be right. In the case of using types (get/set), the descriptor will be passed as the third argument (method) to wrapMethod, which returns this argument after mutating it. Ref https://github.com/sinonjs/sinon/blob/master/lib/sinon/spy.js#L188

@43081j
Copy link
Contributor Author

43081j commented Dec 4, 2019

great, thanks for looking into it.

i was fixing up the typescript types, turns out they've been wrong for some time 🙈 PR is open anywho

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants