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

Spying on instance methods #296

Closed
Fauntleroy opened this issue Aug 11, 2013 · 7 comments
Closed

Spying on instance methods #296

Fauntleroy opened this issue Aug 11, 2013 · 7 comments

Comments

@Fauntleroy
Copy link

Inside some of my tests I've found that I need to spy on instance methods, but it seems that it's impossible at present since these are read only properties. When I try to use sinon's spy method...

var next_spy = sinon.spy( player, 'next' );

this happens:

TypeError: Cannot assign to read only property 'next' of #<Object>
    at Object.wrapMethod (C:\Users\Timothy\Repos\sync4\node_modules\sinon\lib\sinon.js:84:30)
    at Object.spy (C:\Users\Timothy\Repos\sync4\node_modules\sinon\lib\sinon\spy.js:228:22)
    at C:\Users\Timothy\Repos\sync4\test\db_room_model.js:102:26
    at C:\Users\Timothy\Repos\sync4\node_modules\orm\lib\Model.js:297:12
    at C:\Users\Timothy\Repos\sync4\node_modules\orm\lib\Singleton.js:32:10
    at Object.<anonymous> (C:\Users\Timothy\Repos\sync4\node_modules\orm\lib\Model.js:112:12)
    at C:\Users\Timothy\Repos\sync4\node_modules\orm\lib\Instance.js:24:7
    at Array.map (native)
    at Instance.emitEvent (C:\Users\Timothy\Repos\sync4\node_modules\orm\lib\Instance.js:23:17)
    at C:\Users\Timothy\Repos\sync4\node_modules\orm\lib\Instance.js:608:4

Is there a recommended method for mocking/spying on instance methods?

@dxg
Copy link
Collaborator

dxg commented Aug 11, 2013

What exactly is next?

@Fauntleroy
Copy link
Author

For what it's worth...

Room.Player = Room.extendsTo( 'player', {
    elapsed: {
        type: 'number',
        defaultValue: 0
    },
    duration: Number,
    order: {
        type: 'enum',
        values: ['normal','shuffle'],
        defaultValue: 'normal'
    }
}, {
    // TODO use this instead -> autoFetch: true,
    hooks: {
        afterLoad: function(){
            // TODO see if this can be mixed in to the instance
            if( !this.events ) this.events = new EventEmitter;
            this.timer;
        }
    },
    methods: {
        // select and play the next item
        next: function( cb ){
            cb = cb || function(){};
            var player = this;
            player.getRoom( function( err, room ){
                if( err ) return cb( err );
                room.getItems( function( err, items ){
                    if( err ) return cb( err );
                    if( items.length === 0 ){
                        player.load( null, function( err ){
                            if( err ) return cb( err );
                            return cb();
                        });
                        return;
                    }
                    var item;
                    if( player.order === 'normal' ) item = items[0];
                    if( player.order === 'shuffle' ) item = items[ _.random( items.length ) ];
                    room.removePlaylistItem( item.id, function( err ){
                        if( err ) return cb( err );
                        player.load( item, function( err ){
                            if( err ) return cb( err );
                            return cb();
                        });
                    });
                });
            });
        },
        ...
    },
    reverse: 'room'
});

@dresende
Copy link
Owner

I'm not sure if the next word is not reserved for iterators. You should change it before it bites you when ES6 arrives (without having to pass options to node executable).

@dresende
Copy link
Owner

I think this is because of this line:

https://github.com/dresende/node-orm2/blob/master/lib/Instance.js#L457

It defines the methods not specifying configurable (so the default is false), meaning it cannot be changed later. This is what sinon is trying to do. Please change it in your orm copy and see if it works. Something like this:

    for (k in opts.methods) {
        Object.defineProperty(instance, k, {
            value: opts.methods[k].bind(instance),
            enumerable: false,
            configurable: true
        });
    } 

If it works I'll update it. I don't want to touch the repository now since I'm waiting for some changes from a fork and don't want to cause conflicts.

@dresende
Copy link
Owner

It might be the writable option and not the configurable one.

@dresende
Copy link
Owner

If you still want this and the above change works please reopen and I'll change it.

@Fauntleroy
Copy link
Author

@dresende the writable change fixes the issue for me.

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

No branches or pull requests

3 participants