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

Fix for 1512: sandbox.stub() fails on prototype props #1516

Merged

Conversation

fatso83
Copy link
Contributor

@fatso83 fatso83 commented Aug 7, 2017

Purpose (TL;DR) - mandatory

This PR restores the functionality in sandboxes that made it possible to stub methods that were higher on the prototype chain.

Background

The bug introduced in 74263dd happened as the check for a property only checked on the instance, not on the properties further up the prototype chain. That meant it was no longer possible to stub out methods on instances of a "class". See issue #1512 for background info.

Solution

Simply checks for existence of the (enumerable) property.

How to verify - mandatory

  1. Check out this branch
  2. npm install
  3. npm test (will run the testcase)
  4. git checkout HEAD~1 && npm test should fail

@fatso83
Copy link
Contributor Author

fatso83 commented Aug 7, 2017

The test only checks for enumerable properties though ... I can change it to use Object.hasOwnProperty, but it will need to recurse up the prototype chain to the top. Do we have such a utility? Wrong See below.

@lucasfcosta
Copy link
Member

Hi @fatso83, excellent work, as always.

We do not have that exact utility, but we have get-property-descriptor, which goes up the prototype chain until it finds a property descriptor, otherwise it returns undefined.

I think we could use that and check if the descriptor returned is defined or not.

@fatso83
Copy link
Contributor Author

fatso83 commented Aug 7, 2017

Ha, I was wrong: the original code was 100% correct, and I just learned how to read the EcmaScript specification 😄

Some exploratory tests using prop in object on non-enumerable objects made me confused, as it showed that it actually could tell if a non-enumerable property was present. I thought that it was the complement of the for ... in, but then I

  1. found the definition of how the in operator works, which lead me to
  2. [[HasOperator]], which again led me to
  3. [[GetProperty]], which essentially is exactly what our internal util does - a recursive lookup on the prototype to find the descriptor!

So, basically I'll just merge this as it is, as it does exactly what was intended (somewhat by chance) 🙈

@fatso83 fatso83 merged commit 75ad693 into sinonjs:master Aug 7, 2017
@fatso83 fatso83 deleted the 1512-sandbox-stub-props-on-prototype branch August 7, 2017 19:56
@mroderick
Copy link
Member

👍

@lucasfcosta
Copy link
Member

lucasfcosta commented Aug 8, 2017

@fatso83 awesome! I haven't noticed that before.

Btw, if you're interested in doing some more research into the MetaObject Protocol I highly recommend you this link. Which explains them and relates them to how Proxies work.

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

Successfully merging this pull request may close these issues.

3 participants