Refactor stub methods #1239
Refactor stub methods #1239
Conversation
mroderick
force-pushed
the
refactor-stub-method
branch
from
be2d032 to
9d64dbf
Compare
mroderick
force-pushed
the
refactor-stub-method
branch
from
9d64dbf to
daa9ab4
Compare
|
|
||
| return stubbedObj; | ||
| if (isStubbingEntireObject) { | ||
| collectOwnMethods(stubbed).forEach(this.add.bind(this)); |
There was a problem hiding this comment.
Not that it matters here, but you know that you can pass a custom this to forEach as the second argument?
There was a problem hiding this comment.
I did, don't use it much, as I try to avoid using this as I find it reduces problems when I do :)
There was a problem hiding this comment.
Absolutely, but since you're using this here already ... 😆
|
This is quite a bit of cleanup! I like how you made things more readable by moving the complex decision making logic into separate modules. Looks all good to me. Regarding the API inconsistency you spotted: I remember running into this as well a while back, but I think the only reason it wasn't cleaned up was laziness. It makes sense to fold this into the stub implementation instead of a collection-only feature. |
My proposal was to remove it from |
|
Oh, I didn't read careful enough then. I now remember that it's only implemented for sandboxes because there wouldn't be a possibility to restore the previous value, because the |
| var walk = require("./util/core/walk"); | ||
| var getPropertyDescriptor = require("./util/core/get-property-descriptor"); | ||
| var valueToString = require("./util/core/value-to-string"); | ||
| var failOnFalsyObject = require("./fail-on-falsy-object"); |
There was a problem hiding this comment.
how about throwOnFalsyObject - as this tells you explicitly what this will do. fail could mean many different things.
There was a problem hiding this comment.
I agree, that is a better name. I've renamed the file, the function and all the variables in new commits.
Thank you for the feedback 👍
mroderick
force-pushed
the
refactor-stub-method
branch
from
daa9ab4 to
d9fb4af
Compare
mroderick
force-pushed
the
refactor-stub-method
branch
from
d9fb4af to
e9e5ba2
Compare
| var wrapper; | ||
|
|
||
| deprecated.printWarning( | ||
| "sinon.stub(obj, 'meth', fn) is deprecated and will be removed from" + |
There was a problem hiding this comment.
Hi, friends, I noticed that the third argument for the stub function is no longer going to be valid. While this seems to be okay when stubbing functions themselves I'd like to know how could we stub the get/set properties of descriptors.
Are we going to have a specific API for that or users should just use Object.getOwnPropertyDescriptor and then stub the get and set functions in there?
Also, if you want to take a look at how it works right now, #692 is the PR responsible for introducing it.
There was a problem hiding this comment.
Good call, although I am not sure.
There was a problem hiding this comment.
@lucasfcosta that's an interesting discussion. I am not sure it should block merging of this PR. If you agree, would you mind opening a separate issue for the discussion, so we can merge this?
There was a problem hiding this comment.
Yeah, I totally agree.
Let's discuss that in a separate issue, I've got some ideas for the API and how to implement it, I'd love to help with this. Also, this PR LG(reat)TM.
Awesome work, sir 😄
|
OK, since there are no objections, I am merging this |
This PR refactors
sinon.stub()andsandbox.stub()(internallycollection.stub()methods to improve legibility.Feedback on naming of files and functions as well as locations of these in the tree is most welcome.
The aim was to not make any changes to functionality, but make it easier to understand the
stubmethods and to highlight differences between them for future improvements.During refactoring this, I discovered an inconsistency in the API.
Proposal: fix inconsistency of stubbing non-function properties
As you can see in
collection.stuband its tests, it supports stubbing non-function properties, whichsinon.stubdoes not.The documentation for
sandbox.stubreads:In my opinion, stubs should be used for replacing methods and objects with fakes. Instead of stubbing out a property on an object for the duration of a test, the author should use a stub in the right place to return an entirely fake object that has the (fake) methods and properties that is needed for the test.
I propose that we remove stubbing of non-function properties feature from
collection.stub, so that it's behaviour matches that ofsinon.stub(since it would then defer everything tosinon.stub).I am happy to do that as part of this PR.
What do you think?