Fix restoring getters/setters/values for previously unexisting props

[lucasfcosta]

Purpose (TL;DR)

This allows our users to restore getters, setters and values (property descriptor's properties) for properties that were previously undefined.

It also removes the unnecessary restore method inside the value behavior introduced in #1416 since we can use the stub's own restore method to accomplish the goal of restoring a value.

This is a remake of #1418 because of further problems I discovered while playing with that solution.

Background (Problem in detail)

This happened because we were always getting the actualDescriptor of the property being stubbed, thus, if this property was not defined, actualDescriptor would be undefined.

This caused the restore method to throw an error when using Object.defineProperty to redefine that property back to its old value because undefined was passed to it as the descriptor.

Solution

In order to solve this I added a check to see if the descriptor we've got inside the stub closure is defined. If it is not defined we can just delete the stubbed property.

In order to be able to delete it, that property must be configurable so I had to add a configurable: true property to the property descriptors passed when defining a new getter, a new setter and a new value for a property.

Also, noew the value behavior also defines an enumerable property, because that way it behaves more like a property defined using an assignment operator (=).

This PR also includes tests for all those changes, one for each default behavior that has been modified.

How to verify

1. Check out this branch (see github instructions below)
2. npm install
3. npm test

PS: Sorry for opening that other unuseful PR before this one. 😓

[coveralls]

Coverage increased (+0.04%) to 94.896% when pulling d351178 on lucasfcosta:fix-restoring-unexisting-props into 64d51be on sinonjs:master.

[mantoni]

This looks fine to me, but I have a question: Sinon currently throws when trying to stub a non-existing function. Shouldn't the same be true for unknown properties? If so, cleaning up undefined properties wouldn't be necessary because it shouldn't happen in the first place, right?

If we conclude that adding previously undefined properties with Sinon is fine, this fixes an issue and should be a patch.

[mroderick]

👍

[lucasfcosta]

@mantoni, thanks for your review, but actually sinon does allow users to stub undefined properties, thus this is just a PATCH actually.

This happens because of this check inside sinon.stub. Due to it, when deciding whether we're going to just return the created stub or call wrapMethod, we'll return the created stub instead of calling wrapMethod, because we're wrapping a non-function property.

The piece responsible for throwing an error is wrapMethod, but it won't be called.

This PR is focused at fixing the behaviors that make sense to be used with non-function properties, which are mainly get, set and value.

IMO another PR is needed to allow stubbing undefined properties as functions and if you agree I'd be more than happy to make it. I've been looking forward to refactoring wrapMethod for a while.

Thanks everyone for your input.

[mantoni]

@lucasfcosta Thanks for elaborating. I was trying to point out that there is an inconsistency between function stubbing and property stubbing. I do like the exception that is being thrown when I try to stub a non-existing function because it catches my typos while writing tests. My question is not whether we should remove that check, it's whether we should introduce it for property stubbing as well?

[lucasfcosta]

@mantoni I think that it's impossible to throw when stubbing properties that are undefined only when we want them to have their "call behavior" changed (as it happens when using callsFake for example). Because we only add a behavior to a stub after it's created.

IMO we should add relevant throws to callsFake and related behaviors so that they will help users avoiding typos. This is very valid because I cannot see a use case where someone would like a property that does not exist to call another function instead of the original one. However, I can see cases where someone wants to stub unexisting properties much more often.

[mantoni]

@lucasfcosta Yeah, I don't have a strong opinion here. It makes sense to have different semantics for properties and functions for the reasons you mentioned.

[lucasfcosta]

@mantoni Thanks for your review and for sharing your opinion, I agree with what you've said when it comes to stub functions.

I'll see what I can do regarding those function related behaviors and send another PR.

[mroderick]

This has become part of [email protected], which is on npm

[fatso83]

Seems like we have introduced a regression (#1456).