-
-
Notifications
You must be signed in to change notification settings - Fork 770
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
syntax sugar for resolvesArg issue-1665 #1846
Conversation
Pull Request Test Coverage Report for Build 2554
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good. I have few suggestions though.
test/stub-test.js
Outdated
var stub = createStub.create(); | ||
stub.resolvesArg(2); | ||
|
||
assert.equals(resolveSpy.callCount, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert(resolveSpy.notCalled)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -165,6 +165,21 @@ module.exports = { | |||
fake.fakeFn = undefined; | |||
}, | |||
|
|||
resolvesArg: function resolvesArg(fake, index) { | |||
if (typeof index !== "number") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
test/stub-test.js
Outdated
var stub = createStub.create(); | ||
stub.rejects(Error("should be superseeded")).resolvesArg(1); | ||
|
||
return stub().then(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some meaningful assertion to this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/sinon/behavior.js
Outdated
@@ -137,6 +138,8 @@ var proto = { | |||
throw args[this.throwArgAt]; | |||
} else if (this.fakeFn) { | |||
return this.fakeFn.apply(context, args); | |||
} else if (typeof this.resolveArgAt === "number") { | |||
return (this.promiseLibrary || Promise).resolve(args[this.resolveArgAt]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will you make this more strict (like throwsArg
)? By default this will resolve to undefined
, but I think it should throw
if there are not enough arguments. Having it resolve to an nonexistent argument doesn't seem like what people expect when writing tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did the old check just like throwArgAt in this pull request.
I'm happy to use that ensureArgs if PR #1848 get merged first and uses the correct argument index number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove a duplicate test and it's good to go from end.
assert(resolveSpy.notCalled); | ||
}); | ||
|
||
it("throws if index is not a number", function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test and the one below are essentially the same test. String
and undefined
are both not Number
s. I'd keep this test label/message, but use the test below (using undefined
) personally. I think that's the test throwsArg
uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
This pull request expands the API. For that to be useful, people need to know about it. Please document this new feature in the |
I've done the doc update with a since api version 6.1.0 after @mroderick 's semver:minor label. I'll update if this is not the case. |
This has been published to NPM as |
Purpose (TL;DR) - mandatory
Fix issue #1665
Background (Problem in detail) - optional
I'm well aware that the issue was closed in favour of workarounds with existing syntax. But I can argue that issue #804 falls into the same category as:
they're doable with a few more keystrokes.
they're syntax sugars.
Basically we're saving the effort of writing:
stub.callsFake(async (params) => params)
into
stub.resolvesArg(0)
It makes sense to have a resolvesArg given that we already had returnsArg and throwsArg.
How to verify - mandatory
npm install
Checklist for author
npm run lint
passed