Skip to content

Conversation

@dpmott
Copy link
Contributor

@dpmott dpmott commented Oct 30, 2017

Purpose (TL;DR) - mandatory

Fix issue #1598 by implementing sandbox.createStubInstance(), tests, and documentation.

Background (Problem in detail) - optional

The addition of sandbox.createStubInstance() increases orthogonality with the Sinon stub interface.

Solution - optional

Implemented sandbox.createStubInstance() as a convenience wrapper around sandbox.stub(), for the case of stubbing an entire object.

How to verify - mandatory

  1. Check out this branch
  2. npm install
  3. npm test

Checklist for author

  • npm run lint passes
  • References to standard library functions are cached.
  • Verify that documentation is sufficient for new functionality
  • Verify that unit test coverage is sufficient for new functionality.

"test": "run-s test-node test-headless test-webworker",
"check-dependencies": "dependency-check package.json --unused --no-dev",
"build": "./build.js",
"build": "node ./build.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was necessary to run npm build or npm test locally on a Windows box. If it's inappropriate, then I'll happily revert this change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine. At some point we agreed that it was not a pre-requisite that the entire build process runs on Windows, as none of the maintainers were using it, but we are certainly not opposed to improving the situation :-)

assert.isFunction(collection.mock);
});

describe(".createStubInstance", function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests have largely been copied from stub-test.js.

@dpmott
Copy link
Contributor Author

dpmott commented Oct 30, 2017

Do I need to add any additional tests in sadnbox-test.js?

Copy link
Contributor

@fatso83 fatso83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mroderick mroderick merged commit 201a652 into sinonjs:master Nov 2, 2017
@mroderick mroderick added the semver:minor changes will cause a new minor version label Nov 2, 2017
@mroderick
Copy link
Member

Thank you 🚀

This has become [email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver:minor changes will cause a new minor version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants