Navigation Menu

Skip to content
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

Unsure how / no way to replace methods with fakes #1850

Closed
akdor1154 opened this issue Jul 2, 2018 · 10 comments
Closed

Unsure how / no way to replace methods with fakes #1850

akdor1154 opened this issue Jul 2, 2018 · 10 comments
Labels

Comments

@akdor1154
Copy link

akdor1154 commented Jul 2, 2018

Describe the bug
If I fake a method (a function with a this reference) with sinon.fake, it looks like its this reference will become broken and unbindable.

This makes fakes entirely useless for stubbing methods of OOP style code.

To Reproduce
Steps to reproduce the behavior:

const sinon = require('sinon')
const method = function() { return this.a; }
const fakeMethod = sinon.fake(method);

const o = {
  a: 'asdf',
  method: method,
  fakeMethod: fakeMethod,
  replacedMethod: method,
  explicitlyBoundMethod: method
}

sinon.replace(o, 'replacedMethod', sinon.fake(o.replacedMethod));
sinon.replace(o, 'explicitlyBoundMethod', sinon.fake(o.explicitlyBoundMethod).bind(o)); // i would hope this workaround would be considered unnecessary, even if it did work...

o.method() // 'asdf'
o.fakeMethod() // undefined
o.replacedMethod() // undefined
o.explicitlyBoundMethod() // undefined

Expected behavior
Setup as above.

o.method() // 'asdf'
o.fakeMethod() // 'asdf'
o.replacedMethod() // 'asdf'
o.explicitlyBoundMethod() // 'asdf'

Context (please complete the following information):

  • Library version: 6.0.1
  • Environment: nodejs 8 on linux from nodesource
@mroderick
Copy link
Member

I ran your example with sinon@6.0.1 and current master in node 8 and node 10 on macOS.
All the lines returned asdf as desired.

Are you still able to reproduce this issue?

@akdor1154
Copy link
Author

akdor1154 commented Jul 8, 2018

That's a very strange result, I'm testing at home now (still Linux) on Node 10 with both the REPL and a js file, and I'm still seeing undefined for my test case above. Did you copy-paste my repro directly? (I'm wondering if you hand-reproduced and wrote an arrow function somewhere there might be slightly different binding behaviour). I have a mac at work I can try but I'd probably be even more confused if there were actual platform-specific differences in behaviour here! Sinon 6.1.3 and master (by npm install sinonjs/sinon)

test.js.txt

@akdor1154
Copy link
Author

runkit repro: https://runkit.com/akdor1154/5b415c4a4999cd0012bb094d

@rgroothuijsen
Copy link
Contributor

rgroothuijsen commented Jul 18, 2018

Confirmed the issue on my own machine (Ubuntu 18.04, Node 8.10.0). The explicitly bound method workaround does seem to produce the expected result if used in a slightly different way:

sinon.fake(o.explicitlyBoundMethod.bind(o))

@stale
Copy link

stale bot commented Sep 16, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 16, 2018
@akdor1154
Copy link
Author

still no way in 6.3.3. @mroderick I'm really confused as to how you managed to get this binding to work, are you able to demonstrate anything in my runkit that's done incorrectly?

@stale stale bot removed the stale label Sep 16, 2018
@fatso83
Copy link
Contributor

fatso83 commented Sep 17, 2018

I verified this too, just replacing console.log with assert statements. Tested using 10.2, 9.6, and 8.0

``` const assert = require("assert"); const sinon = require("sinon"); const method = function() { return this.a; }; const fakeMethod = sinon.fake(method);

const o = {
a: "asdf",
method: method,
fakeMethod: fakeMethod,
replacedMethod: method,
explicitlyBoundMethod: method
};

sinon.replace(o, "replacedMethod", sinon.fake(o.replacedMethod));
sinon.replace(
o,
"explicitlyBoundMethod",
sinon.fake(o.explicitlyBoundMethod).bind(o)
); // i would hope this workaround would be considered unnecessary, even if it did work...

assert.equal(o.method(), "asdf");
assert.equal(o.fakeMethod(), undefined);
assert.equal(o.replacedMethod(), undefined);
assert.equal(o.explicitlyBoundMethod(), undefined);

</details>

@fatso83 fatso83 added the Bug label Sep 17, 2018
@fatso83
Copy link
Contributor

fatso83 commented Sep 17, 2018

EDIT: I posted something wrong. Buggy code. Fixed now
The real problem is that the fake doesn't return the value from the wrapped function. I trimmed down all the fat from the original:

It doesn't seem to use the outer context in the wrapped function.

const assert = require("assert");
const sinon = require("sinon");
const method = sinon.stub().callsFake(function() {
  return this.foo;
});

const o = {
  foo: "asdf",
  method
};

//o.fakeMethod = sinon.fake(method.bind(o)); // works
o.fakeMethod = sinon.fake(method); // doesn't use `this` when invoked

const results = o.fakeMethod();
assert.equal(method.callCount, 1);
assert.equal(o.fakeMethod.callCount, 1);
assert.equal(results, "asdf");

@fatso83
Copy link
Contributor

fatso83 commented Sep 17, 2018

OK, found a fix. It was the most obvious fix, but a bug in the driver code seemed to imply it was wrong. Submitting a PR.

fatso83 added a commit to fatso83/sinon that referenced this issue Sep 17, 2018
fatso83 added a commit to fatso83/sinon that referenced this issue Sep 17, 2018
@fatso83
Copy link
Contributor

fatso83 commented Sep 18, 2018

Published as 6.3.4

franck-romano pushed a commit to franck-romano/sinon that referenced this issue Oct 1, 2019
franck-romano pushed a commit to franck-romano/sinon that referenced this issue Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants