Spy's thisValue not pointing to instance when spying constructor

[PVince81]

This snippet works fine with Sinon 2.0.0 but fails with Sinon 4.2.2:

it('returns correct thisValue', function() {
    window.SomeClass = function() {
        this.counter = 0;
        this.inc = function() {
            this.counter++;
        };
        this.getCounter = function() {
            return this.counter;
        };
    };

    var spy = sinon.spy(window, 'SomeClass');
    var some = new window.SomeClass();
    some.inc();

    expect(spy.thisValues[0].counter).toEqual(1);
    expect(spy.thisValues[0].getCounter()).toEqual(1);
    some.inc();
    expect(spy.thisValues[0].counter).toEqual(2);
    expect(spy.thisValues[0].getCounter()).toEqual(2);
});

It looks like the thisValues[0] object is a proxy and does not refer to the actual instance of some.

This is the error with Sinon 4.2.2:

Expected undefined to equal 1
TypeError: undefined is not a function (evaluating 'spy.thisValues[0].getCounter()')

[mroderick]

This is a good candidate for using git bisect to establish when the behaviour was changed, which might give some clue to whether or not the change was intentional (a feature) or not (a bug).

It shouldn't be that difficult to turn the example @PVince81 provided into a small test script that can be used for git bisect run.

If you don't know how to use git bisect, this is an excellent opportunity to level up!

http://www.marclittlemore.com/how-to-find-bugs-using-git-bisect-with-this-easy-guide/

[PVince81]

Oh, I love bisecting! I'll take care of this..

[PVince81]

Here we go: 911c498 Spy passes through calling with new (#1626)

[fatso83]

Thanks for doing that, @PVince81! A fix might want to also inspect #1265, as it touches upon which changes are needed to support constructor spying.

[rgroothuijsen]

When an object is not created with new, this method is called, which alters the contents of thisValue and by extension thisValues. In contrast, the method called when new is used leaves thisValue unaltered.

[PVince81]

considering that our code only has a single usage of this I'd be rather tempted to go the quick route and change our single test that triggered this as I don't have time to wrap my head around sinon's internals and don't feel qualified to make potentially breaking changes

since not that many people seems to have complained about this issue, maybe people usually don't test this way and a fix isn't worth it (just an observation, up to the maintainers to decide)

[mroderick]

Even if it is not commonly used, or at least not commonly reported, I'd like to have this regression fixed.

[ivan-zakharchuk]

Encountered the same issue, is there any plans on fixing this?

[fatso83]

@ivan-zakharchuk If no-one is volunteering, there are no plans, no. If you want it fixed, there's a natural next step that could be taken ;-)

This is the offending commit: #1683 (comment)

[gwpmad]

Just to say, I have spent a long time trying to write a fix for this, but have been unsuccessful. It's a nightmare of this values. The core problem seems to be that newing the provided function here inherently creates a brand new this, whereas thisValues[0] (as used in the original post) refers to the this created by the new in the test itself (i.e. the var some = new window.SomeClass();) - very confusing I know.

Essentially, fixing this means trying to have it both ways - calling new absolutely means a new this, but we have to call it twice (i.e. one in OP's test and one in proxy-invoke.js) to satisfy ES6. So you're always going to end up with two thises. The only thing I could think of was to bind the functions from one this on to the other, but that was too hacky even for this and broke a few other things.

I hope the above makes sense, please feel free to correct/request clarification. See here for a refresher on what the new keyword actually does - TLDR it creates a new object and makes it this, whatever the context.

Anyhow, one thing I did notice was that 911c498 (the commit that introduced the regression) does not include a test covering the issue it addresses - the requirement to use new with ES6 classes. This is probably because the linting options for the tests do not allow the class keyword as they only cater for ES5. I have a commit adding a test so that any future fix for the current issue does not un-fix the fix in 911c498 - I can PR it if there's interest.

[fatso83]

Six years since inception, I thought I would have a quick go... but man, is this a minefield of mutable state popping out its warts everywhere 😅 Changing a single line will easily trip up between 80 and 200 tests.

If no one else is able to touch this either I am inclined to just remove the prop altogether.