Implement spies for get/set functions on accessor properties

[apsillers]

I think it would be useful to extend spyOn (or add another method, like spyOnGetter) to spy on the getter or setter function of an accessor property. Currently there is no way to spy on the following get functions, because Jasmine currently can only spy on functions that are properties, and not on functions that are set/get accessor functions of a property:

Object.defineProperty(obj, "dynamicFoo", {
    configurable: true,
    get: function() {
        return Math.random();
    }
});

var obj = {
    get dyanmicBar() { return Math.random(); }
}

It seems like the core functionality could be implemented using defineProperty to rewrite the property with a spy for its get or set method.

The current code is

this.spyOn = function(obj, methodName) {
    ...
    var spy = j$.createSpy(methodName, obj[methodName]);
    obj[methodName] = spy;
}

The code could handle accessor functions with a third argument (either "get" or "set") like so:

this.spyOn = function(obj, methodName, accessType) {
    ...
    var desc = Object.getPropertyDescriptor(obj, methodName);
    if(desc[accessType]) { // "get" or "set" exists on the property
        var spy = j$.createSpy(methodName, desc[accessType]);  

        desc[accessType] = spy;

        Object.defineProperty(obj, methodName, desc);
    }
}

However, I'm not sure how this would break existing assumptions. For example, in my sample code above, createSpy is given the property name, but the property is not being spied on; rather, one of the property's accessor functions is being spied on. This change would require eliminating the assumption that a function being spied on is a property of an object, and instead writing logic that admits the possibility that a spied function could be a get or set function within an object property.

[StanleyGoldman]

+1

[slackersoft]

I can see that being useful. I don't think we want to add that complexity to the existing spyOn implementation, but I would be happy to review a pull request to add something like spyOnProperty or something.

[mgilliganrca]

+1

[isadovskiy]

+1

[jamesbrobb]

+1

[apaatsio]

+1

[maxyharr]

+1

[vojtech-cerveny]

+1

[salickc]

+1

[ghost]

+1

[nicbou]

+1

[anjmao]

+1

[FunkeyFlo]

+1

[benediktarnold]

+1

[chrisatanasian]

+1

[ptomato]

I think at this point the +1s are crowding out any actual discussion that might happen here; please show your enthusiasm by subscribing yourself to the issue or adding a 👍 reaction with GitHub's new "reaction" feature so that everybody doesn't continue to get notifications for each new +1.

Or, better yet, attach a pull request 😄

[henrahmagix]

@ptomato #1008 is a pull-request that fulfills this issue, already attached

[Gigitsu]

+1

[henrahmagix]

@slackersoft We now have #1008 and #1203 able to be merged. Is there a timeline for this feature?

[elwynelwyn]

Any guidance on when this will make it into a release?

[joshfermin]

+1

[fimius23]

+1

[henrahmagix]

@elwynelwyn @joshfermin @fimius23 I know this is late so you may know already, but this is available in 2.6 (#1203 (comment)) with docs at https://jasmine.github.io/api/edge/global.html#spyOnProperty

[pauljeffreys]

I realise I am late to the game here, but does anybody know of a similar function call for jest? Wanting to test a getter in that also, but this addition is just jasmine..

[slackersoft]

@pauljeffreys you should probable ask the Jest folks. (https://github.com/facebook/jest) I'm not totally sure it's even still built on top of Jasmine in a way that would allow them to incorporate the work done here.

[phra]

@pauljeffreys @slackersoft see my PR at jestjs/jest#5107