-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat(sandbox): adds support for chai.spy.sandbox #61
Conversation
This is the first iteration of sandboxes feature. @keithamus would like to here from you about this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
object = methods.reduce(function (object, methodName) { | ||
object[methodName] = chai.spy(name + '.' + methodName); | ||
return object; | ||
}, {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the intent, but I feel like the interface isn't so clean:
- Why overload
object
? Why not have it as something likespy.interface([...methods...], name<optional>)
? - What if I want to spy on String methods (
spy.on('foo', ['endsWith']);
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
||
if (typeof method === 'function' && !method.__spy) { | ||
sandbox.on(object, methodName); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I can add another check for undefined methods:
!(methodName in object) || typeof method === 'function' && !method.__spy
Do you think it's fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
return true; | ||
} | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'])
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, make sense. Will implement both suggestions
|
||
if (!tracked) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this silent failure path. We should throw an error here, telling the user they cannot restore a non-tracked spy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
tracked.object[tracked.methodName] = tracked.originalMethod; | ||
} else { | ||
delete tracked.object[tracked.methodName]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work here! This conditional will resolve lots of subtle errors.
delete tracked.object[tracked.methodName]; | ||
} | ||
|
||
spy.__spy.tracked = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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');
Sandbox.prototype.on = function (object, methodName) { | ||
var method = chai.spy('object.' + methodName, object[methodName]); | ||
|
||
method[ID_KEY] = ++spyAmount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the id be moved into the __spy
object instead of another assignment to the object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@@ -91,59 +251,76 @@ module.exports = function (chai, _) { | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I also think that method should be removed and this should be a part of a separate PR
return object; | ||
}, {}); | ||
chai.spy.restore = function () { | ||
return DEFAULT_SANDBOX.restore.apply(DEFAULT_SANDBOX, arguments) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like we could shorten a lot of this to just;
chai.spy = new Sandbox();
chai.spy.sandbox = () => new Sandbox();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point about regular spies. Let's keep the code as is then 👍
Also adds DEFAULT_SANDBOX and all spies from chai.spy.on are tracked under DEFAULT_SANDBOX Fixes chaijs#38
d7ad443
to
6c68099
Compare
@keithamus could you please take a look? Want to finish this today |
Hey @stalniy. This is all looking great now. Happy to merge! Thanks for your hard work and patience! |
Also adds DEFAULT_SANDBOX and all spies from chai.spy.on and chai.spy.object are tracked under DEFAULT_SANDBOX
Fixes #38