Backwards incompatible change for sinon.createStubInstance between version 1.17 and 1.16

[sterling]

It looks like createStubInstance does not work the same in version 1.17 when compared to 1.16. I have two examples of using createStubInstance, but each example only works in their respective versions.

works in 1.17

'use strict';

let sinon = require('sinon');
let assert = require('assert');

class a {
  constructor() {
    console.log('called constructor');
  }

  foo() {
    return 'bar';
  }
}

let mock = sinon.createStubInstance(a);
mock.foo.returns('baz');

assert.equal(mock.foo(), 'baz');

works in 1.16

'use strict';

let sinon = require('sinon');
let assert = require('assert');

class a {
  constructor() {
    console.log('called constructor');
  }

  foo() {
    return 'bar';
  }
}

let mock = sinon.createStubInstance(a);
sinon.stub(mock, 'foo').returns('baz');

assert.equal(mock.foo(), 'baz');

Notice the differences for stubbing foo.

sinon.stub(mock, 'foo').returns('baz') throws TypeError: Attempted to wrap foo which is already wrapped in 1.17 and mock.foo.returns('baz') throws TypeError: mock.foo.returns is not a function in 1.16.

Apparently, 1.17 stubs each of the methods when creating an instance whereas 1.16 did not.

[mantoni]

There was a bug in 1.17.0. Can you make sure you have 1.17.1 installed?

[sterling]

I have tested in 1.17.1 and it behaves the same.

[kyleseely]

This broke all my tests when upgrading to 1.17.1 from 1.16.1

[traviskaufman]

starting with 1.17 createStubInstance now stubs all non-enumerable properties defined on an object, except for constructor and properties inherited from Object.prototype. This was done primarily to support your use case above: you should not have to re-mock a method defined in a class declaration when you create a stubbed instance from that constructor. This change is why you get an error about wrapping an already wrapped function in 1.17. @mantoni I'd be happy to update any docs / Changelogs with this info to make the explanation more visible outside of this issue.

[traviskaufman]

more context: #795

[mantoni]

I just checked the documentation again and I think it's short and to the point:

Creates a new object with the given function as the protoype and stubs all implemented functions. The given constructor function is not invoked.

@sterlinghw @skyle143 I think your test cases used to work coincidentally, but it was never an intention that the result of createStubInstance is stubbed again.

I'm tempted to close this, but as it's failing existing test cases, I'd like to hear if @cjohansen, @mroderick or @fatso83 have more thoughts to add?

[kyleseely]

My only complaint it that the functionality changed and should have been a major version bump to not break existing tests. I actually prefer the new behavior.

[sterling]

Like @skyle143 mentioned, my main point is that it's a non-backwards-compatible change. Since I assume that a minor version bump will not introduce backwards incompatible changes, I ran into this issue. I would think that this could hurt others coding under this same assumption.

[fatso83]

Agree with Kyle on the principle of bumping the major version as it is a
breaking API change.

On the other hand, should we invest more efforts into the 1.x branch, or
simply say the cat's out of the bag on this one, and say people should
stick to 1.16 of they need the old behavior? Depends on the cost of
implementing the old behavior, I would say.
30. sep. 2015 8.54 p.m. skrev "Kyle Seely" [email protected]:

My only complaint it that the functionality changed and should have been a
major version bump to not break existing tests. I actually prefer the new
behavior.

—
Reply to this email directly or view it on GitHub
#860 (comment).

[mroderick]

I vote for bumping the major version (publish from master, there are other changes). We might have to retire the 1.17.x releases so far, so people won't use them, thinking they're safe. We can create a 1.x branch, that we will keep updating for some time (six months?).

[mantoni]

I understand the problem a lot better after trying to fix #867. Essentially, classes where never stubbed at all in Sinon 1.16. Using createStubInstance on a class produced the same result as Object.create. I would say that 1.17 is the first release that allows creating stub instances of classes.

I would even argue that it could have been a patch to fix the missleading behavior where createStubInstance(myClass) produced an object where the functions wheren't stubbed. Therefore I don't agree that this is breaking existing functionality. I wouldn't retire 1.17.x. Does that make sense?

[sterling]

It may be "wrong" behavior in 1.16 to not have any methods stubbed on the class object, but regardless, if people are relying on this behavior, the api shouldn't change. Simply upgrading sinon from a minor version bump should never break tests.

[mantoni]

I understand your point, and it's generally valid. But we didn't change any semantics and we repaired a broken behavior. In this particular case, you're asking to bump the major because your tests where relying on a bug in Sinon. If you see that it's not doing the right thing and then rely on it anyway, we can't support that.

[cjohansen]

Thanks for explaining @mantoni, I agree with you.