Idea for future milestone(s)

[mroderick]

Background

sinon.stub / sandbox.stub has become the kitchen sink of
configurable behaviour with issues that are often difficult to find and fix without regressions.

I think that the root cause of the difficulties is that it stub has far too many responsibilities.

Further, stub also has problematic usage caused by the fact that behaviour is set after it has been created, and can be redefined many times over.

var myStub;

beforeEach(function(){
    myStub = sinon.stub().resolves('apple pie :)');
});

// several hundred lines of tests later
myStub = sinon.stub().rejects('no more pie :(');

// several hundred lines of tests later
// what behaviour does myStub currently have? Can you tell without 
// reading the entire file?
// can you safely change the behaviour without affecting tests further 
// down in the file?

And then there are the more confusing scenarios

var myStub = sinon.stub()
                .withArgs(42)
                .onThirdCall()
                .resolves('apple pie')
                .rejects('no more pie')

What does that even do?

Instead of continuing to add more responsibilities to stub, I propose that instead we introduce new members to sinon, which are much narrower in scope.

The most important I can think of would be an immutable stand in for a function.

We can then later figure out what we're going to do about properties (as a separate, new, single responsibility member).

sinon.fake

A fake (the return value of calling sinon.fake) is a pure and immutable Function. It does one thing, and one thing only. It has the same behaviour on each and every call. Unlike stub, its behaviour cannot be redefined. If you need different behaviour, make a new fake.

Single responsibility

A fake can have one of these responsibilities

• resolve a Promise to a value
• reject a Promise to an Error
• return a value
• throw an Error
• yield value(s) to a callback

If you want/need side effects, and you still want the spy interface, then either use the real function, use a stub or make a custom function

sinon.replace(myObject, myMethod, sandbox.spy(function(args) {
    someFunctionWithSideEffects(args);
});

Throws errors generously

Will be generous with throwing errors when user tries to create/use them in unsupported ways.

// will throw TypeError when `config` argument has more than one property
const fake = sinon.fake({
    resolves: true, 
    returns: true
});

Uses the spy API

Except for .withArgs, as that violates immutability

Usage ideas

// will return a Promise that resolves to 'apple pie'
var fake = sinon.fake({resolves: 'apple pie'})

// will return a Promise that rejects with the provided Error, or 
// creates a generic Error using the input as message
var fake = sinon.fake({rejects: new TypeError('no more pie')});
var fake = sinon.fake({rejects: 'no more pie'});

// returns the value passed
var fake = sinon.fake({returns: 'apple pie'});

// throws the provided Error, or creates a generic Error using the 
// input as message
var fake = sinon.fake({throws: new RangeError('no more pie')});
var fake = sinon.fake({throws: 'no more pie'});

// replace a method with a fake
var fake = sinon.replace(myObject, 'methodName', sandbox.fake({
    returns: 'apple pie'
}))
// .. or use the helper method, which will use `sandbox.replace` and `
// sinon.fake`
var fake = sinon.setFake(global, 'methodName', {
    returns: 'apple pie'
});

// create an async fake
var asyncFake = sinon.asyncFake({
    returns: 'apple pie'
});

Synonyms

I don't know if fake is the best noun to use here, but I think we should try to stick to the convention of using nouns, and not stray into adjectives or verbs.

Proposed API changes

Use a default sandbox

This is something I've been considering for awhile, why don't we make a default sandbox? If people need separate sandboxes, they can still create them.

We should create a default sandbox that is used for all methods that are exposed via sinon.*.
This means that sinon.stub will become the same as sandbox.stub, which will remove the limitation of being able to stub properties using sinon.stub.

sandbox.replace

Create sandbox.replace and use that for all operations that replace anything anywhere. Expose this as sinon.replace and use the default sandbox when used this way.

This should probably have some serious input validation, so it'll only replace functions with functions, accessors with accessors, etc.

[mroderick]

Ping @sinonjs/core

[mantoni]

Good suggestions, Morgan. Thanks for bringing this up. I also think that the stub API is confusion and I like all of your suggestions. Here are some thoughts:

sinon.fake

I agree that immutability is key here. We could allow some "sane" use cases that are currently possible with stubs though.

For example, it could be a valid use case to yield and return:

sinon.fake({
  yields: [null, 42],
  returns: true
})

We can check what makes sense and what doesn't.

Also, if we support callsThrough: true as a config (which is invalid in combination with any of the behaviors properties), the new fakes could also be used instead of the "spy" API. This would be more self explaining than learning what "spy" and "stub" means in Sinon-speak 😄

Use a default sandbox

While I like this idea, it means that calling sinon.restore() after a test could revert some left-overs from other tests and lead to surprising results - or failing tests that happened to work before. The brilliant thing this would enable is to reset the global sandbox in beforeEach to make improve test isolation. 👍

sandbox.replace

I like this a lot. I understand this as a "just let me stick this thing there" utility, right?

[mroderick]

Also, if we support callsThrough: true as a config (which is invalid in combination with any of the behaviors properties), the new fakes could also be used instead of the "spy" API. This would be more self explaining than learning what "spy" and "stub" means in Sinon-speak 😄

Would that mean that we wouldn't need spy or stub at all?

[mroderick]

sandbox.replace

I like this a lot. I understand this as a "just let me stick this thing there" utility, right?

Yeah, that was the idea. Instead of overloading the same method (sinon.stub) to do many, many things, have explicit methods that do one thing only

[mantoni]

As you highlighted, the fake API is probably not going to support everything that is currently possible with spies and stubs. But yeah, I think the fake API is an opportunity to unify the stub and spy functionality.

[mroderick]

While I like this idea, it means that calling sinon.restore() after a test could revert some left-overs from other tests and lead to surprising results - or failing tests that happened to work before. The brilliant thing this would enable is to reset the global sandbox in beforeEach to make improve test isolation. 👍

It is certainly a breaking change, and should not be introduced lightly.

[mroderick]

When creating a fake, if you don't pass it a behaviour configuration, it would be equivalent to a spy.

// ~spy, records all calls, has no behaviour
const fake = sinon.fake();

// ~stub, records all calls, returns 'apple pie'
const fake = sinon.fake({
    returns: 'apple pie'
});

[mantoni]

How would you create a stub that does nothing then?

[mroderick]

How would you create a stub that does nothing then?

I am not sure I fully understand your question... but here goes

// a fake that has no behaviour
const fake = sinon.fake();

// put it in place of an existing method
sandbox.replace(myObject, 'someMethod', fake);

[mantoni]

Ah, I think I understand what you mean now: A fake is always a stub. When you said ~spy, records all calls I understood "calls through to original function". However, the fake has no knowledge about the function it is replacing – that is what sandbox.replace does.

So with that in mind, here is another proposal how we could fold the current spy functionality (as in calling through) into the new fakes:

const fake = sinon.fake(function () {
   // Any custom function
});

The given function would be called by the fake. This API makes it impossible to mix it with other behaviors. In fact, a config object would create a function that implements the specified behavior and then pass it to the fake.

The sandbox.spy(object, method) implementation could then become this:

const original = object[method];
const fake = sinon.fake(original);
sandbox.replace(object, method, fake);

Basically a one-liner 🤓

[mroderick]

Yep. Once you simplify things, then you can start re-mixing for fun 🎉 and profit 💰

[mroderick]

However, if we want to gravitate towards just using fake and no longer using spy and stub, then we should probably just leave those two alone.

[mantoni]

I'm thinking about the "next" API here. You would need sandbox.spy to have the replacement logic somewhere. As I understand it, that should be backward compatible. The stub implementation could then be deprecated.

[mroderick]

You would need sandbox.spy to have the replacement logic somewhere. As I understand it, that should be backward compatible. The stub implementation could then be deprecated.

I am not sure I follow. Could you elaborate?

[mantoni]

Sure. As I understand your proposal, you want a replacement for the over-complicated stub API. The way stubs are currently implemented is by creating a spy with a function implementing the behavior. What I'm suggesting is to do the same thing with the fake API and internally create a spy, but we wouldn't return a behavior anymore, because we want to get rid of the chaining. We'd just return the spy. This makes the fake implementation an alternative to stub with the returned function being compatible with all of the current Sinon APIs. Does that make sense or am I missing something?

[mroderick]

OK, I think we have a similar understanding 👍

Just to re-iterate, in case we missed something and so that other contributors will have the same understanding.

TL;DR

• all replacements will be done by a new utility: sandbox.replace (this currently lives in stub)
• sinon will have a default sandbox, allowing for sinon.reset and sinon.restore (should we just merge these?)
• sinon.fake — an immutable, programmable replacement for functions that records all calls
• sinon.spy
• sinon.stub

// effectively a spy that has no target
const fake = sinon.fake()

// spy on a function
const fake = sinon.fake(console.log);
const fake = sinon.fake(function() { return 'apple pie'; });

// a shorthand construction of fake with behaviour
const fake = sinon.fake({
    returns: 'apple pie'
});

// replacing an existing function with a fake
var fakeLog = sinon.fake();
sandbox.replace(console, 'log', fakeLog);

At this state, the proposal only deals with Function. We need to consider what to do about non-function properties and accessors. At the very least, we should see if we can limit sandbox.replace to only allow sane replacements.

[fatso83]

Does this mean sinon.stub() and sinon.spy() both will be deprecated in the future in favor of sinon.fake(), or just redone internally? If so, then we are essentially moving towards TestDouble's thinking. Not necessarily a bad thing, IMHO, but it might be worth considering that if lots of people find that they will need to replace all their Sinon api calls anyway for sinon.fake(), they might as well just use another library (although that would mean they would lose all their existing knowledge of Sinon's API).

[mroderick]

Does this mean sinon.stub() and sinon.spy() both will be deprecated in the future in favor of sinon.fake(), or just redone internally? If so, then we are essentially moving towards TestDouble's thinking.

I guess it does overlap somewhat. My primary motivation for this proposal is to have fake functions with immutable behaviour.

Not necessarily a bad thing, IMHO, but it might be worth considering that if lots of people find that they will need to replace all their Sinon api calls anyway for sinon.fake(), they might as well just use another library (although that would mean they would lose all their existing knowledge of Sinon's API).

If people find that another library better serve their needs, then I am happy the we helped them learn that :)

[fatso83]

But will we keep the spy and stub methods or deprecate them, with some possible reductions in functionality? That was unclear to me.

[mroderick]

But will we keep the spy and stub methods or deprecate them, with some possible reductions in functionality?

Once fake looks stable, then I would deprecate spy and stub, and then give it like a year to allow people time to upgrade.

[mroderick]

I think we should try our best to provide codemods and great documentation, to help people move their code

[mroderick]

I am working on a branch for the first parts of this (default sandbox). I have refactored the code so that sandbox and collection are now one. I've gotten the default sandbox working.

I'll tidy up the commits over the next few days, and then create a branch on this repository for the updated API.

[lucasfcosta]

This is a great idea, very well written btw.

I'd also add deprecation notices to stubs and spies.

I was also thinking about maybe changing passing an object with keys by passing functions.

This would add the following benefits:

• This would allow us to add a type to those functions for user that want to use typescript or other kind of static checkers
• Users would get errors when trying to invoke functions for behaviors that do not exist
• We could document those functions separately and make the docs even better
• We could provide useful errors when passing arguments that do not make sense to those behaviors and allow them to have optional/more than one arguments
• It would make things more composable as well (even though I don't see many cases for this in this case) and allow people to reuse created behaviors
• IMO this would also be simpler than having an object with behavior

Therefore, the API would look like this instead:

// It would be cool to allow users to import these using destructuring to make code more concise
import { resolves, rejects, returns } from 'sinon/behaviors'; 

var fake = sinon.fake(resolves('apple pie'))

var fake = sinon.fake(rejects(new TypeError('no more pie')));
var fake = sinon.fake(rejects('no more pie'));

var fake = sinon.fake(returns('apple pie'));

var fake = sinon.fake(throws(new RangeError('no more pie'));
var fake = sinon.fake(throws('no more pie'));

When it comes to implementing this it could just be a matter of returning very simple objects like the ones you are proposing. Then, if we have more than one behavior we can just merge them.

Also, when it comes to mixing stuff like onThirdCall and withArgs I think that what happens in those cases should be documented.

Sorry for reviewing this so late. The last few months have been very busy.

[mroderick]

@lucasfcosta check out the PR #1586

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[mlippert]

The 5.0.0 previous version is causing problems w/ the later 5.0.0-next.* prerelease versions in package.json because 5.0.0 is greater than any prerelease version.

Since 5.0.0 is out there, I think the next prerelease numbers need to be bumped perhaps to 5.0.1-next.1?

I noticed this because another package I was using was getting a deprecated msg and its package.json depends on "sinon": "^5.0.0-next.4"

npm WARN deprecated sinon@5.0.0: this version has been deprecated

I wasn't sure if this was worthy of opening a new issue for a prerelease problem, so a comment here seemed safest.

[mroderick]

Another solution would be to release the next major version. What do you think @sinonjs/core?

[mantoni]

@mroderick I can't tell anymore what all the changes for v5 are. From my last tests it worked fine and I'm looking forward to using the new fakes. It's a new major, so hey, ship it 😄

[mroderick]

There is just one more PR #1764 that I'd like to get merged, before we release the next major version.

I've published sinon@5.0.1-next.1, hopefully that'll make life easier for people in the meantime.

[mlippert]

Thanks, I've tested (always good to double check) dependencies in package.json and "sinon": "^5.0.1" gives an error as it should because there is no match found (no release yet), and "sinon": "^5.0.1-next.1" works properly getting that version.

This was never a big deal, I just thought it was worth making you aware, particularly when I saw that v5 had been in development for a while so I wasn't sure how long until it was released. I think releasing in the near future sounds like a good idea.

[mroderick]

fake has been introduced with #1768, which became sinon@5.0.2