Retain spy function names and fix spy.named(name) #1987
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 2811
💛 - Coveralls |
mroderick
reviewed
Mar 1, 2019
| @@ -152,6 +152,12 @@ function createProxy(func, proxyLength) { | |||
| return p.invoke(func, this, slice(arguments)); | |||
| }; | |||
| } | |||
| var nameDescriptor = Object.getOwnPropertyDescriptor(func, "name"); | |||
There was a problem hiding this comment.
The code in this change looks very similar to the code in the next change. Would it be wise to extract it to a shared function? Perhaps even put that function in @sinonjs/commons?
There was a problem hiding this comment.
Yes, I had the same thought, but in one case we're just using one descriptor as is, in the other case we modfy the value. Extracting a function produces more code and would include a superfluous value assignment for one of the cases. So I decided against it for clarity.
I am treating commons as a toolbelt for shared functions. This is a Sinon only feature so far. We can move it if we want to reuse it elsewhere.
Closed
|
Released in |
franck-romano
pushed a commit
to franck-romano/sinon
that referenced
this pull request
Oct 1, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose (TL;DR) - mandatory
nameas the function they wrapnameproperty inspy.named(name), not just thedisplayName(used for sinon error messages)Fixes #250
Background (Problem in detail)
We used to retain the name of spied functions, but this has been lost along the way at some point.
There is #950 which shows in the issue description that the
nameproperty of a spy reflected the original function name. However, the test that is verifying this was added later and it hard coded the name to"proxy".I think this dates back to the old times when we used
evalto generate the spy function.Solution - optional
This change verifies that a
namecan be configured on the function – unfortunately this is inconsistent across (old but supported) browser versions – and then configures the name using the same property descritor flags as the original value.How to verify - mandatory
npm installnpm testnpm run test-cloudChecklist for author
npm run lintpasses