Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PiotrSliwa
Copy link
Contributor

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:

var spy = chai.spy.returnsInOrder('first', 'second');
spy().should.equal('first');
spy().should.equal('second');

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.

@keithamus
Copy link
Member

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');

@PiotrSliwa
Copy link
Contributor Author

Wow, thanks @keithamus , that was a quick reply;)

Of course, nevertheless, by going this way you should also agree that chai.spy.returns can be replaced even easier by

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 .thenReturn(value1, value2), in GoogleMock by InSequence, etc.) and I think chai-spies is great enough to have it too ;)

@keithamus
Copy link
Member

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:

  • Is the behaviour a common idiom.
  • Is it something a majority of our users will benefit from.
  • Is it something a majority of our users expect to exist.
  • Does it solve a problem painlessly, which traditionally causes a lot of pain.

Let's address these:

common idiom

There aren't really many functions within JavaScript which will give you different results each time you call them (Array#shift() and Date.now() are the only two that spring to mind).

majority of users will benefit from / expect it

chai.spy.returns() was added because many users wanted it. See #35 and #39 - people are really keen to use the feature (not just one, but a handful of different users are vocal about wanting it).

solves a problem painlessly

I think I've addressed this above - it doesn't have that much benefit over doing it manually. returns() doesn't either - but it does answer the other questions.

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.

@PiotrSliwa
Copy link
Contributor Author

common idiom

In 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 it

Unfortunately, I cannot provide such data, although I believe there should be a few of them.

solves a problem painlessly

I'd say yes - tested, easy to use and much more readable than user-defined solution without side-effects. Note, that .shift() removes the first element from the array, thus

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.

@stalniy
Copy link
Contributor

stalniy commented Dec 10, 2016

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants