[feature] Spy passes through calling with `new` #1626

ProLoser · 2017-12-06T01:15:03Z

Purpose (TL;DR)

This changes the spy behavior so that if the spy is calledWithNew() then
the original function will also get called with new.

This was throwing an error if you tried spying on an ES6 class object.

Fixes #1265

How to verify

let x = {
  y: class {}
};

spy(x, 'y');

// This would normally throw during callThrough()
new x.y();

assert(x.y.calledWithNew());

Checklist for author

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

This changes the spy behavior so that if the spy is `calledWithNew()` then the original function will also get called with `new`. This was throwing an error if you tried spying on an ES6 class object.

fatso83

I think this seems like a good thing. It doesn't break any existing tests (do fix the linting though!), and fixes a valid case for ES2015.

It needs a verification test that shows this is working though!

fatso83 · 2017-12-06T07:52:33Z

lib/sinon/spy.js

+            } else {
+              returnValue = (this.func || func).apply(thisValue, args);
+            }
+
            if (thisCall.calledWithNew() && typeof returnValue !== "object") {


Perhaps nest this within the first check?

I hope this is a decent enough way to unit test this behavior

ProLoser · 2017-12-06T17:32:53Z

@fatso83 Updated.

Sorry for doing everything on the Github website vs a streamlined PR, didn't want to take time to clone/install/test/etc when it's all built into CI/CD

Using a named function makes it far easier to follow what is happening in a debugger.

fatso83 · 2017-12-07T10:55:34Z

lib/sinon/spy.js

+            if (thisCall.calledWithNew()) {
+                // Call through with `new`
+                // eslint-disable-next-line new-parens
+                returnValue = new (Function.prototype.bind.apply(this.func || func, [thisValue].concat(args)));


We already have a list of methods from the various built-in above, so I'll extract the bind as a variable and move it up to be consistent and make it a bit more readable.

fatso83 · 2017-12-07T13:13:12Z

Thank you! I had a hard time understanding what was going on, so needed to play with the code and run it in a debugger and examine it before I could accept it. Made a tiny change, but your test was sufficient to prove everything still worked.

Since the everything done here logically belonged in one commit it was ok that the history was a bit messy, as I could squash everything, including my commit. And yes, the unit test you added was fine :-)

fatso83 · 2017-12-07T13:24:08Z

Released as 4.1.3

ProLoser · 2017-12-07T17:40:40Z

lib/sinon/spy.js

@@ -195,7 +196,7 @@ var spyApi = {
            if (thisCall.calledWithNew()) {
                // Call through with `new`
                // eslint-disable-next-line new-parens


This line could probably have been removed with your addition.

ah, true. thanks! Fixed in 6ba959e

mroderick · 2017-12-08T07:23:00Z

Thank you!

Was fixed by some minor changes in the squashed commit. Ref #1626 (comment)

This changes the spy behavior so that if the spy is `calledWithNew()` then the original function will also get called with `new`. This was throwing an error if you tried spying on an ES6 class object. fixes sinonjs#1265

Was fixed by some minor changes in the squashed commit. Ref sinonjs#1626 (comment)

[feature] Spy passes through calling with new

6b8b8b3

This changes the spy behavior so that if the spy is `calledWithNew()` then the original function will also get called with `new`. This was throwing an error if you tried spying on an ES6 class object.

fatso83 requested changes Dec 6, 2017

View reviewed changes

ProLoser added 3 commits December 6, 2017 09:17

fixed PR 1626 build errors

e2efc65

Merged thisCall.calledWithNew() if-statements

2f9b8f2

Added "spy passes 'new' to underlying function" test

3d26922

I hope this is a decent enough way to unit test this behavior

ProLoser and others added 3 commits December 6, 2017 10:53

Removed trailing space in spy.js

f01fb29

Fixed lint errors in spy-test.js

3526a6b

Extract method from prototype and make test easier to debug

478e58a

Using a named function makes it far easier to follow what is happening in a debugger.

fatso83 approved these changes Dec 7, 2017

View reviewed changes

fatso83 merged commit 911c498 into sinonjs:master Dec 7, 2017

ProLoser deleted the patch-1 branch December 7, 2017 17:27

ProLoser commented Dec 7, 2017

View reviewed changes

fatso83 added a commit that referenced this pull request Dec 8, 2017

Remove unneeded eslint-disable line

6ba959e

Was fixed by some minor changes in the squashed commit. Ref #1626 (comment)

fatso83 mentioned this pull request Dec 8, 2017

Support constructor functions and ES6 classes in spies and stubs #1265

Closed

danxuliu mentioned this pull request Dec 8, 2017

Fix constructor spy in unit test with Sinon 4.1.3 nextcloud/server#7433

Merged

PVince81 mentioned this pull request Feb 11, 2018

Spy's thisValue not pointing to instance when spying constructor #1683

Open

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

Remove unneeded eslint-disable line

1dc7f3d

Was fixed by some minor changes in the squashed commit. Ref sinonjs#1626 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] Spy passes through calling with `new` #1626

[feature] Spy passes through calling with `new` #1626

ProLoser commented Dec 6, 2017 •

edited

fatso83 left a comment

fatso83 Dec 6, 2017

ProLoser commented Dec 6, 2017

fatso83 Dec 7, 2017

fatso83 commented Dec 7, 2017

fatso83 commented Dec 7, 2017

ProLoser Dec 7, 2017

fatso83 Dec 8, 2017 •

edited

mroderick commented Dec 8, 2017

[feature] Spy passes through calling with new #1626

[feature] Spy passes through calling with new #1626

Conversation

ProLoser commented Dec 6, 2017 • edited

Purpose (TL;DR)

How to verify

Checklist for author

fatso83 left a comment

Choose a reason for hiding this comment

fatso83 Dec 6, 2017

Choose a reason for hiding this comment

ProLoser commented Dec 6, 2017

fatso83 Dec 7, 2017

Choose a reason for hiding this comment

fatso83 commented Dec 7, 2017

fatso83 commented Dec 7, 2017

ProLoser Dec 7, 2017

Choose a reason for hiding this comment

fatso83 Dec 8, 2017 • edited

Choose a reason for hiding this comment

mroderick commented Dec 8, 2017

[feature] Spy passes through calling with `new` #1626

[feature] Spy passes through calling with `new` #1626

ProLoser commented Dec 6, 2017 •

edited

fatso83 Dec 8, 2017 •

edited