Comments (19)
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()
from chai-spies.
@epsitec we certainly could do that. I did think about, but I kind of don't like it for 2 reasons:
- It modifies props on a function which isn't strictly ours - in other words
foo.bar.restore()
could potentially exist before we makefoo.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 forchai.spy.returns()
overspy = chai.spy(); spy.returns()
), or a.getCall
... they all start getting tacked onto the spy and each one causes new potential issues. - Having
restore()
andrestoreAll()
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.
from chai-spies.
@ketihamus you have convinced me. I am looking forward to test the sandboxes on chai.spy.
from chai-spies.
😄
from chai-spies.
@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.
from chai-spies.
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.
from chai-spies.
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.
from chai-spies.
@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.
from chai-spies.
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 environmentschai.spy.sandbox()
returns an object withon()
,object()
andrestore()
. Each method spies on is tracked within the sandbox.sandbox.restore()
should set all spies created from the sandbox to their original functionschai.spy.on()
,chai.spy.object()
andchai.spy.restore()
should be bound to a top-level sandbox.chai.spy()
andchai.spy.returns()
should still work - and not be bound to a sandboxchai.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.
from chai-spies.
@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:
- Make spies being mutable (don't like as it leads to writing non-logical code)
- Add instance method
returns
to spies which exists only forspy.on
created spies (this one has the same disadvantages asrestore
instance method) - Return
link
object fromspy.on
which hasreturns
method (probably others in future). For spying multiple object methods addchai.spy.on.all(object, method1, method2, method3)
in this case spying object is returned andreturns
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;
}
from chai-spies.
@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);
from chai-spies.
@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)
from chai-spies.
@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?
from chai-spies.
@stalniy just spitballing here, but what about, instead, doing something like chai.spy.on(array, 'push', chai.spy.returns(5))
?
from chai-spies.
@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 havecalls
,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
from chai-spies.
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));
from chai-spies.
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
from chai-spies.
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.
from chai-spies.
Other alternatives for returns
syntax may be this:
chai.spy.new(spy => spy.on(object, 'push').returns(5))
chai.spy.on(object, 'push', { returns: 5 })
chay.spy.on(object, 'push', returns => 5)
from chai-spies.
Related Issues (20)
- Expect nested method to have been called
- ChaiSpies with Chai As Promiesed w/o using Timeout HOT 3
- TypeError: Cannot use 'in' operator to search for 'Date' in global HOT 1
- Bring back spy.reset() HOT 6
- export spy function as names export from chai-spies HOT 1
- An in-range update of mocha is breaking the build 🚨 HOT 16
- Restoring issue HOT 4
- Module '"chai-spies"' has no exported member 'spies'. HOT 3
- Incorrect type definitions for `spy.interface` HOT 1
- Feature: called with reports actual arguments
- `spy.on` doesn't work with nullish prototype
- expect(...).to.have.been.called.once is not a function HOT 2
- chai-spies.js needs a rebuild on master HOT 9
- to.have.been.called.with() throws an error HOT 3
- Switch logicalparadox refs to chaijs HOT 4
- expect(spy).to.have.been.called.with always true HOT 6
- An in-range update of mocha is breaking the build 🚨 HOT 3
- An in-range update of rollup-plugin-commonjs is breaking the build 🚨 HOT 4
- Cannot attach spy on the same function more than once HOT 2
- spy.interface documentation appears to be wrong HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from chai-spies.