Return returned value of invokant when using yields* and callsArg* #1724

ungrim97 · 2018-03-05T18:33:50Z

Purpose

Ensure that the yields* and callsArg* methods of the stub cause the stub to return the returned value of the invoked callback. Will defer to all other return value handlers (returns, returnsThis, resolves, rejects ect)

Background

#1689 allows the yield* and callArg* methods to return the value returned by the invoked callback. It is preferable that the stub methods of yields* and callsArg* provide the same functionality to allow testing of functions that are invoked as a callback on the stub that return a promise.

This prevents the need for the test to wait for Process.nextTick() to check if the promise has resolved but can instead us await or .then to do so instead. Thus guaranteeing that the promise is resolved

Solution

Adds a new property to the behaviour of yields that is set to true for all yields* and callsArg* methods in default-behaviour.js.

Ensure that if the yields property is true, and no other return setting is enabled (such as returns, returnsThis ect) then the returned value of the invoked callback is returned. by the invoke function in behaviour.js

How to verify

Check out this branch
npm install
npm test

Checklist for author

npm run lint passes
References to standard library functions are cached.

mantoni · 2018-03-05T19:00:17Z

lib/sinon/behavior.js

@@ -156,7 +158,12 @@ var proto = {
            return (this.promiseLibrary || Promise).reject(this.returnValue);
        } else if (this.callsThrough) {
            return this.stub.wrappedMethod.apply(context, args);
+        } else if (this.returnValueDefined) {


Where is returnValueDefined being set? I don't see it anywhere else.

Its set in default-behaviour.js for returns, and resolves. Not sure it was the perfect answer. Original thought of typeof this.returnValue !== "undefined". Open to suggestions

I have gone back and used typeof this.returnValue !== "undefined" as methods such as returnsThis don't set returnValueDefined

mantoni · 2018-03-05T19:01:47Z

lib/sinon/behavior.js

@@ -156,7 +158,12 @@ var proto = {
            return (this.promiseLibrary || Promise).reject(this.returnValue);
        } else if (this.callsThrough) {
            return this.stub.wrappedMethod.apply(context, args);
+        } else if (this.returnValueDefined) {
+            return this.returnValue;
+        } else if (this.yields) {


Is a new property really required? I think checking for callsArgAt would also do, no?

It would seem that callArgAt will work for this.

mantoni · 2018-03-05T19:07:12Z

test/stub-test.js

+        it("returns the result of the yielded callback", function () {
+            var stub = createStub().yields();
+            var callback = createStub().returns("return value");
+            assert.same(stub(callback), "return value");


Minor styling: Can you separate setup / testing code from assertions with a blank line? This style should be used everywhere. You did it right in the first few tests already. It makes the tests easier to read :)

mantoni · 2018-03-05T19:10:27Z

Nice pull request with very good description and testing. Thanks!

I've added a few comments. Maybe @mroderick or @fatso83 have something to add?

…ermine return value

…returnValueDefined property

ungrim97 · 2018-03-05T21:24:43Z

Add spaces between setup and assertions in tests
Use callArgAt property rather than new property
Use returnValue rather than returnValueDefined

mroderick

Great work!

Just one comment from me

mroderick · 2018-03-06T08:53:18Z

test/stub-test.js

@@ -1496,6 +1555,50 @@ describe("stub", function () {
                this.stub(callback);
            });
        });
+
+        it("plays nice with throws", function () {


plays nice with ... is not descriptive of what the expectation of the system is. If a reader has to read the implementation of the test to understand the expectation, then we're opening up for some very difficult to fix bugs/misunderstandings. We are likely to have minor defects or unclear code in the implementation, especially as code changes over the years. The expectation should be clear and specific from the beginning, so Future Developer won't misinterpret it.

I appreciate that this is a pattern that was well established in the codebase, and that you've been diligent at following established patterns. However, I think that in this case, the established pattern should be avoided.

Would you mind rephrasing the new expectations in this test file to be very specific about what expected behaviour is?

Not at all. Does "Throws takes precedent over yields return value" work here?

@mroderick Done

ungrim97 · 2018-03-06T09:17:59Z

Rename new "plays well with X" tests to have more explicit test names

…ield and other return value setters

ungrim97 · 2018-03-08T09:24:23Z

@mroderick Your requested change should be implemented now.

mantoni · 2018-03-08T21:07:06Z

@mroderick @fatso83 Feel free to merge this. I’m AFK for a couple of days.

mroderick · 2018-03-09T17:45:36Z

@ungrim97 very nice! There's a minor misspelling: "precident" => "precedent" ... would you mind fixing that before we merge it?

ungrim97 · 2018-03-12T11:08:25Z

@mroderick fixed

mroderick · 2018-03-15T07:34:40Z

This has been published as sinon@4.4.6 on npm

…inonjs#1724)

Return returned value of invokant when using yields* and callsArg*

03314a4

ungrim97 mentioned this pull request Mar 5, 2018

Change return value of yield and callArg #1689

Merged

2 tasks

mantoni reviewed Mar 5, 2018

View reviewed changes

ungrim97 added 3 commits March 5, 2018 21:18

Add spacing between test setup and assertion statements

07b2144

Use existing callArgAt property rather than new yield property to det…

542e286

…ermine return value

Use set of returnValue to determine return value of stub rather than …

ea9f9a8

…returnValueDefined property

mroderick requested changes Mar 6, 2018

View reviewed changes

Update test names to be more explicit about expected byplay between y…

9b8d3b1

…ield and other return value setters

1724: Fixed typo

0a7e336

nfriedly mentioned this pull request Mar 14, 2018

stub.onCall(#).yields(x).returns(y) returns undefined instead of y #1736

Closed

mroderick merged commit e5e6835 into sinonjs:master Mar 15, 2018

franck-romano pushed a commit to franck-romano/sinon that referenced this pull request Oct 1, 2019

Return returned value of invokant when using yields* and callsArg* (s…

e1d4c07

…inonjs#1724)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return returned value of invokant when using yields* and callsArg* #1724

Return returned value of invokant when using yields* and callsArg* #1724

ungrim97 commented Mar 5, 2018 •

edited

mantoni Mar 5, 2018

ungrim97 Mar 5, 2018

ungrim97 Mar 5, 2018

mantoni Mar 5, 2018

ungrim97 Mar 5, 2018

mantoni Mar 5, 2018

mantoni commented Mar 5, 2018

ungrim97 commented Mar 5, 2018

mroderick left a comment

mroderick Mar 6, 2018

ungrim97 Mar 6, 2018

ungrim97 Mar 6, 2018

ungrim97 commented Mar 6, 2018 •

edited

ungrim97 commented Mar 8, 2018 •

edited

mantoni commented Mar 8, 2018

mroderick commented Mar 9, 2018

ungrim97 commented Mar 12, 2018

mroderick commented Mar 15, 2018

Return returned value of invokant when using yields* and callsArg* #1724

Return returned value of invokant when using yields* and callsArg* #1724

Conversation

ungrim97 commented Mar 5, 2018 • edited

Purpose

Background

Solution

How to verify

Checklist for author

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mantoni commented Mar 5, 2018

ungrim97 commented Mar 5, 2018

mroderick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ungrim97 commented Mar 6, 2018 • edited

ungrim97 commented Mar 8, 2018 • edited

mantoni commented Mar 8, 2018

mroderick commented Mar 9, 2018

ungrim97 commented Mar 12, 2018

mroderick commented Mar 15, 2018

ungrim97 commented Mar 5, 2018 •

edited

ungrim97 commented Mar 6, 2018 •

edited

ungrim97 commented Mar 8, 2018 •

edited