sinon.stub an object with 'watch' function doesn't stub the watch function

[ajdaniel]

• Sinon version : 1.12.2 [Edit, 1.17.3]
• Environment : JS test framework with Intern

What did you expect to happen?

sinon.stub( { watch: function() {} } ) to stub the object and the watch method.

What actually happens

watch is not stubbed in FIREFOX.

How to reproduce

var stubbedObject = sinon.stub({ watch: function() {} });
stubbedObject.watch.args[1];

stubbedObject.watch does not have property 'args'

This only fails in Firefox. The problem is that when sinon.stub iterates over the functions of an object to stub them, it makes sure it doesn't stub the same function twice. It uses an object to keep track of all of the functions seen already. Object should have a prototype method called 'watch' (https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Object/watch) but right now, only Firefox implements this.

When .stub walks through the object's functions, and compares them to what it has 'seen' (in /lib/sinon/util/core/walk.js line 19) but the seen object has methods on it already.

Currently, the check if the function is seen is simply '!seen[k]'. I think it should be 'seen[k] !== true' seeing as that directly corresponds to the way the function records if an object is seen (with seen[k] = true). I have tested this and it works.

[mantoni]

The latest release of Sinon is 1.17.3. Can I ask you to update and check the latest version? This might have been fixed already.

[fatso83]

Ah, well spotted. I didn't notice. We should add that info to our issue templates

[ajdaniel]

According to this JSFiddle it looks to be working. Thank you for the response.

A colleague suggested I try updating to the latest version to see if it was fixed. I was foolish enough to assume that it wasn't, because the code was unchanged. Big slap on the wrist for me.

[ajdaniel]

Okay so I made another mistake. Here's an update.

We ARE using 1.17.3, I got our version number wrong.
The problem STILL exists.
You can see in this fiddle https://jsfiddle.net/Lmy3xtms/1/ when run in Firefox, that 'watch' is not being stubbed when using sinon.stub(myObject);
I am therefore reopening this issue, and refer to my fix suggestion once more

[mroderick]

@ajdaniel I have added https://github.com/sinonjs/sinon/pull/1030. Do you feel that test is an accurate test for this issue?

I could not wrap my head around getting the proposed solution to work. I think we'll need something akin to Set, which can be implemented with an Array, for keeping track of the seen values. Using an Object will always be susceptible to these kinds of errors where runtimes decorate Object.prototype with their own methods and properties, or code in user-land does.

[mantoni]

Hm, maybe we can fix it by passing Object.create(null) as the seen argument here. This creates an object that does not inherit from Object.prototype. Can't check myself right now, sry.

[mroderick]

We will need some way to do that for older JS runtimes also

[ajdaniel]

@mroderick that unit test looks accurate to me thank you.

What I thought would work best is replacing if (!seen[k]) here which is implicitly looking for a truthy value of the seen object key (which succeeds whether the value is 'true' or if it is simply a function) with if (seen[k] !== true) which explicitly looks for the value (true) that is assigned to the seen object key on the next line.

This is how we patch our version of Sinon to confirm the fix works. It may be over-simplified or have ramifications elsewhere, so I trust your judgement on this idea.

[mantoni]

@mroderick Yes - although Object.create is supported all the way back to IE 9. I don't think we should support anything older than that with Sinon 2.0.

Anyway, the approach @ajdaniel suggested seems fine to me and does not require Object.create, so that sounds like a better solution to me. :+1:

[mroderick]

I did try the proposed solution, but that breaks a unit test for walk. I was too tired to figure it out last night

[fatso83]

@ajdaniel, would you care to make a small PR with the change you proposed?

[ajdaniel]

Absolutely

[fatso83]

@ajdaniel : any progress? #1030 has a test for this.

[mroderick]

I have merged a fix for this into v1.17 branch. We will need to make a fix for master before this can be closed.

I propose that we cut a new 1.17.x release now. Do you want to do that @fatso83?

[mroderick]

This has now been closed. We can cut new 1.17.x and 2.0.0-pre-x releases. Will you do the honours @fatso83 (considering that I screwed it up with the pre version last time)

[fatso83]

Haha, well I screwed up the pre version the time before that, ref some old Twitter DMs 😊 Sure, I can cut a new releases tomorrow after merging the Maps matchers. Just need to have a look first.