New API: default sandbox and fake #1586

mroderick · 2017-10-11T09:09:39Z

This PR is a solution for the proposal in #1559

This PR should not be merged into master until we have given it a good trial in real world projects.

Contributions (code, fixes, tests, documentation) are very welcome. Please make pull requests into this branch.

I'll keep the branch up to date by pulling from master at intervals. Once we're happy with the branch, then I'll rebase against master and get rid of the merge commits.

Tasks

To try it out in your project directly from source

Clone the repo
git checkout default-sandbox-and-fake
npm link
In your project: npm link sinon
Run your tests

Once we start publishing release candidates on npm, then we'll add instructions for testing in your projects here.

fatso83 · 2017-10-11T19:24:14Z

lib/sinon.js

+        create: deprecated.wrap(
+            createSandbox,
+            // eslint-disable-next-line max-len
+            "`sandbox.create()` is deprecated. Use default sandbox at `sinon.sandbox` or create new sandboxes with `sinon.createSandbox()` or `new sinon.Sandbox()`"


create new sandboxes with sinon.createSandbox() or `new sinon.Sandbox()

Why have two ways of creating sandboxes? We just made a new factory function.

I'll remove the second way and tidy up the documentation. Thanks

mroderick · 2017-10-14T13:56:10Z

Should we expose the Sandbox prop if we don't mean to mean to make it public?

In the current version of the branch (5883852), that property is not exposed.

> var sinon = require('./lib/sinon');
undefined

> typeof sinon.Sandbox
'undefined'

mroderick · 2017-12-28T12:24:16Z

Ping @sinonjs/sinon-core

This is getting close to being reviewable, however two of the tests are failing in Chromium.

Any ideas why that might be?

mantoni · 2017-12-29T15:07:28Z

@mroderick Nice work! I had a look at the failing tests. It's proxyquire changing the instance, so actual instanceof Sandbox also returns false. What you can do to fix the tests is adding sinon = require("../lib/sinon"); or use proxyquire only where it's actually needed.

fearphage · 2017-12-29T15:26:59Z

docs/release-source/release/stubs.md

@@ -178,6 +176,10 @@ This is equivalent to calling both `stub.resetBehavior()` and `stub.resetHistory

 *Updated in `[email protected]`*

+*Since `[email protected]`*
+
+You can reset all stubs using `sinon.reset()`


Clarify what this resets. I assume it's behavior and history here, right?

fearphage · 2017-12-29T15:27:29Z

docs/release-source/release/stubs.md

+
+*Since `[email protected]`*
+
+You can reset behaviour of all stubs using `sinon.resetHistory()`


Typo: resetHistory doesn't reset behavior.

Also do we consistently use the Old English (British?) spelling of behavior in the docs? The actual method name uses the American English spelling.

Also do we consistently use the Old English (British?) spelling of behavior in the docs? The actual method name uses the American English spelling.

My natural language uses (mostly) British English spelling. Sometimes when writing code and documentation, that spelling slips through.

(I wonder if there's a spelling linter where you can set the language preference)

mroderick · 2018-01-01T11:31:16Z

Tests are passing now (Travis was having a bit of a day). If you don't have any further comments for this PR, then I'll rewrite the history to read better.

Do we need more documentation of fake? Are you happy with me publishing it as sinon@next?

fearphage · 2018-01-01T17:42:49Z

test/fake-test.js

+
+function noop() {}
+
+var hasFunctionNameSupport = typeof noop.name !== "undefined";


noop.name === 'noop' is a bit more straight forward.

Good point, thanks :)

coveralls · 2018-01-02T13:14:19Z

Coverage increased (+0.02%) to 96.516% when pulling e9a978b on default-sandbox-and-fake into ca9e2fa on master.

lucasfcosta

This idea is great!

This solves all the problems we've had with inconsistent APIs and regressions and adds a better and simpler API at the same time.

I can't wait to get this on the codebase!

Awesome job.

I've left a few comments regarding the docs, but I still gotta review the code itself.

I was really busy in the last couple months because I was moving to London, so now that I'm all set up I will be able to come back and work with you some more 😄

(I missed y'all btw)

lucasfcosta · 2018-01-03T22:33:24Z

docs/release-source/release/fakes.md

+
+### Fakes with behaviour
+
+Fakes can be created with behaviour, which cannot be changed once the fake has been created.


IMO it would be good to point out that it is possible to do a restore as mentioned here and that would be enough for the user to get the original property back into the object.

As a user I think that would be something I'd like to know how to do as soon as I read that:

[a fake] cannot be changed once the fake has been created

replace and restore belong together. Fakes are created separately from replacing properties.

Fakes are just one possible argument to replace, you could use something else if you wanted to (like mock, spy or stub ... or even Date).

I'd like to keep the documentation focused, so not mention topics that are not necessary for understanding the current topic. With that in mind, I think that mentioning replace in the documentation about creating fakes with behaviour, would reduce the focus and make the text a bit confusing.

I totally understand, but then maybe we could rephrase this to make it more specific.

The first time I read this I had the strong impression that you would not be able to restore any property replaced with a fake.

The wording is a bit difficult though, what do y'all think? 🤔

@lucasfcosta I think the documentation already points out that replace is a sandbox method that may use a fake as an argument. The standbox can then be restored. There is nothing to restore in the fake itself. I agree with @mroderick here that talking about restore here would make the documentation more confusing.

lucasfcosta · 2018-01-03T22:34:55Z

docs/release-source/release/fakes.md

+// 42
+```
+
+When you want to restore the replaced properties, `sinon.restore` method.


Even though this is really simple it might be worth having an example added.

Also, I'd add another header for this to make it as visible as possible given the importance of this method.

Good point, I've pushed new commits

lucasfcosta · 2018-01-03T22:36:01Z

docs/release-source/release/sandbox.md


-Creates a sandbox object with spies, stubs, and mocks.
+Since `[email protected]`, the `sinon` object is a default sandbox. Unless you have a very advanced setup or need a special configuration, you probably want to just use that one.


I think I'd also add a line here explaining why this change happened and how this makes testing easier, especially mentioning how useful it is to clear/restore all methods at once without having to create lots of sandboxes on each test.

At first I agreed with you, and wrote some documentation.

But then I thought about it some more. The motivation is only really interesting when you're transitioning from earlier versions to this MAJOR version. Once you're over that hump, stating that in the documentation just adds noise.

Instead, I've put details into the migration guide for v5, and have pushed a new commit with that.

lucasfcosta · 2018-01-03T22:37:59Z

docs/release-source/release/sandbox.md

+
+`replacement` can be any value, including `spies`, `stubs` and `fakes`.
+
+This method only works on non-accessor properties, for replacing accessors, use `sandbox.replaceGetter()` and `sandbox.replaceSetter()`.


This is great idea btw.

Since we've been dealing with getters/setters we've had a lot of trouble making the APIs generic enough to deal with them, but I guess that encapsulating that logic into separate methods is a lot better and allows us to avoid having lots of special conditions and a complex control-flow.

mroderick · 2018-01-09T09:17:34Z

I've tried using this branch with an internal project with 1793 tests, most of them using Sinon. I am happy to report, that there were no problems.

It would be great if someone else could take it for a spin, before we publish it as sinon@next.

mantoni · 2018-01-09T10:05:39Z

docs/guides/migrating-to-5.0.md

+
+describe('myFunction', function() {
+    afterEach(function () {
+        this.sandbox.restore();


You saw nothing! Nothing!

fatso83 · 2018-01-09T12:49:57Z

I also

checked out this branch
ran npm link in the sinon repo
ran npm link sinon and npm test in a project with 200 tests - many sinon

All green 😄

Word of warning: I write really simple tests and I don't have access to projects from my previous employers that had much more elaborate setups. But at least initial impressions are good! And the fact that internal sinon tests pass is very promising.

mroderick · 2018-01-09T16:03:52Z

Thanks @fatso83. I'll try to find some time in near future to put a sinon@next on npm, then we can ask the bigger audience to try it out.

mroderick · 2018-01-13T12:36:52Z

This is now on npm as sinon@next

IE doesn't have support for name property on Function. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/name

Explain how to update to use the default sandbox

Inspired by https://remysharp.com/2018/01/10/open-source-with-a-cap-in-hand Implementation shamelessly copied from https://github.com/remy/nodemon/blob/9d498523d750496aba1fb0f44c2befe42cb9d9f7/package.json#L42

mroderick · 2018-02-25T08:30:57Z

This has been replaced by #1700

* Update tests to include bucket:create permission * Use sinon.createSandbox instead of sinon.sandbox.create This seems to be the first of many consequences from sinonjs/sinon#1586 and sinonjs/sinon#1700. I'm not sure what the final result of all of this will be -- we might not even need this sandbox any more, because a default one is created for us -- but at some point in the future, we'll probably have to rewrite everything that uses a stub(), and I prefer to wait until then to make any drastic changes. This just silences the warnings until then. * Try to make tests work on both kinto versions

mroderick added the semver:major changes will cause a new major version label Oct 11, 2017

fatso83 reviewed Oct 11, 2017

View reviewed changes

mroderick force-pushed the default-sandbox-and-fake branch 2 times, most recently from 7b2953a to dc8e063 Compare November 11, 2017 09:07

mroderick force-pushed the default-sandbox-and-fake branch from 695b75f to 25c97fb Compare December 28, 2017 12:18

fearphage reviewed Dec 29, 2017

View reviewed changes

sinonjs deleted a comment from coveralls Jan 1, 2018

fearphage reviewed Jan 1, 2018

View reviewed changes

sinonjs deleted a comment from coveralls Jan 2, 2018

lucasfcosta requested changes Jan 3, 2018

View reviewed changes

mroderick mentioned this pull request Jan 4, 2018

Idea for future milestone(s) #1559

Closed

mroderick force-pushed the default-sandbox-and-fake branch from e9a978b to ed3c9dc Compare January 9, 2018 08:47

mantoni reviewed Jan 9, 2018

View reviewed changes

mroderick force-pushed the default-sandbox-and-fake branch from f3bc96e to 5650f00 Compare January 9, 2018 11:02

mroderick force-pushed the default-sandbox-and-fake branch from b2353c5 to 9b10f93 Compare January 13, 2018 12:51

fatso83 mentioned this pull request Jan 15, 2018

It would be useful that yield, yieldTo, etc returns the return value of the yielded function #903

Closed

mroderick force-pushed the default-sandbox-and-fake branch 2 times, most recently from b4e2234 to a9a7cc0 Compare January 16, 2018 19:52

mroderick added 23 commits February 24, 2018 08:34

Fix typo in documentation

a0fbd28

Extract nextTick to own file

6a51394

Add yields and yieldsTo to fake

4f3fa63

Improve language of fake documentation

60682fa

Fix broken test in IE11

a7fa009

IE doesn't have support for name property on Function. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/name

Simplify hasFunctionNameSupport

c188795

Docs: Added example for how to use restore

222a0c5

Docs: add migration guide for v5

a994b5a

Explain how to update to use the default sandbox

Add postinstall banner pointing to Open Collective

10c04e8

Inspired by https://remysharp.com/2018/01/10/open-source-with-a-cap-in-hand Implementation shamelessly copied from https://github.com/remy/nodemon/blob/9d498523d750496aba1fb0f44c2befe42cb9d9f7/package.json#L42

Update History.md and AUTHORS for new release

fc0246b

5.0.0

85ceb18

Add release documentation for v5.0.0

d67d808

Update docs/changelog.md and set new release id in docs/_config.yml

cd815d6

Remove deprecated spy.reset method

f2ccca6

Set new package version

fac8ed5

Fixup: document removal of spy.reset in migration guide

7fd2ef0

Fixup: bad rebase

9451299

Update History.md and AUTHORS for new release

efbc9c5

5.0.0-next.2

607360f

Update History.md and AUTHORS for new release

0aaec00

5.0.0-next.3

66663da

Add release documentation for v5.0.0-next.3

2e2f632

Update docs/changelog.md and set new release id in docs/_config.yml

5767a33

mroderick force-pushed the default-sandbox-and-fake branch from a9a7cc0 to 5767a33 Compare February 24, 2018 08:37

mroderick mentioned this pull request Feb 25, 2018

Next fakes #1700

Merged

11 tasks

mroderick closed this Feb 25, 2018

mroderick deleted the default-sandbox-and-fake branch February 25, 2018 08:31

rsimha mentioned this pull request Jul 31, 2018

🏗 Upgrade sinon to 6.1.4 and forbid importing of sinon ampproject/amphtml#17197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New API: default sandbox and fake #1586

New API: default sandbox and fake #1586

mroderick commented Oct 11, 2017 •

edited

Loading

fatso83 Oct 11, 2017 •

edited

Loading

mroderick Oct 12, 2017

mroderick commented Oct 14, 2017

mroderick commented Dec 28, 2017

mantoni commented Dec 29, 2017

fearphage Dec 29, 2017

fearphage Dec 29, 2017 •

edited

Loading

mroderick Dec 30, 2017

mroderick commented Jan 1, 2018

fearphage Jan 1, 2018

mroderick Jan 2, 2018

coveralls commented Jan 2, 2018

lucasfcosta left a comment

lucasfcosta Jan 3, 2018

mroderick Jan 9, 2018

lucasfcosta Jan 10, 2018

mantoni Jan 16, 2018

lucasfcosta Jan 3, 2018

mroderick Jan 9, 2018

lucasfcosta Jan 3, 2018

mroderick Jan 9, 2018

lucasfcosta Jan 3, 2018

mroderick commented Jan 9, 2018

mantoni Jan 9, 2018

mroderick Jan 9, 2018

fatso83 commented Jan 9, 2018

mroderick commented Jan 9, 2018

mroderick commented Jan 13, 2018

mroderick commented Feb 25, 2018


		Since `[email protected]`

		You can reset behaviour of all stubs using `sinon.resetHistory()`


		function noop() {}

		var hasFunctionNameSupport = typeof noop.name !== "undefined";


		### Fakes with behaviour

		Fakes can be created with behaviour, which cannot be changed once the fake has been created.


		Creates a sandbox object with spies, stubs, and mocks.
		Since `[email protected]`, the `sinon` object is a default sandbox. Unless you have a very advanced setup or need a special configuration, you probably want to just use that one.


		`replacement` can be any value, including `spies`, `stubs` and `fakes`.

		This method only works on non-accessor properties, for replacing accessors, use `sandbox.replaceGetter()` and `sandbox.replaceSetter()`.

New API: default sandbox and fake #1586

New API: default sandbox and fake #1586

Conversation

mroderick commented Oct 11, 2017 • edited Loading

Tasks

To try it out in your project directly from source

fatso83 Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroderick commented Oct 14, 2017

mroderick commented Dec 28, 2017

mantoni commented Dec 29, 2017

Choose a reason for hiding this comment

fearphage Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroderick commented Jan 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 2, 2018

lucasfcosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroderick commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fatso83 commented Jan 9, 2018

mroderick commented Jan 9, 2018

mroderick commented Jan 13, 2018

mroderick commented Feb 25, 2018

mroderick commented Oct 11, 2017 •

edited

Loading

fatso83 Oct 11, 2017 •

edited

Loading

fearphage Dec 29, 2017 •

edited

Loading