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

Stub get does not work with sandbox stub #1401

Closed
lukiano opened this issue May 10, 2017 · 6 comments · Fixed by #1416
Closed

Stub get does not work with sandbox stub #1401

lukiano opened this issue May 10, 2017 · 6 comments · Fixed by #1416
Assignees
Labels
Feature Request semver:major changes will cause a new major version

Comments

@lukiano
Copy link

lukiano commented May 10, 2017

stub().get() used to stub property getters, works well with sinon.stub, but I get get is not a function with sandbox.stub.

Also, slightly related, .get does not exist on the typescript definition @types/sinon

@tumbledwyer
Copy link

I have the same problem, wanted to switch to using the sandbox and expected stubs to work the same but there is no get function on the sandbox. Is there another method to accomplish the same functionality?

@tumbledwyer
Copy link

tumbledwyer commented May 15, 2017

@lukiano I did some digging through the code and you can stub out properties, just with a different syntax. Quite confusing and would be nice if this was consistent.
So with sinon:
sinon.stub(myObject, 'myProperty').get(() => 'stubbedValue')
and with sandbox:
sandbox.stub(myObject, 'myProperty', 'stubbedValue')

@fatso83
Copy link
Contributor

fatso83 commented May 15, 2017

@lucasfcosta, do you remember why we chose not to align these two APIs? There was something ... but the details elude me.

@lucasfcosta
Copy link
Member

Hi @fatso83, that's because we discussed that through DM's on Twitter.

I also explain why we can't do this on this post. Basically, this issue is a duplicate of #781.

Since we've had many changes in the codebase since then, these are my new thoughts on how we could solve this.

Since a sandbox extends a collection and a collection's stubs do not take a third value as argument anymore we will not have the same problem as before because now we cannot mistake a third argument that is an object for the new value of that property or a property descriptor, as I've described in the previously mentioned post.

This also means we can now have a uniform API and add get and set methods to the stubs created by sandbox.stub.

Also, by looking at the collection.stub method, I can't see why we are not reusing the stub function since we're basically duplicating lots of logic and thus making things more error prone in order to achieve the same goal. I think we could just use the stub function and add it to the collection itself.

If you all agree on this I can refactor sandbox.stub the way I described above and fix this.

@fatso83
Copy link
Contributor

fatso83 commented May 16, 2017

@lucasfcosta, I think that makes plenty of sense. If you have the time, then please!

@fatso83 fatso83 added Feature Request semver:major changes will cause a new major version labels May 16, 2017
@fatso83 fatso83 self-assigned this May 16, 2017
@lucasfcosta
Copy link
Member

@fatso83, for sure!

I'll love to work on this refactor and a few other related improvements while eating some sushi this weekend 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request semver:major changes will cause a new major version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants