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

Spy's thisValue not pointing to instance when spying constructor #1683

Open
PVince81 opened this issue Feb 9, 2018 · 11 comments
Open

Spy's thisValue not pointing to instance when spying constructor #1683

PVince81 opened this issue Feb 9, 2018 · 11 comments

Comments

@PVince81
Copy link

PVince81 commented Feb 9, 2018

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
Copy link
Member

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
Copy link
Author

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

@PVince81
Copy link
Author

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

@fatso83
Copy link
Contributor

fatso83 commented Feb 12, 2018

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
Copy link
Contributor

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
Copy link
Author

PVince81 commented Aug 8, 2018

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
Copy link
Member

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

@ivan-zakharchuk
Copy link

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

@fatso83
Copy link
Contributor

fatso83 commented Aug 30, 2019

@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
Copy link

gwpmad commented Apr 12, 2020

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
Copy link
Contributor

fatso83 commented May 23, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants