syntax sugar for resolvesArg issue-1665 #1846
Conversation
Pull Request Test Coverage Report for Build 2554
💛 - Coveralls |
test/stub-test.js
Outdated
| var stub = createStub.create(); | ||
| stub.resolvesArg(2); | ||
|
|
||
| assert.equals(resolveSpy.callCount, 0); |
| @@ -165,6 +165,21 @@ module.exports = { | |||
| fake.fakeFn = undefined; | |||
| }, | |||
|
|
|||
| resolvesArg: function resolvesArg(fake, index) { | |||
| if (typeof index !== "number") { | |||
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.
Add some meaningful assertion to this test.
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.
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.
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.
| assert(resolveSpy.notCalled); | ||
| }); | ||
|
|
||
| it("throws if index is not a number", function () { |
There was a problem hiding this comment.
This test and the one below are essentially the same test. String and undefined are both not Numbers. I'd keep this test label/message, but use the test below (using undefined) personally. I think that's the test throwsArg uses.
|
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 installChecklist for author
npm run lintpassed