feat(sandbox): adds support for chai.spy.sandbox

[stalniy]

Also adds DEFAULT_SANDBOX and all spies from chai.spy.on and chai.spy.object are tracked under DEFAULT_SANDBOX

Fixes #38

[stalniy]

This is the first iteration of sandboxes feature. @keithamus would like to here from you about this!

[keithamus]

Great work @stalniy. I've added a few comments I'd like to see addressed or discussed.

@meeber I know you had reservations about this project in #57 but I think @stalniy has shown some good work here - and I think we should take a second look at this and #57. Love to get your thoughts here and #57 again.

[keithamus]

I think the interface for this path feels slightly awkward (.spy.on('thing', ['a', 'b']) does not read so well). I feel like I'd prefer a dedicated method for this instead.

[stalniy]

It's interface creation, like: spy.object('Array', ['push', 'pop']) returns a mock for arrays. So, could you please explain why you think it's awkward?

[keithamus]

I get the intent, but I feel like the interface isn't so clean:

• Why overload object? Why not have it as something like spy.interface([...methods...], name<optional>)?
• What if I want to spy on String methods (spy.on('foo', ['endsWith']);)?

[stalniy]

The initial intent of creating spy.object was an ability to create a mock (object consists from spies). But yeah, eventually name became a bit confusing. So, I will remove logic related to spy.on and will rename it interface. Logic related to spy.on will remove from spy.interface

[keithamus]

I don't like that there is a silent failure path here. If the method is already a spy, then doing nothing is acceptable (because the intent from the developer is that they want a spy, and it is one); however if the method doesn't exist - we should either throw, or create an empty spy (my preferred option).

[stalniy]

check for functions covers different case:

var object = {
   firstName: 'John',
   lastName: 'Doe',
   fullName() {
     return this. firstName + ' ' + this.lastName
   }
};

spy.on(object)

// without check
typeof object.firstName // function, enumerable property was wrapped in spy

[stalniy]

But I can add another check for undefined methods:

!(methodName in object) || typeof method === 'function' && !method.__spy

Do you think it's fine?

[keithamus]

The main problem here is if I do something like:

var object = {
   firstName: 'John',
   lastName: 'Doe',
   fullName() {
     return this. firstName + ' ' + this.lastName
   }
};
spy.on(object, ['firstName', 'lastName'])
expect(object.firstName).to.be.a.spy // this is fine
expect(object.lastName).to.be.a.spy // this fails, `lastName` was never set to be a spy, but I wasn't given an error when I tried to make it a spy.

[stalniy]

it's not about spies but about functions. Users want to wrap only functions into spies, there is usually no point to wrap a property.

So, lets wrap all:

• object methods in spies if they are not spies
• undefined values if its key doesnt exist in object

For everything else I will throw an exception:
Unable to spy property. Only methods and non-existing properties can be spied.

Is this ok?

[keithamus]

Could be nice to either return this - allowing for chaining (spy.restore(Array.prototype, 'push').restore(Array.prototype, 'pop')) or alternatively allow passing an Array to restore's second arg (spy.restore(Array, ['push', 'pop'])).

[stalniy]

ok, make sense. Will implement both suggestions

[keithamus]

I don't like this silent failure path. We should throw an error here, telling the user they cannot restore a non-tracked spy.

[stalniy]

sure

[keithamus]

Great work here! This conditional will resolve lots of subtle errors.

[keithamus]

This is very interesting - having the spy clear its tracking, but still be a spy. A developer could restore a spy method after using it (maybe even inside the spy itself) but still assert on the spy after its call. Very cool. Opens us up for later cool things like:

var myArray = [];
pushSpy = chai.spy.on.once(myArray, 'push')
expect(myArray.push).to.be.a.spy();
myArray.push('foo');
expect(myArray.push).to.not.be.a.spy.and.equal(Array.prototype.push);
expect(pushSpy).to.have.been.called(1).with.exactly('foo');

[keithamus]

Could the id be moved into the __spy object instead of another assignment to the object?

[stalniy]

make sense, also I would like to hide __spy under Symbol, so nobody can get access to private info. But this will be done in a separate PR

[keithamus]

Completely agree. If we do though, in the same PR we'll need something way to get the underlying calls, e.g. chai.spy.getCall(spy, callNumber)

[keithamus]

Maybe not as part of this PR, but I'd like to remove the reset method from the spy and push it up into our API - or possibly even get rid of reset entirely now.

[stalniy]

Yes, I also think that method should be removed and this should be a part of a separate PR

[keithamus]

Feels like we could shorten a lot of this to just;

chai.spy = new Sandbox();
chai.spy.sandbox = () => new Sandbox();

[stalniy]

But then it won't be possible to create regular spies, like:

var success = chai.spy()

asyncOperation().then(success)

expect(success).to.have.been.called()

[stalniy]

If you want to make things shorter I can do this:

['on', 'restore', 'object'].forEach(function (methodName) {
  chai.spy[methodName] = function() {
    return DEFAULT_SANDBOX[methodName].apply(DEFAULT_SANDBOX, arguments)
  };
})

Update: but then it will be harder to write down JSDocs

[keithamus]

Ah good point about regular spies. Let's keep the code as is then 👍

[stalniy]

@keithamus could you please take a look? Want to finish this today

[keithamus]

Hey @stalniy. This is all looking great now. Happy to merge! Thanks for your hard work and patience!