Add getters/setters stub behaviors new API

[lucasfcosta]

This pull request is now complete. Please see this comment to read about the new changes.

Purpose (TL;DR)

This aims to add get/set stubbing features for #1258.

I also added some tests for it (supporting only the current state of this PR, not the whole feature).

Background

As I've said in the previous section, given the new API and deprecation of stubDescriptor (#1239) we should have a new API for setting getter and setter stubs.

Right now it's only allowing us to define getters and setters for non-function properties. Which I admit is not that useful as if it allowed stubbing getters and setters for functions too.

The reason I'm opening an incomplete PR is both to have your feedback on the current work and also because I'll be traveling tomorrow so I won't have time to work on this for the next two weeks so I will leave this here in case someone wants to continue this work (sorry for that!).

Solution

Since this solution is partial I'll explain what I've done until now and the problems I've faced.

First of all I needed to store a reference to the root object and the original property name which we're adding this stub to in this line. I only do that when someone passes a non-function property in order to avoid calling wrapMethod which ends up throwing an error when we pass non-function properties to it. We might want to look forward to allow it to wrap non-function properties too in order to fully accomplish this PR's goal.

The reason I needed to import getPropertyDescriptor is to avoid triggering existing getters when doing our property type checks.

Then finally I have just added two new behaviors which you can see here and here. These are the methods responsible for adding the getter/setter replacements to the desired property on the object we're stubbing. It simply gets the reference to the root object and the property's name and defines a new descriptor for it.

Known Issues With This Approach:

• We cannot stub getters/setters for functions
• We cannot restore these getters/setters

Also, @fatso83 talked to me about not having the stub available anymore when retrieving a property if we stub its property descriptor. I haven't thought about this when I was talking to him, but since the behavior methods return this.chain() it would not be a problem, we would just need to assign the result of calling the get or set method to a variable and then use it to manage our stub.

Things we might wanna do for this to be complete:

• Allow wrapMethod to wrap non-function properties in order for us to be able to stub them
• Allowing it to wrap non-function properties would also be cool because then we could just call it on our behaviors passing property descriptors and then we would be able to restore them and we also get to reuse more code

In a nutshell: if we make wrapMethod work for this use case it would be perfect because then we could use it and accomplish our goal without too much branching.

How to verify - mandatory

1. Check out this branch (see github instructions below)
2. npm install
3. npm test -> This will run the newly created tests

Feel free to clone my fork and continue your work from there if you want to, this is just a proof of concept and a way of sharing what I've thought as I was talking to @fatso83 this morning.

Sorry again for not being able to finish this, I hope this PR helps you deciding what to do.

[coveralls]

Coverage decreased (-0.001%) to 95.42% when pulling 20c197a on lucasfcosta:getters-setters-new-api into c26a2dd on sinonjs:master.

[fatso83]

Good stuff. I implemented most of the stuff you mentioned in 1258 yesterday, which included most of this, but not the part where you modify the stub in the outer module to keep the reference to the original object. That was the missing part of the puzzle I was too tired to find yesterday 😅

Using your other suggestions I guess most things are covered, and AFAIK this should also work for sandboxes. I think we could easily modify this to also support restoring, if we store the original property definition (actualDescriptor ) on the stub as well. I might have some time to look at this in the coming week, integrating your changes.

One thing the current approach does not support, AFAI can see, is support the chaining we were discussing. AFAIK, each invocation of Object.defineProperty will overwrite the previous definition, which means we would might need to keep the previous definition and merge it should we choose to support it.

[mroderick]

@lucasfcosta this branch needs a rebase. How far would you guess you are with this? Any chance of completing it soon?

[fatso83]

As said above he will be unable to complete this until he gets back in another two weeks. I did some of it, but won't have time until next weekend.

[mroderick]

As said above he will be unable to complete this until he gets back in another two weeks.

My apologies, I was clearly not paying attention

[cjohansen]

Do we want to wait another two weeks for the 2.0 release? Can't this be in 2.1? Or is it breaking?

[fatso83]

I'd say we ship this MF. And bump the minor version once we have this. 7. mar. 2017 1.44 p.m. skrev "Christian Johansen" <notifications@github.com

…

: Do we want to wait another two weeks for the 2.0 release? Can't this be in 2.1? Or is it breaking? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#1297 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAluXNSwZoVrELWFpHVsczf2q0l_6hdDks5rjVEhgaJpZM4MMN3z> .

[mroderick]

