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

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

Merged
merged 7 commits into from Dec 7, 2017

Conversation

Projects
None yet
3 participants
@ProLoser
Contributor

ProLoser commented Dec 6, 2017

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.
[feature] Spy passes through calling with `new`
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!

Show outdated Hide outdated lib/sinon/spy.js Outdated

ProLoser added some commits Dec 6, 2017

Added "spy passes 'new' to underlying function" test
I hope this is a decent enough way to unit test this behavior
@ProLoser

This comment has been minimized.

Show comment
Hide comment
@ProLoser

ProLoser Dec 6, 2017

Contributor

@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

Contributor

ProLoser commented Dec 6, 2017

@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

ProLoser and others added some commits Dec 6, 2017

Extract method from prototype and make test easier to debug
Using a named function makes it far easier to follow what is happening
in a debugger.
@fatso83

fatso83 approved these changes Dec 7, 2017

Show outdated Hide outdated lib/sinon/spy.js Outdated

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fatso83

This comment has been minimized.

Show comment
Hide comment
@fatso83

fatso83 Dec 7, 2017

Contributor

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 :-)

Contributor

fatso83 commented Dec 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@fatso83
Contributor

fatso83 commented Dec 7, 2017

@ProLoser ProLoser deleted the ProLoser:patch-1 branch Dec 7, 2017

Show outdated Hide outdated lib/sinon/spy.js Outdated
@mroderick

This comment has been minimized.

Show comment
Hide comment
@mroderick

mroderick Dec 8, 2017

Member

Thank you!

Member

mroderick commented Dec 8, 2017

Thank you!

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

Remove unneeded eslint-disable line
Was fixed by some minor changes in the squashed commit.
Ref #1626 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment