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

Sandbox throws error 'cannot stub non-existent own property' #1537

Closed
ZebraFlesh opened this Issue Aug 18, 2017 · 13 comments

Comments

Projects
None yet
5 participants
@ZebraFlesh

ZebraFlesh commented Aug 18, 2017

I just tried upgrading from 2.4.1 to 3.2.1 and encountered the following issue. This code works in 2.4.1:

        const spy = sandbox.spy();
        sandbox.stub(window, 'google').value({
            maps: {
                LatLng: x => x,
                Map: spy
            }
        });

But in 3.2.1 it throws an exception: TypeError: Cannot stub non-existent own property google

It's not mentioned in the migration guide so it appears to be a regression.

@fatso83

This comment has been minimized.

Show comment
Hide comment
@fatso83

fatso83 Aug 18, 2017

Contributor

Related to #1512.

Contributor

fatso83 commented Aug 18, 2017

Related to #1512.

@fatso83

This comment has been minimized.

Show comment
Hide comment
@fatso83

fatso83 Aug 18, 2017

Contributor

If the property doesn't exist you don't need to add it to the sandbox. Simply overwrite it. But yeah, if it worked before, and we haven't explicitly said it should change, then it's a regression.

Not sure what we should do here. Update the docs to say stubbing non-existing values doesn't make sense and is unsupported, or make it possible?

Contributor

fatso83 commented Aug 18, 2017

If the property doesn't exist you don't need to add it to the sandbox. Simply overwrite it. But yeah, if it worked before, and we haven't explicitly said it should change, then it's a regression.

Not sure what we should do here. Update the docs to say stubbing non-existing values doesn't make sense and is unsupported, or make it possible?

fatso83 referenced this issue Aug 18, 2017

Remove deprecated
* sandbox.stub(obj, 'meth', val)
* sinon.stub(obj, 'meth', fn)
* sinon/util/core
@ZebraFlesh

This comment has been minimized.

Show comment
Hide comment
@ZebraFlesh

ZebraFlesh Aug 18, 2017

If the property doesn't exist you don't need to add it to the sandbox. Simply overwrite it.

The nice thing about adding the property to the sandbox is that sinon then helps me keep my global test environment clean between each test via sandbox.restore(). It is an extremely useful feature, especially when dealing with 3rd party libraries like Google Maps where I don't control the API. It would be great if it could be made to work in the 3.x line.

Also I just noticed that I committed the sin of not providing a complete example. My sandbox is being created with 2.4.1 format:

let sandbox;

before(() => { sandbox = sinon.sandbox.create(); })
afterEach(() => { sandbox.restore(); })

Not sure if that's important; apologies for not providing it sooner.

ZebraFlesh commented Aug 18, 2017

If the property doesn't exist you don't need to add it to the sandbox. Simply overwrite it.

The nice thing about adding the property to the sandbox is that sinon then helps me keep my global test environment clean between each test via sandbox.restore(). It is an extremely useful feature, especially when dealing with 3rd party libraries like Google Maps where I don't control the API. It would be great if it could be made to work in the 3.x line.

Also I just noticed that I committed the sin of not providing a complete example. My sandbox is being created with 2.4.1 format:

let sandbox;

before(() => { sandbox = sinon.sandbox.create(); })
afterEach(() => { sandbox.restore(); })

Not sure if that's important; apologies for not providing it sooner.

@mroderick

This comment has been minimized.

Show comment
Hide comment
@mroderick

mroderick Aug 22, 2017

Member

I think that in the scenarios like the one @ZebraFlesh describes, I would prefer to have the text fixture be more explicit.

// not so explicit, doesn't work with sinon@3.x
beforeEach(function() {
    const spy = sandbox.spy();
    sandbox.stub(window, 'google').value({
        maps: {
            LatLng: x => x,
            Map: spy
        }
    }); 
});
// more explicit, works with sinon@2, sinon@3
function setGoogleMapsFixture(sandbox) {
    window.google = {
        maps: {
            LatLng: x => x,
            Map: sandbox.spy()
        }
    };
}

function removeGoogleMapsFixture() {
    delete window.google;
}

beforeEach(function() {
    setGoogleMapsFixture(sandbox)
});

// not using afterEach, as this only needs to happen
// after the last test in this block is run
after(function() {
    removeGoogleMapsFixture();
});

With more explicit setting of the fixture like outlined above, you don't need a feature in Sinon that would allow stubbing of non-existing own properties.

Not sure what we should do here. Update the docs to say stubbing non-existing values doesn't make sense and is unsupported, or make it possible?

While I recognise that it might be convenient in some scenarios (like the one described by @ZebraFlesh), I think that stubbing non-existing own properties is likely to lead to mistakes in tests, where the test passes because the author mistyped the name of the existing property they were aiming to stub. We should aim to eliminate the possibility of mistakes where we can without being too restrictive.

I think stubbing non-existing own properties should remain unsupported. We should update the documentation.

Member

mroderick commented Aug 22, 2017

I think that in the scenarios like the one @ZebraFlesh describes, I would prefer to have the text fixture be more explicit.

// not so explicit, doesn't work with sinon@3.x
beforeEach(function() {
    const spy = sandbox.spy();
    sandbox.stub(window, 'google').value({
        maps: {
            LatLng: x => x,
            Map: spy
        }
    }); 
});
// more explicit, works with sinon@2, sinon@3
function setGoogleMapsFixture(sandbox) {
    window.google = {
        maps: {
            LatLng: x => x,
            Map: sandbox.spy()
        }
    };
}

function removeGoogleMapsFixture() {
    delete window.google;
}

beforeEach(function() {
    setGoogleMapsFixture(sandbox)
});

// not using afterEach, as this only needs to happen
// after the last test in this block is run
after(function() {
    removeGoogleMapsFixture();
});

With more explicit setting of the fixture like outlined above, you don't need a feature in Sinon that would allow stubbing of non-existing own properties.

Not sure what we should do here. Update the docs to say stubbing non-existing values doesn't make sense and is unsupported, or make it possible?

While I recognise that it might be convenient in some scenarios (like the one described by @ZebraFlesh), I think that stubbing non-existing own properties is likely to lead to mistakes in tests, where the test passes because the author mistyped the name of the existing property they were aiming to stub. We should aim to eliminate the possibility of mistakes where we can without being too restrictive.

I think stubbing non-existing own properties should remain unsupported. We should update the documentation.

@fatso83

This comment has been minimized.

Show comment
Hide comment
@fatso83

fatso83 Aug 22, 2017

Contributor

@mroderick I concur with you in that it might make for less bugs, but we already support this for normal stubs. If we should stop supporting this behavior, we need to remove it there as well, to be consistent. It would be strange to only support this feature outside of sandboxes, as sandboxes usually add some possibilities. And removing the support is a breaking feature, so a major bump would be required as well.

So either:

  • remove the check promptly for sandboxes to fix this breaking feature

or/and(?)

  • remove the functionality for both normal and sandboxed stubs
  • release new major version with updated docs
Contributor

fatso83 commented Aug 22, 2017

@mroderick I concur with you in that it might make for less bugs, but we already support this for normal stubs. If we should stop supporting this behavior, we need to remove it there as well, to be consistent. It would be strange to only support this feature outside of sandboxes, as sandboxes usually add some possibilities. And removing the support is a breaking feature, so a major bump would be required as well.

So either:

  • remove the check promptly for sandboxes to fix this breaking feature

or/and(?)

  • remove the functionality for both normal and sandboxed stubs
  • release new major version with updated docs
@ZebraFlesh

This comment has been minimized.

Show comment
Hide comment
@ZebraFlesh

ZebraFlesh Aug 22, 2017

With more explicit setting of the fixture like outlined above, you don't need a feature in Sinon that would allow stubbing of non-existing own properties.

I think that works well if your fixture never varies among your tests. However, my fixture does. A simple example is covering both success and failure cases:

it('handles the success case', () => {
        const spy = sandbox.spy();
        sandbox.stub(window, 'google').value({
            maps: {
                LatLng: x => x,
                Map: spy
            }
        });
        // ... test, including asserting that the spy was called
});

it('handles the failure case', () => {
        const msg = 'test error';
        sandbox.stub(window, 'google').value({
            maps: {
                LatLng: x => x,
                Map: sandbox.stub().throws(new Error(msg))
            }
        });
        // ... test, ignoring spy calls and instead focusing on error handling
});

The behavior in 2.x has the advantage of everything being properly cleaned up after each test via sandbox.restore(). Using the more explicit fixture setup example outlined above, I suppose you could delete the non-own property in an afterEach hook to achieve the same effect.

To solve the problem of introducing potential mistakes by inadvertently typing the name of an existing own property, sinon could modify the public API:

  • stub.ownValue(): stubs only own properties, throws for non-own properties
  • stub.value(): stubs only non-own properties, throws for own properties

The API becomes more explicit and the consumer is forced to choose the appropriate tool for the task at hand.

ZebraFlesh commented Aug 22, 2017

With more explicit setting of the fixture like outlined above, you don't need a feature in Sinon that would allow stubbing of non-existing own properties.

I think that works well if your fixture never varies among your tests. However, my fixture does. A simple example is covering both success and failure cases:

it('handles the success case', () => {
        const spy = sandbox.spy();
        sandbox.stub(window, 'google').value({
            maps: {
                LatLng: x => x,
                Map: spy
            }
        });
        // ... test, including asserting that the spy was called
});

it('handles the failure case', () => {
        const msg = 'test error';
        sandbox.stub(window, 'google').value({
            maps: {
                LatLng: x => x,
                Map: sandbox.stub().throws(new Error(msg))
            }
        });
        // ... test, ignoring spy calls and instead focusing on error handling
});

The behavior in 2.x has the advantage of everything being properly cleaned up after each test via sandbox.restore(). Using the more explicit fixture setup example outlined above, I suppose you could delete the non-own property in an afterEach hook to achieve the same effect.

To solve the problem of introducing potential mistakes by inadvertently typing the name of an existing own property, sinon could modify the public API:

  • stub.ownValue(): stubs only own properties, throws for non-own properties
  • stub.value(): stubs only non-own properties, throws for own properties

The API becomes more explicit and the consumer is forced to choose the appropriate tool for the task at hand.

@fatso83 fatso83 changed the title from Regression: stubbing window property to Sandbox throws error 'cannot stub non-existent own property' Aug 23, 2017

@fatso83

This comment has been minimized.

Show comment
Hide comment
@fatso83

fatso83 Aug 23, 2017

Contributor

This is very much related to the discussion in #1508 (although it deals with normal stubs)h, where @lucasfcosta has the opposing view - that we should not throw for undefined properties. Whatever we land on, I strongly believe we need to be consistent in the stubbing APIs for normal stubs and sandboxes. We should not support it in one case, and not the other.

Right now, the situation is:

  • normal stubs used to throw in 1.x, but this changed in 2.0 and does not throw now anymore
  • sandboxes did not use to throw, but started throwing in 3.1(?)

So for a while we had feature parity, but then we lost it again ... I don't think this zig-zagging is very beneficial for the users, so we should land this discussion. While I do agree with Morgan in that it might make for more specific tests, I don't like dropping a behavior for two major releases, then re-adding it again. I think it would make the least noise (fixes for clients, questions/issues on this tracker) just to revert this regression.

Contributor

fatso83 commented Aug 23, 2017

This is very much related to the discussion in #1508 (although it deals with normal stubs)h, where @lucasfcosta has the opposing view - that we should not throw for undefined properties. Whatever we land on, I strongly believe we need to be consistent in the stubbing APIs for normal stubs and sandboxes. We should not support it in one case, and not the other.

Right now, the situation is:

  • normal stubs used to throw in 1.x, but this changed in 2.0 and does not throw now anymore
  • sandboxes did not use to throw, but started throwing in 3.1(?)

So for a while we had feature parity, but then we lost it again ... I don't think this zig-zagging is very beneficial for the users, so we should land this discussion. While I do agree with Morgan in that it might make for more specific tests, I don't like dropping a behavior for two major releases, then re-adding it again. I think it would make the least noise (fixes for clients, questions/issues on this tracker) just to revert this regression.

@fearphage

This comment has been minimized.

Show comment
Hide comment
@fearphage

fearphage Aug 28, 2017

Member

While I understand the inconvenience, it seems like there's an easy workaround with minimal code changes.

before(function() {
  window.google = 'This is a placeholder for sinon to overwrite.';
});

after(function() {
  delete window.google;
});

This allows the sinon code to stay unchanged.

Regression vs poor documentation

This appears to be expected behavior since there are tests for it. We should update the docs to reflect that in my opinion. It came in a major so breaking changes are tolerated.

Member

fearphage commented Aug 28, 2017

While I understand the inconvenience, it seems like there's an easy workaround with minimal code changes.

before(function() {
  window.google = 'This is a placeholder for sinon to overwrite.';
});

after(function() {
  delete window.google;
});

This allows the sinon code to stay unchanged.

Regression vs poor documentation

This appears to be expected behavior since there are tests for it. We should update the docs to reflect that in my opinion. It came in a major so breaking changes are tolerated.

@fatso83

This comment has been minimized.

Show comment
Hide comment
@fatso83

fatso83 Aug 28, 2017

Contributor

@fearphage Keeping the status quo means stubbing non-existing fields is unsupported behavior for sandboxes, while it is supported behavior for normal stubs. Isn't a bit unfortunate that the two feature sets don't align?

Contributor

fatso83 commented Aug 28, 2017

@fearphage Keeping the status quo means stubbing non-existing fields is unsupported behavior for sandboxes, while it is supported behavior for normal stubs. Isn't a bit unfortunate that the two feature sets don't align?

fatso83 added a commit to fatso83/sinon that referenced this issue Sep 12, 2017

Remove support for stubbing undefined props
This was a longer discussion that can be found in the
issues #1508, #1552 and #1537 - and internally amongst the core team.

Support for stubbing props using get()/set() was added in #1237
and also worked for undefined properties.

This type of behavior (stubbing undefined props) has been
explicitly disabled for sandboxes, so in order for Sinon to
behave in a consistent way, we should do the same here.

This change will bring normal stubs back to how it has
used to be historically, and will make sandbox stubs
and normal sinon stubs behave the same.

It might break things for people relying on the behavior
that has been present since Sinon 2.0, but it should
make things more reliable going forward.

fatso83 added a commit to fatso83/sinon that referenced this issue Sep 13, 2017

Remove support for stubbing undefined props
This was a longer discussion that can be found in the
issues #1508, #1552 and #1537 - and internally amongst the core team.

Support for stubbing props using get()/set() was added in #1237
and also worked for undefined properties.

This type of behavior (stubbing undefined props) has been
explicitly disabled for sandboxes, so in order for Sinon to
behave in a consistent way, we should do the same here.

This change will bring normal stubs back to how it has
used to be historically, and will make sandbox stubs
and normal sinon stubs behave the same.

It might break things for people relying on the behavior
that has been present since Sinon 2.0, but it should
make things more reliable going forward.

fatso83 added a commit that referenced this issue Sep 18, 2017

Remove support for stubbing undefined props (#1557)
This was a longer discussion that can be found in the
issues #1508, #1552 and #1537 - and internally amongst the core team.

Support for stubbing props using get()/set() was added in #1237
and also worked for undefined properties.

This type of behavior (stubbing undefined props) has been
explicitly disabled for sandboxes, so in order for Sinon to
behave in a consistent way, we should do the same here.

This change will bring normal stubs back to how it has
used to be historically, and will make sandbox stubs
and normal sinon stubs behave the same.

It might break things for people relying on the behavior
that has been present since Sinon 2.0, but it should
make things more reliable going forward.

@fatso83 fatso83 referenced this issue Sep 18, 2017

Merged

Patch docs #1565

@mroderick

This comment has been minimized.

Show comment
Hide comment
@mroderick

mroderick Oct 4, 2017

Member

Resolution was implemented in #1557

Member

mroderick commented Oct 4, 2017

Resolution was implemented in #1557

@mroderick mroderick closed this Oct 4, 2017

@franklin-ross

This comment has been minimized.

Show comment
Hide comment
@franklin-ross

franklin-ross Jan 3, 2018

I've read the various threads and I can see why this happened, but it's a real pain in Typescript where you've often got functions that are implemented on a class prototype, in which case sinon spits the dummy even though everything looks fine type wise (since keyof YourType will happily allow all public functions which are defined further down the prototype chain).

I get that Typescript probably isn't a priority for you guys, but even in JS it seems counter intuitive that myObject.callMe() will execute perfectly happily, while sinon.stub(myObject, "callMe") won't in that case. I'd prefer not to have to go and investigate how that particular object was put together just so I know how to stub it.

I really think this is an important use case to make a happy path for, considering classes are getting more native support in JS.

franklin-ross commented Jan 3, 2018

I've read the various threads and I can see why this happened, but it's a real pain in Typescript where you've often got functions that are implemented on a class prototype, in which case sinon spits the dummy even though everything looks fine type wise (since keyof YourType will happily allow all public functions which are defined further down the prototype chain).

I get that Typescript probably isn't a priority for you guys, but even in JS it seems counter intuitive that myObject.callMe() will execute perfectly happily, while sinon.stub(myObject, "callMe") won't in that case. I'd prefer not to have to go and investigate how that particular object was put together just so I know how to stub it.

I really think this is an important use case to make a happy path for, considering classes are getting more native support in JS.

@fatso83

This comment has been minimized.

Show comment
Hide comment
@fatso83

fatso83 Jan 5, 2018

Contributor

If you get an error saying that the method is undefined on the object, you know the error is probably up the prototype. Then directly modifing the object using myObject.callMe = sinon.stub(); doesn't seem like that much of a hassle IMHO ... Should also save you from creating cleanup/teardown functions, as the prototype was never changed.

Contributor

fatso83 commented Jan 5, 2018

If you get an error saying that the method is undefined on the object, you know the error is probably up the prototype. Then directly modifing the object using myObject.callMe = sinon.stub(); doesn't seem like that much of a hassle IMHO ... Should also save you from creating cleanup/teardown functions, as the prototype was never changed.

@franklin-ross

This comment has been minimized.

Show comment
Hide comment
@franklin-ross

franklin-ross Jan 11, 2018

Yeah, I guess it's not overly difficult to work around, it's just putting more cognitive load on me to be aware of how things are implemented.

It also just seems unexpected, so that I felt the need to add a comment in the test to explain why the stubbing code on 2 consecutive lines for the same object was different, and why I was manually deleting one of the stubs in my teardown, but the other was handled by the sandbox.

franklin-ross commented Jan 11, 2018

Yeah, I guess it's not overly difficult to work around, it's just putting more cognitive load on me to be aware of how things are implemented.

It also just seems unexpected, so that I felt the need to add a comment in the test to explain why the stubbing code on 2 consecutive lines for the same object was different, and why I was manually deleting one of the stubs in my teardown, but the other was handled by the sandbox.

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