Do we want to wait another two weeks for the 2.0 release? Can't this be in 2.1? Or is it breaking?

If I understood it correctly, then it we have introduced a breaking change, that has the potential of getting fixed by this PR. While I am keen to ship, I don't think waiting a week or two is going to make that big a difference.

[lucasfcosta]

Hi friends, I just got back from London, I'll be able to work on this again. If you have any related PRs or issues that happened in this time but were not linked to this issue, please let me know.

Also, when it comes to releasing this change, I think we should do this ASAP, since this PR adds a feature that has been removed due to the deprecation of the old API (even though it still exists on the codebase).

I'll try to work on this tonight and tomorrow, but I can't guarantee I'll be able to finish it fast since we've got lots of edge cases to cover and checks to remove. I'd like to be careful when removing some of these checks because I want to enable this feature without damaging other restrictions and user feedback. My main concern are the wrapMethod restrictions.

[lucasfcosta]

Hi friends, this one is done!

I will not edit the first post to keep the history of changes and previous explanation. Here goes a description of the changes I've just done.

Background

So, as I've explained on the first post of this PR we still needed to decide how we were going to wrap function properties. The old approach tried to detect non-function properties and then trigger specific behavior for it. Due to that fact we were not able to wrap function properties, because then we would still call the old wrapMethod and then we wouldn't have access to the properties added when we detected non-function properties.

We also needed a restore method to get the old descriptor back in its place if needed.

Solution

Instead of changing wrapMethod and then having to deal with lots of special edge cases, I tried to reuse it and just add new behavior to the stub we are using as the new method. As you can see here I always create a stub with the rootObj and propName properties doesn't matter if what we're wrapping is a function or not. The difference with this new approach is that we will avoid calling wrapMethod if we're not wrapping a method and we will just return the newly created stub itself instead.

Also, there's no problem in always using arity as the argument to stub.create because it is 0 if object[property]'s length is falsy.

Finally I added a simple restore method to the stub we're creating. It is just a closure that has access to actualDescriptor, object and property so that we can call Object.defineProperty on the property that has been wrapped.

I also added a few new tests for that. They ensure this change is compliant with the following specs:

• allows users to stub getter/setter functions for properties
• allows users to stub getter/setter functions for function properties
• allows users to replace old getters/setters without calling them
• can set getters/setters for non-existing properties
• can restore stubbed setters/getters for functions
• can restore stubbed setters/getters for properties

Other Details

• This Pull Request also includes a commit updating docs with the new API syntax and examples
• I also rebased this into master to add the get and set behaviors to the new defaultBehavior file introduced in addBehavior #1302.

PS.: Does anyone know why coveralls is failing? I've seen #1319 but I can't see why it happens on every PR.

---

Sorry for taking so long to continue this. If you need any further changes I'll be more than happy to do them.

[coveralls]

Coverage decreased (-1.5%) to 93.92% when pulling ad58467 on lucasfcosta:getters-setters-new-api into e0cfccc on sinonjs:master.

[coveralls]

Coverage decreased (-1.5%) to 93.92% when pulling ad58467 on lucasfcosta:getters-setters-new-api into e0cfccc on sinonjs:master.

[fatso83]

We are doing this for free. Don't feel bad :-) The code looks fine, so I am merging this. Coveralls does not come with any good docs, so I am not sure why it fails, but I am guessing it simply fails the build if the coverage drops beyond some threshold.

Looking at some of the non-covered code Coveralls complains about, it seems that it has to do with non-ES5 code which is not triggered. We no longer support non-ES5 browsers, so that code could well be scrapped.

[fatso83]

And with that merge done we are now in ready-to-publish-Sinon-2-land, @sinonjs/sinon-core 🎉

[lucasfcosta]

@fatso83 yayyy! Congrats 😃

Also, I was thinking about refactoring wrapMethod, would you mind if I do it and remove the code to handle non-ES5 browsers?

[fatso83]

@lucasfcosta sure, go ahead, but it won't stop us from releasing 2.0.

[mroderick]

And with that merge done we are now in ready-to-publish-Sinon-2-land, @sinonjs/sinon-core 🎉

@cjohansen will you do the honours?

[mroderick]

Also, I was thinking about refactoring wrapMethod, would you mind if I do it and remove the code to handle non-ES5 browsers?

Improvements are always welcome 👍

[cjohansen]

@mroderick Just did!

02a6ff6
https://www.npmjs.com/package/sinon