add throwsArg(index) to stubs by seppevs · Pull Request #1319 · sinonjs/sinon

seppevs · 2017-03-08T11:42:01Z

Fixes issue #1270 by adding a throwsArg(index) method to stubs

I have added unit tests & docs.

This was requested through sinonjs#1270

coveralls · 2017-03-08T11:43:40Z

Coverage decreased (-1.4%) to 93.974% when pulling 32757dc on seppevs:add_throwsArg_to_stubs into e0cfccc on sinonjs:master.

mroderick · 2017-03-09T07:02:16Z

This PR looks good to me.

However, I cannot figure out why coveralls thinks that this should not be merged.

So, I tried to run the coverage report locally, but it doesn't generate a nice HTML representation. @mantoni do you know the magic incantation to get the mochify-istanbul to play nice and generate an HTML coverage report?

mantoni · 2017-03-09T07:37:45Z

@mroderick You can run npm run test-coverage to get a text coverage report. If you want HTML, you can add another --reporter html argument and then open coverage/index.html. While it generates the HTML report for me, it fails with a runtime error in the istanbul module. Therefore ~~I'm not sure whether~~ the report is not complete.

fearphage · 2017-04-06T03:27:52Z

I cannot figure out why coveralls thinks that this should not be merged

Coverage dropped by >= 1%. Coveralls is configured to block the commit when that happens. I'm not 100% sure why that is the case, but I believe it has to do with the way it's tracking coverage. I submitted a PR to attempt to fix this.

fearphage

Thanks for the contribution. There are just a few missing tests.

fearphage · 2017-04-06T12:02:41Z

                this.exception ||
                typeof this.returnArgAt === "number" ||
                this.returnThis ||
+                typeof this.throwArgAt === "number" ||


Please add a test.

fearphage · 2017-04-06T12:03:09Z


        delete this.returnValue;
        delete this.returnArgAt;
+        delete this.throwArgAt;


Please add a test.

fearphage · 2017-05-19T02:45:40Z

@seppevs Any plans to finish this PR? It looks like a solid addition.

seppevs · 2017-05-19T09:22:41Z

@fearphage sorry I'm quiet busy. I tried a few weeks ago to add the tests, but I couldn't find where I should add them.

fearphage · 2017-05-19T12:45:14Z

@seppevs Completely understandable. Would you mind opening/unlocking the PR so others can push to your branch to complete this PR? I'm not sure how to describe it, because I've never done it before. There is (or at least was) a checkbox somewhere in the right column that said something along the lines of "allow other collaborators to push to your PR" or something along those lines. If you check that box, someone else can help you wrap this up. Also thank you very much for your contribution.

seppevs · 2017-05-19T12:55:38Z

@fearphage "Allow edits from maintainers." is checked. The description when I hover over it:

If checked, users with write access to sinonjs/sinon can add new commits to your add_throwsArg_to_stubs branch. You can always change this setting later.

coveralls · 2017-05-19T14:18:25Z

Coverage decreased (-0.4%) to 95.033% when pulling 176dd78 on seppevs:add_throwsArg_to_stubs into e0cfccc on sinonjs:master.

fearphage · 2017-05-19T14:23:57Z

I'm going to let someone else review and merge. @sinonjs/sinon-core

mantoni · 2017-05-19T14:33:12Z

+
+Causes the stub to throw the argument exception at the provided index.
+
+`stub.returnsArg(0);` causes the stub to throw the first argument exception.


Copy pasted from returnsArg?

mantoni · 2017-05-19T14:38:52Z

        } else if (this.returnThis) {
            return context;
+        } else if (typeof this.throwArgAt === "number") {
+            throw args[this.throwArgAt];


I think this should test whether the function was called with enough arguments to throw the argument at the given index.

That makes sense. What's the expected behavior if there aren't enough params?

I think it should throw an exception. I know, it sounds ironic in this context, but an exception with a more telling message and not throwing undefined makes sense, I think. We do something similar in yield.

coveralls · 2017-05-19T15:20:28Z

Coverage decreased (-0.3%) to 95.037% when pulling 0b374b9 on seppevs:add_throwsArg_to_stubs into e0cfccc on sinonjs:master.

mantoni · 2017-05-19T15:32:30Z

For completeness, this feature should also be mirrored in the spy API. Like spy.callArg.

fearphage · 2017-05-20T00:56:56Z

@mantoni Is that the last bit? I'd prefer to do all the changes at once instead of a few at a time.

mantoni · 2017-05-20T07:23:03Z

Yes, sorry. It came to my mind too late. I didn't do this intentionally. We can also merge this as is and add the spy API separately.

coveralls · 2017-05-20T13:39:19Z

Coverage decreased (-0.3%) to 95.053% when pulling 58efb00 on seppevs:add_throwsArg_to_stubs into e0cfccc on sinonjs:master.

fearphage · 2017-05-20T13:55:04Z

@mantoni Back to you.

I filed a related issue when I went to add docs for the spy method -- #1411 Spy methods aren't documented.

mantoni · 2017-05-20T15:21:17Z

@fearphage Thank you!

mroderick · 2017-05-21T07:51:59Z

Since this is an expansion of the API, I think we should publish a new MINOR release. What do you think @fearphage @mantoni?

mantoni · 2017-05-21T08:18:57Z

@mroderick Yes, true. Are you doing it?

fearphage · 2017-05-21T12:51:49Z

I concur. Minor seems fitting.

add throwsArg(index) to stubs

32757dc

This was requested through sinonjs#1270

lucasfcosta mentioned this pull request Mar 11, 2017

Add getters/setters stub behaviors new API #1297

Merged

fearphage mentioned this pull request Apr 6, 2017

Exclude test and node_modules directories from coverage #1374

Merged

fearphage requested changes Apr 6, 2017

View reviewed changes

added additional tests

176dd78

fearphage approved these changes May 19, 2017

View reviewed changes

mantoni reviewed May 19, 2017

View reviewed changes

reviewer comments

0b374b9

added spy.throwArg to mirror stub.throwsArg

58efb00

mantoni merged commit 4f3a3ac into sinonjs:master May 20, 2017

mroderick added the semver:minor changes will cause a new minor version label May 21, 2017


		Causes the stub to throw the argument exception at the provided index.

		`stub.returnsArg(0);` causes the stub to throw the first argument exception.

Conversation

seppevs commented Mar 8, 2017

Uh oh!

coveralls commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mroderick commented Mar 9, 2017

Uh oh!

mantoni commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fearphage commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fearphage left a comment

Choose a reason for hiding this comment

Uh oh!

fearphage Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

fearphage Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

fearphage commented May 19, 2017

Uh oh!

seppevs commented May 19, 2017

Uh oh!

fearphage commented May 19, 2017

Uh oh!

seppevs commented May 19, 2017

Uh oh!

coveralls commented May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fearphage commented May 19, 2017

Uh oh!

mantoni May 19, 2017

Choose a reason for hiding this comment

Uh oh!

mantoni May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fearphage May 19, 2017

Choose a reason for hiding this comment

Uh oh!

mantoni May 19, 2017

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mantoni commented May 19, 2017

Uh oh!

fearphage commented May 20, 2017

Uh oh!

mantoni commented May 20, 2017

Uh oh!

coveralls commented May 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fearphage commented May 20, 2017

Uh oh!

mantoni commented May 20, 2017

Uh oh!

mroderick commented May 21, 2017

Uh oh!

mantoni commented May 21, 2017

Uh oh!

fearphage commented May 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Mar 8, 2017 •

edited

Loading

mantoni commented Mar 9, 2017 •

edited

Loading

fearphage commented Apr 6, 2017 •

edited

Loading

coveralls commented May 19, 2017 •

edited

Loading

mantoni May 19, 2017 •

edited

Loading

coveralls commented May 19, 2017 •

edited

Loading

coveralls commented May 20, 2017 •

edited

Loading