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.

Sign up for GitHub

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

Jump to bottom

stub#withArgs: set promiseLibrary correctly #1497

Merged
merged 2 commits into from Jul 26, 2017
Merged

stub#withArgs: set promiseLibrary correctly #1497

merged 2 commits into from Jul 26, 2017

Conversation

HugoMuller
Copy link
Contributor

Purpose (TL;DR) - mandatory

Fix issue #1474 (again) by copying stub promiseLibrary to its new fakes when calling stub#withArgs.

Background (Problem in detail) - optional

The problem was similar to the one identified with stub#onCall: stub fakes created with withArgs did not preserve the promise library defined when calling usingPromise on the original stub.
There "defaultBehavior"s were missing the property promiseLibrary.

Solution - optional

The same patch was applied here.
Extending the new fake with the promiseLibrary from the stub.defaultBehavior solves the issue.

How to verify - mandatory

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

@HugoMuller HugoMuller mentioned this pull request Jul 26, 2017
Closed
@coveralls
Copy link

coveralls commented Jul 26, 2017 •
edited

Coverage Status

Coverage increased (+0.009%) to 95.011% when pulling 5e73b6f on HugoMuller:issue-1474-promise-withArgs into 20eb865 on sinonjs:master.

mroderick
mroderick requested changes Jul 26, 2017
View reviewed changes
Copy link
Member

@mroderick mroderick left a comment

Choose a reason for hiding this comment

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

Thank you for the speedy turnaround!

I only have the one very minor comment

.gitignore Outdated
@@ -5,3 +5,5 @@ coverage/
.idea
.sass_cache
_site
.vscode
Copy link
Member

Choose a reason for hiding this comment

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

The .idea one was a mistake that got merged at some point. Let's not make more of those.
Please put these lines in your global .gitignore instead.

@coveralls
Copy link

coveralls commented Jul 26, 2017 •
edited

Coverage Status

Coverage increased (+0.009%) to 95.011% when pulling 4194755 on HugoMuller:issue-1474-promise-withArgs into 20eb865 on sinonjs:master.

@mroderick mroderick merged commit d5e4f04 into sinonjs:master Jul 26, 2017
@mroderick
Copy link
Member

Thank you!

@mroderick mroderick added the semver:patch changes will cause a new patch version label Jul 26, 2017
@mroderick
Copy link
Member

This has become v2.4.1

@HugoMuller HugoMuller deleted the issue-1474-promise-withArgs branch July 26, 2017 14:20
franck-romano pushed a commit to franck-romano/sinon that referenced this pull request Oct 1, 2019
@HugoMuller
stub#withArgs: set promiseLibrary correctly (sinonjs#1497)
6e2111e 
* stub#withArgs:  set promiseLibrary correctly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mroderick mroderick mroderick approved these changes

Assignees
No one assigned
Labels
semver:patch changes will cause a new patch version
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@HugoMuller @coveralls @mroderick