Refactor sandbox and allow it to stub getters and setters

[lucasfcosta]

Purpose (TL;DR)

This aims to fix #1401 and #781 by allowing users to stub getters and setters while using sandboxes.

In order to do this I had to refactor sandbox.stub and add new value default behavior for the sake of consistency.

Another side-effect of this PR is adding the value default behavior to stubs, allowing users to stub the value property on property descriptors.

Background (Problem in detail)

As discussed in the two issues linked above, the sandbox API did not allow our users to set getters and setters for non-function properties they stubbed, because that meant we were going to call stubNonFunctionProperty and return a raw object with a restore method. Due to this fact, the returned object would not allow us to call the get and set default behaviors.

Another problem I explained at #781 was that by having sandbox.stub to accept a third argument, it was not possible to differentiate this argument from a property descriptor when we wanted to stub non-function-properties, because we would never know whether the user wanted that value to replace that property or if the user wanted that value to be the new property descriptor.

GIven the way sandbox.stub was working, it presented a bit of duplication and error prone code, because some of the same checks we have in sinon.stub were being made differently in sandbox.stub. This could cause unwanted getter triggers when checking property values and therefore inconsistent behavior compared to sinon.stub.

Solution

This is how I solved these problems:

1. Allowing getters and setters (consistent sandbox API)
   I had to create a separate utility file called sandbox-stub.js which does the job of handling calls to the old version of the sandbox.stub API. The function in this file is called whenever a user passes a third argument to sandbox.stub. Otherwise, we always call the sinon.stub function in order to make the behavior of sandbox.stub as consistent as we can.
   This allows us to have all the default stub behaviors available on the stub created by sandbox.stub.
   Then all we have to do is add all the stubbed methods (which are more than one if we're stubbing an entire object) to the current collection.
2. Allow stubbing non-function values while still allowing get and set behaviors
   Since there is no way to differentiate a descriptor from a desired property value to replace another when we allow three arguments on the sandbox.stub API, I added a new default behavior called value. This makes the API consistent with stubbing other values on property descriptors, such as get and set.
   In order to restore a stubbed non-function property a user can just call the restore method on the created fake.

This PR also has:

• Tests for the new value behavior
• Tests for sandbox.stub being able to stub getters and setters
• Updated docs
• A minor improvement in which I add a name to a previously anonymous function on stubNonFunctionProperty
• Fixes to avoid tests for the old sandbox.stub API to log deprecation warnings to the console. Like this, for example.

How to verify - mandatory

1. Check out this branch (see github instructions below)
2. npm install
3. npm test - This runs both the old tests (proving nothing breaks) and the new ones (proving the new features work properly)

As always, any feedbacks will be great, specially given it's a long PR.

Thanks for your time!

[coveralls]

Coverage decreased (-0.2%) to 94.855% when pulling 46fd81b on lucasfcosta:refactor-sandbox-stub into bf2bfbd on sinonjs:master.

[mroderick]

Excellent work @lucasfcosta!

I don't have any comments for the code.

From reading the PR, I would say that this should be a new MINOR version. Does anyone disagree with that?

[lucasfcosta]

Thanks @mroderick! You and all the other maintainers have been doing an excellent work as well, it's great to collaborate with such good people.

I'd say this is a MINOR version too.

Since I didn't need to change any test cases in order to make it pass and the API contract is still the same (even though we've got a new deprecation warning) I'd say you're correct.

Btw, I always like to think about SemVer as breaking.feature.fix and this falls into the feature category.

[mroderick]

I've published sinon@2.3.0 which includes this

[fearphage]

I just found this in a pending state. I realize this is already merged. I'm fixing it in a separate ticket.

[fearphage]

Use beforeEach and afterEach instead. If the above assertion fails, this line will never be reached and original value of that method will be lost for the remainder of the test run.

[mantoni]

@lucasfcosta I'm floored by your work here. I usually don't work with getters a lot, but just now I had a case where this made my test trivial:

const stream = new PassThrough();
sandbox.stub(process, 'stdin').get(() => stream); // This failed without your change

I wanted to say thank you ❤️

[lucasfcosta]

@mantoni I'm happy I was able to help.

I love contributing to Sinon. All of you are always so kind, active and do great reviews. Actually I'm the one that should be thanking you for making the contribution experience so pleasurable and for taking care of this awesome codebase. ❤️

Whenever you need anything I'll be more than happy to help.