Restore fully spied objects in the sandbox#2193
Conversation
Both spy.js and stub.js has these checks already
|
Coincidentally, the exact discussion about own vs prototype values was just revived (unrelated to this issue): #1557 (comment) |
The property descriptor is recursively retrieved up the
prototype chain. It can by thus often _not_ be owned by
the object. Therefore the previous error message was wrong.
We even have an explicit test for the sandbox.replace* methods
that states this:
it("should replace an inherited property", ...
This is in opposition to stubs and spies which refuse to replace
fields that are not owned by the object, which is somewhat strange ...
b55132a to
8979784
Compare
|
I agree that is is confusing and unfortunate. An improvement would be to clarify in the documentation when fakes, spies and stubs can/cannot replace prototype methods. That won't solve the inconsistency, but at least make it less confusing for people using the library. Since the discussion is here (or linked from here), should we do that as part of this pull request? |
|
I was about to say yes to that, but I see won't have time to update this PR with any meaningful amount of additional work. I say yes to creating an additional PR to deal with the texts. Of course, I would have like to settle the matter, but that is probably an even greater issue ... |
|
This has been published as |
Purpose (TL;DR) - mandatory
Additionally, this PR also fixes a misleading error message.
Background (Problem in detail) - optional
See #2192. It is currently not possible to use the sandbox restore functionality on objects that have been spied on like this
sinon.spy(obj).Solution
The only thing I want to point out as strange is the fixes to the error message. I found it somewhat strange that we allow fakes to replace stuff on the prototype, even though we have had a discussion about this earlier regarding spies and stubs where @mroderick came to the opposite conclusion: do not allow stubbing on fields up the prototype chain (hence the error messages). This makes using fakes a different beast from using spies and stubs, which I think is a bit unfortunate.
How to verify - mandatory
npm testpassChecklist for author
npm run lintpasses