Devise a way to track & restore spies

[keithamus]

One big problem with spies is being able to quickly restore old behaviour, when you want the function to execute its original behaviours.

Now we have chai.spy.on(object, ...methods); I think we could track which methods we spy on what objects. We could implement some kind of sandboxing:

sandbox = chai.spy.sandbox();
sandbox.spy.on(myArray, 'push', 'pull');
sandbox.spy.restore(myArray, 'push');
sandbox.spy.restoreAll();

The main export of chai-spies could also be a sandbox in of itself - and so:

chai.spy.on(myArray, 'push', 'pull');
chai.spy.restore(myArray, 'push');
chai.spy.restoreAll(); // restores Array.pull

Potentially we could also make a decorator/higher order function which would help inside of tests; for example:

test(chai.spy.sandbox(function (spy) {
  expect(spy()).to.be.a('function');
}));

This is just a rough draft of what I have in my head - I'd very much like feedback, especially from @stalniy who has been doing some great contributions lately, including the improved chai.spy.on interface.

[epsitec]

Alternatively, a simpler approach would be to provide a restore() function, as does sinon,js:

spy.on(foo, 'bar');
foo.bar('Spy me!');
foo.bar.restore(); // restores original foo.bar()

[keithamus]

@epsitec we certainly could do that. I did think about, but I kind of don't like it for 2 reasons:

1. It modifies props on a function which isn't strictly ours - in other words foo.bar.restore() could potentially exist before we make foo.bar a spy. chai.spy is our own namespace though, and so we can safely add whatever we want to it. It also encourages/codifies this as the pattern for all future design questions - e.g. what if we want a .returns (we had this discussion before and opted for chai.spy.returns() over spy = chai.spy(); spy.returns()), or a .getCall... they all start getting tacked onto the spy and each one causes new potential issues.
2. Having restore() and restoreAll() in the same place makes sense, and obviously having .restoreAll() on individual spies makes no sense.

Maybe though... maybe individual restore()s aren't as useful. When I've worked with sinon in the past I tend to use the sandboxing feature because I want to declare each spy during my test and so subsequently I'll restore everything all at once, rather than restoring individual methods. Maybe it makes sense to just have sandboxes that you can wipe down completely, so:

sandbox = chai.spy.sandbox();
sandbox.spy.on(Array, 'push', 'pop');
sandbox.restore(); // restores Array push AND pop, is the only non-spy method on sandbox.

[epsitec]

@ketihamus you have convinced me. I am looking forward to test the sandboxes on chai.spy.

[keithamus]

:smile:

[wsb9]

@keithamus your points are strong. Using sandbox over individual restore() would look as overkill for 1-2 spies in test suite (+1 for default sandbox on chai.spy), but for larger count it is really useful.

Didn't catch the idea of decorator.

[keithamus]

Didn't catch the idea of decorator.

The point of the decorator is that you pass in a callback function which automatically restores spies after execution of the function - saving you all of the boilerplate, take the following two code examples:

it('pushes items to array', ()=> {
  let spy = chai.spy.sandbox();
  spy.on(Array, 'push');
  // some tests...
  spy.restore();
});

it('pushes items to array', chai.spy.sandbox((spy)=> {
  spy.on(Array, 'push');
  // some tests...
}));

@wsb9 do you think it's really necessary to be able to restore single spies, or would you typically restore a bunch/all of them? I think just having one method chai.spy.restore() (or chai.spy.sandbox().restore()) that restores that whole sandbox would be best - but I'd be interested to see other peoples thoughts on restoring individual spies.

[keithamus]

To extend a bit more on the whole restore one vs restore all - I think if you have the ability to restore one at a time, you can end up in a bit of a mess - if I spy on 5 methods and only restore 2, when do the remaining 3 get cleaned up? Or do they just sit there waiting for someone to trip up on them later down the line. I guess conversely if I want to restore 2 methods but keep spying on the other 3, then having to re-spy on the other 3 could be a pain.

Not sure what's best here.

[wsb9]

@keithamus i think there may be situations where individual restore would be handy, but can't find where right now :)

I'd just clean up whole sandbox in test runner hook after test case (mocha's afterEach). So it would be useful to make restore() silently return if there is no spies installed, with no exceptions. I can imagine 1-2 such no-spy tests per suite, for example validating property/method existence.

When you need to leave half of spies while restoring other half, then spies possibly belong to different pieces of logic and it worth to factor them out to two different explicit sandboxes. And release on appropriate time.

[keithamus]

I think you've pretty much nailed it @wsb9.

I'd say we're ready enough with this design to try to get it working. To summarise:

• chai.spy.sandbox() exists for creating sandboxed environments
• chai.spy.sandbox() returns an object with on(), object() and restore(). Each method spies on is tracked within the sandbox.
• sandbox.restore() should set all spies created from the sandbox to their original functions
• chai.spy.on(), chai.spy.object() and chai.spy.restore() should be bound to a top-level sandbox.
• chai.spy() and chai.spy.returns() should still work - and not be bound to a sandbox
• chai.spy.returns()
• chai.spy.sandbox().sandbox() should not be possible

If anyone wants to take up the challenge of making a PR, I'd definitely look forward to seeing it. Otherwise I might work on this in a week or two.

[stalniy]

@keithamus this is great idea! The only issue which I currently see is spy.returns method as it's very handy. To fix this I see 3 options:

1. Make spies being mutable (don't like as it leads to writing non-logical code)
2. Add instance method returns to spies which exists only for spy.on created spies (this one has the same disadvantages as restore instance method)
3. Return link object from spy.on which has returns method (probably others in future). For spying multiple object methods add chai.spy.on.all(object, method1, method2, method3) in this case spying object is returned and returns can't be applied. (probably this one is the best)

chai.spy.on(localStorage, 'get').returns('test')

var cache = {};
chai.spy.on(localStorage, 'get').as((key) => cache[key]);

// under the hood
on(object, methodName) {
  this._sandbox.addForRestore(object, methodName, object[methodName]);

  object[methodName] = spy();

  return {  // TODO: move to constructor with methods in prototype?
    returns: function(value) {
      object[methodName] = spy.returns(value)
    },

    as: function(fn) {
      object[methodName] = spy(fn)
    }
};

all(object, ..methods) {
  methods.forEach((methodName) => {
    this._sandbox.addForRestore(object, methodName, object[methodName]);
    object[methodName] = spy();
  });

 return object;
}

[keithamus]

@stalniy what if we have .on(object, name, functionOrValue) and .on.all(object, [...names])?

 // localStoage.getItem is spied on
chai.spy.on(localStorage, 'getItem');

 // localStorage.getItem is spied on, and its behaviour is overriden
var cache = {};
chai.spy.on(localStorage, 'getItem', (key) => cache[key]);

// localStorage.getItem is spied on, and always returns `'test'`
chai.spy.on(localStorage, 'getItem', 'test');

 // spy on 'getItem' and 'setItem'
chai.spy.on.all(localStorage, 'getItem', 'setItem');
// spy on all enumerable ('getItem', 'setItem', 'removeItem', 'clear')
chai.spy.on.all(localStorage);

[stalniy]

@keithamus Totally agree about chai.spy.on.all but disagree with the third argument for on method as it's non-verbose, especially version with returning "test" value (it's hard to guess what there is going on).

However this is a good alternative and may work as the first api version (it won't be hard to implement better backward compatible solution in future)

[stalniy]

@keithamus probably I found a good compromise for using on together with returns. So, spy.on(array, 'push').returns(5) can be done by restricting users to call returns only once. So, then people won't be able to do crazy things like

const push = spy.on(array, 'push').returns(5)
push.returns(6) // throws exception "spy returns is not a function"

I'd like spy to still be immutable, so spy.on(array, 'push') and spy.on(array, 'push').returns(5) will return different references. Probably I will need to add a method calls which accepts a function (i.e., spy.on(array, 'push').calls(() => { })

What do you think?

[keithamus]

@stalniy just spitballing here, but what about, instead, doing something like chai.spy.on(array, 'push', chai.spy.returns(5))?

[stalniy]

@keithamus sorry, but I think my suggestion is more clear :)

Basically I suggest to split functionality into 2 types of spies (both are immutable):

• regular spy (this is what we have now)
• spy that tracks object's methods (this one will have few extra methods, like calls, returns which returns a new regular spy which doesn't have calls, returns methods)

So,

// regular spy
const method = chai.spy()
console.log(typeof method.returns) // undefined

// tracking spy
const method = chai.spy.on(object, 'method')

expect(method).to.be.spy // true
object.method === method // true

const methodReturns5 = method.returns(5)e

expect(methodReturns5).to.be.spy // true
methodReturns5 === method // false
object.method === methodReturns5 // true
method.returns(6) // throw spy is already configured/locked/etc

[keithamus]

My problem with that API is that it is not immutable by design - only in the checks we supply; it is an easy way to trip someone up who isn't aware of our design decisions ("why can I do returns() once but not twice?").

Secondly, and I think maybe more importantly, is that we're changing the surface of the API that the method has, e.g.:

Array.prototype.push.returns // undefined
[].push.returns // undefined

chai.spy.on(Array.prototype, 'push');
Array.prototype.push.returns // function
[].push.returns // function

If my experience with chai says anything, is that this will cause a bunch of weird permutations to happen in various libraries - or at the very least cause problems for anyone who wants to spy on a method which has a returns method already attached.

IMO the best solution is:

// regular spy
const method = chai.spy()

// tracking spy
const method = chai.spy.on(object, 'method')

expect(method).to.be.spy
object.method === method

chai.spy.restore(object, 'method');
object.method !== method
const methodReturns5 = chai.spy.on(object, 'method', () => 5);

methodReturns5 !== method
object.method === methodReturns5

// Or, if you want a shorthand:
chai.spy.restore(object, 'method');
const methodReturns6 = chai.spy.on(object, 'method', chai.spy.returns(6));

[stalniy]

First of all, I really like spy.restore(object, "method").

About .on.returns I see your point and it make sense. However your 2nd statement probably can't be satisfied becausespy currently overrides wrapped method api.

function me(){}
me.test = true

var spy = chai.spy(me)
spy.test // undefined

And there is nothing we can do with this because properties may be defined as non-enumerable or using Symbol

[stalniy]

My problem with that API is that it is not immutable by design - only in the checks we supply

Actually it's immutable in relation to spy object because returns creates a new spy object but what also not really good is a side effect which changes spied method on object.

As alternative api, we can return SpyBuilder instance from chai.spy.on which has returns and calls methods (probably others in future) which returns the actual spy object. So,

const spyBuilder = spy.on(array, 'push')
const original = spyBuilder.calls('original') // creates new spy and invokes original method when spy is called
const fake = spyBuilder.calls((...args) => args[0]) // creates new spy and calls passed function
const fakeReturn = spyBuilder.returns(5) // creates new spy which returns 5, eventually when array.push is called fakeReturn will be called and others - not

But again this approach uses side effect... What is actually the main problem in such solution. That's why I suggested to allow to do this only once. Also this could be noted in docs and put spy.on(array, 'push').returns(5) or spy.on(array, 'push').calls(() => ...) examples only, as these are the main usecases

So,

const { returns } = chai.spy
chai.spy.on(array, 'push', returns(5))

is better and even more or less good in terms of readability but it restricts the whole DSL syntax to passing stuff as parameters.

But first of all I will work on sandboxes and then will get back to this.

[stalniy]

Other alternatives for returns syntax may be this:

1. chai.spy.new(spy => spy.on(object, 'push').returns(5))
2. chai.spy.on(object, 'push', { returns: 5 })
3. chay.spy.on(object, 'push', returns => 5)