Fix spying on *etter methods

[lucasfcosta]

Purpose (TL;DR)

This PR aims to fix #1018 on v1.17.
Basically I just needed to pass a new descriptor with only the property from the propDescriptor which is going to be spied on.
I also added tests to prove both stubbing and spying work as @mroderick did on the issue itself.

Background (Problem in detail)

The problem happened because the property descriptor being passed to be spied on had non-function property values (because it was a complete property descriptor taken from that property) and then that caused an error.

Solution

As I've said above I just needed to passa a property descriptor with only the property which was going to be spied instead of the whole property descriptor

How to verify

1. Check out this branch (see github instructions below)
2. npm install
3. npm run test -> This will run the new tests I've added

---

Thanks for the awesome work you have been doing on Sinon, I'm looking forward to contribute more with you, please let me know if you need anything!

I've read the contribution guidelines but I'm not sure this PR meets your commit/coding patterns, please let me know if I've done anything wrong or missed anything and I'll happily fix it. You rock ❤️

EDIT: The build seems to be failing on CI for an unknown reason. I've read the log and I think it might be related to Travis itself and not this PR. Do you have any idea why this is happening?

[fatso83]

I think this looks good as it is. The only thing I find a bit missing are code comments that tell why things are done. To the casual reader, there is very little help in seeing why the descriptor object is being introduced. I am assuming this is to avoid triggering the stubbed property? Could have been nice to see it commented.

On the other hand, this is just my opinion, and I am not sure the others agree. I just happen to like comments :-)

[fatso83]

Sorry for the slow followup ... you are right about the failing build. Restarting the Travis build seems to have fixed the failing tests.

[fatso83]

@mroderick objections to merging this? I could like a little comment or two, other than that ... 👍

[lucasfcosta]

@fatso83 I made some comments while studying this code and doing the fix, but since the code didn't have any comments before I assumed those pieces of information would be considered unnecessary.
I'd be happy to add a few more comments to this code if you want me to, all you gotta do is let me know 😄

Thanks for the feedback!

[fearphage]

I don't like the naming here. Either getters and setters or a getters section separate from the setters section.

[lucasfcosta]

You're right! It really looks better, thanks for the feedback. I'll do it.

[fatso83]

@lucasfcosta, as long as comments document the intent, not the steps of the implementation, I think it usually helps the understanding of the reader. If you can add a small comment I think it would help future readers understanding the code. I also wasn't a fan of the "etters" naming, but it's not a big thing. Just change it per fearphage's comment and this should be good to merge.

[lucasfcosta]

Here it is!

I made sure to add two concise comment lines explaining why we needed a new descriptor object whenever trying to spy on property descriptor's properties.

I also renamed both of those tests containing the getter and setter specific tests to getters and setters, it really looks better than *etters.

Thanks for the feedback @fatso83 and @fearphage, let me know if I missed anything.

[fatso83]

Great work, and thanks for listening to the input. Much appreciated.

[muuki88]

Hi,

Is there a migration path on how to implement this in the new 2.x.x version series? I see that the tests ignore the deprecation warnings.

Thanks a lot :)

[lucasfcosta]

Hi @muuki88, this is only a fix for the v1.17 version.
The implementation for this feature in v2 has happened at #1297.

These are the docs on how to implement getter and setters spies and stubs:

stub.get(getterFn)

Replaces a new getter for this stub.

var myObj = {
    prop: 'foo'
};

createStub(myObj, 'prop').get(function getterFn() {
    return 'bar';
});

myObj.example; // 'bar'

stub.set(setterFn)

Defines a new setter for this stub.

var myObj = {
    example: 'oldValue',
    prop: 'foo'
};

createStub(myObj, 'prop').set(function setterFn(val) {
    myObj.example = val;
});

myObj.prop = 'baz';

myObj.example; // 'baz'

However, by looking at the spy API we've got no alternatives for this.

I'd be more than happy to implement this, however, we've gotta define what's the API going to be like. I'll open an issue for this.

EDIT: Issue #1392 is related to this.*

[muuki88]

Thanks a lot @lucasfcosta . This works for me. The confusing was due to the missing typescript definitions for the get and set methods on the stub. I tried the callsFake, which didn't worked as expected.

Should I open an issue for adding the type signatures to the SinonStub interface?

[fearphage]

The indentation is off here. This should have the same indentation as the test above it and the preceding closing brace should be moved below this test.