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

Allow providing stubs overrides for sinon.createStubInstance #1864

Merged
merged 2 commits into from Sep 12, 2018

Conversation

ifrost
Copy link
Contributor

@ifrost ifrost commented Jul 23, 2018

Purpose (TL;DR) - mandatory

Implements #1857. The solution is slightly different from what I suggested #1857 - it allows simple stub overrides or providing returned value as described in docs:

var stub = sinon.createStubInstance(MyConstructor, overrides);

overrides is an optional map overriding created stubs, for example:

var stub = sinon.createStubInstance(MyConstructor, {
    foo: sinon.stub().returnsThis()
});

is the same as:

var stub = sinon.createStubInstance(MyConstructor);
stub.foo.returnsThis();

If provided value is not a stub, it will be used as the returned value:

var stub = sinon.createStubInstance(MyConstructor, {
    foo: 3
});

is the same as:

var stub = sinon.createStubInstance(MyConstructor);
stub.foo.returns(3);

How to verify - mandatory

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

There's no need to create a new stub for already existing functions so
the syntax is actually shorter.
@coveralls
Copy link

coveralls commented Jul 23, 2018

Pull Request Test Coverage Report for Build 2602

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 95.261%

Totals Coverage Status
Change from base Build 2599: 0.02%
Covered Lines: 1973
Relevant Lines: 2027

💛 - Coveralls

@mroderick
Copy link
Member

Thank you for your pull request.
Things have been a bit busy lately, so I am a bit behind on my open source work.
I will get around to take a closer look at this, when I can give it the attention it deserves.
Thank you for your patience

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 added the semver:minor changes will cause a new minor version label Sep 12, 2018
@mroderick mroderick merged commit f6a0017 into sinonjs:master Sep 12, 2018
@mroderick
Copy link
Member

Thank you for your patience ❤️
This has been published as sinon@6.3.0

@fatso83
Copy link
Contributor

fatso83 commented Sep 12, 2018

Hmm ... isn't the pre-test checks running on the PRs? This failed after merging to master.

@mroderick
Copy link
Member

Hmm ... isn't the pre-test checks running on the PRs? This failed after merging to master.

I think we changed the linting since the PR was created:

https://travis-ci.org/sinonjs/sinon/jobs/427844368

@mroderick
Copy link
Member

I can fix this lint violation and update the preversion.sh to include linting

@fatso83
Copy link
Contributor

fatso83 commented Sep 12, 2018

The linting failed because of not using a cached reference to forEach. This makes it susceptible to bugs where people stub out Array.prototype.forEach in their own tests.

Even if we have just recently added the explicit check for this (good!), we did have a point in our pull request template just for this:

  • References to standard library functions are cached.

There is a reason we have this checklists, @ifrost, so removing them should only be done if you understand why they are there in the first place and if they are irrelevant.

@mroderick mroderick mentioned this pull request Sep 12, 2018
2 tasks
@mroderick
Copy link
Member

@fatso83 #1902

@mroderick
Copy link
Member

@fatso83 in fairness, those requirements were added after this PR was opened.

@fatso83
Copy link
Contributor

fatso83 commented Sep 12, 2018

@mroderick No, they were added one year ago (2017).

@mroderick
Copy link
Member

@mroderick No, they were added one year ago (2017).

Oh!

@ifrost
Copy link
Contributor Author

ifrost commented Sep 13, 2018

The linting failed because of not using a cached reference to forEach. This makes it susceptible to bugs where people stub out Array.prototype.forEach in their own tests.

Even if we have just recently added the explicit check for this (good!), we did have a point in our pull request template just for this:

  • References to standard library functions are cached.

There is a reason we have this checklists, @ifrost, so removing them should only be done if you understand why they are there in the first place and if they are irrelevant.

Sorry, my bad! Thanks for fixing it!

franck-romano pushed a commit to franck-romano/sinon that referenced this pull request Oct 1, 2019
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.

None yet

5 participants