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

Docs: Fix incorrect sandbox configuration documentation. #1443

Merged
merged 1 commit into from
Jun 2, 2017

Conversation

ryan-roemer
Copy link
Contributor

@ryan-roemer ryan-roemer commented Jun 1, 2017

Purpose (TL;DR)

The documentation for sinon.sandbox is incorrect and this PR fixes it. The bug was originally identified in #617.

Background (Problem in detail)

@divmain identified that the sinon.sandbox documentation is wrong in #617 and in a fiddle http://jsfiddle.net/8k0zv6tp/

Here's a concise example:

The documentation says that calling:

var sandbox = sinon.sandbox.create({});

essentially means:

var sandbox = sinon.sandbox.create({
    // ...
    injectInto: null,
    properties: ["spy", "stub", "mock", "clock", "server", "requests"],
    useFakeTimers: true,
    useFakeServer: true
});

This is wrong. In the shell we have:

> var sandbox = sinon.sandbox.create({})
undefined
> sandbox.clock
undefined
> sandbox.server
undefined

What really happens is equivalent to:

var sandbox = sinon.sandbox.create({
    // ...
    injectInto: null,
    properties: ["spy", "stub", "mock"],
    useFakeTimers: false,
    useFakeServer: false
});

The upstream #617 issue was closed with #617 (comment) comment of:

The current documentation shows how to create a sandbox without a config and clearly states that sinon.sandbox.create(config) is an integration feature, that users are likely not to need.

I consider this solved, and am closing this. Feel free to create a PR for the new documentation if you have improvements for this area of the documentation.

As part of the public API, some of our biggest clients use sandbox exclusively for large test suites for better fake management and cleanup, we have a vested interest in making sure the documentation and behavior of sandbox is correct.

So, after the issue was closed I asked "is this a bug? or a documentation bug?" and I haven't received a response. So, I took a hint from the closing comment and making the simplifying assumption it's a documentation bug and here's the requested PR!

Enjoy!

How to verify

The documentation behavior being wrong can be seen with:

$ npm install sinon
$ node
> var sandbox = sinon.sandbox.create({})
undefined

// These **should** be defined.
> sandbox.clock
undefined
> sandbox.server
undefined

// Now, let's do what the PR improved docs say.
> sandbox = sinon.sandbox.create(sinon.defaultConfig)
undefined

// These **should** be defined.
> sandbox.clock
{ now: 0,
  hrNow: 0,
  timeouts: {},
  Date: 
   { [Function: ClockDate]
// ... STUFF ...
> sandbox.server
{ logError: [Function: logError],
  requests: [],
  requestCount: 0,
  queue: [],
  responses: [] }

@coveralls
Copy link

coveralls commented Jun 1, 2017

Coverage Status

Coverage remained the same at 94.957% when pulling 55e437b on FormidableLabs:docs/sandbox-config into c6d01d8 on sinonjs:master.

@mroderick
Copy link
Member

I think this is a great contribution, thank you!

Let's get another set of eyes on it, before we merge it

@mantoni
Copy link
Member

mantoni commented Jun 2, 2017

I agree. Very good contribution. We should get rid of the defaultConfig thingy, in my opinion. The code already has a comment saying that it's exposed for backward compatibility.

@mantoni mantoni merged commit 25f3eeb into sinonjs:master Jun 2, 2017
@divmain divmain deleted the docs/sandbox-config branch June 7, 2017 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants