-
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
Added spy.returnsInOrder method #44
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR @PiotrSliwa! I'll be honest, I'm not sure about this. How often do you need logic like this? Seems like something that can simply be done like so: var items = ['first', 'second']
var spy = chai.spy(function () { return items.shift() });
spy().should.equal('first');
spy().should.equal('second'); |
Wow, thanks @keithamus , that was a quick reply;) Of course, nevertheless, by going this way you should also agree that var value = 'dummy';
var spy = chai.spy(function() { return value });
spy().should.equal(value); However, when I use a mock/spy/utility framework for testing I expect it to do it for me. The most important reason is that whenever a developer creates any logic either in production code or in test code, he's supposed to make it stable by creating a test suite for it. So, when I'd use your solution, I'd also have to write a test for it (causing a need to write a test for a testing tool). The almost-most-important reason is readability which is especially critical in tests: var items = ['first', 'second'];
chai.spy(function () { return items.shift() });
// non-descriptive: you must analyze the implementation to understand what it does
/* vs */
chai.spy.returnsInOrder(['first', 'second']);
// descriptive: you know what it does at a glance Having a framework that is able to meet all possible test scenarios is a major boost of development. I don't think making it as small as possible makes any point as it's not supposed to be used in production code. Besides, returning ordered values is a natural extension for mock/spy framework (in Mockito it's done by |
You're making good points, all of them. Is this a reasonable PR? Sure. Will it save you time? Sure. Does it remove surface area from your code and put it into ours? Sure. Do other libraries have this feature? Sure. However they don't really answer the questions which matter when it comes to merging this in:
Let's address these: common idiomThere aren't really many functions within JavaScript which will give you different results each time you call them ( majority of users will benefit from / expect it
solves a problem painlesslyI think I've addressed this above - it doesn't have that much benefit over doing it manually. Part of being a maintainer of open source is acting as a barometer for which features are desirable. We're not always going to get it right. In this instance, I believe this feature is too niche, there isn't a large enough use case. I'll leave this PR open for now, feel free to add your thoughts on this - I'd be interested to hear them. |
common idiomIn current project there's a need to spy on a repository's driver. For example: var itemIds = ['id1', 'id2']
, driverSpy = chai.spy.returnsInOrder(itemIds)
, sut = new ItemGenerator(driverSpy)
, result = sut.generate(itemIds.length);
/* (...) expectations and stuff */ I think there are plenty of external dependencies that return another value each time they're called like databases, resources (i.e. web), controllers, etc. Basically, any test for a state change could make a use of the feature. majority of users will benefit from / expect itUnfortunately, I cannot provide such data, although I believe there should be a few of them. solves a problem painlesslyI'd say yes - tested, easy to use and much more readable than user-defined solution without side-effects. Note, that var items = ['first', 'second']
var spy = chai.spy(function () { return items.shift() });
spy().should.equal(items[0]);
spy().should.equal(items[1]); won't work. |
Just to prove @keithamus point - during 8+ years of experience I've never done checks like this. So, maybe there is an easier to way to achieve the same results (e.g., add spy on higher function/method, check a bit bigger results using deep equality, move logic under loop into separate method and test it separately, etc) |
Hello *,
this is an extensions for the new returns method, but in this case it allows the spy to return multiple different values in given order. Example usage:
I had to add it for the project I'm working on. It can be helpful, for instance, when you pass a spy to another object which uses it in a loop.