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

Resubmit: Set wrappedMethod on getters/setters #2378

Merged
merged 2 commits into from May 25, 2021
Merged

Conversation

@fatso83
Copy link
Contributor

@fatso83 fatso83 commented May 25, 2021

This is a re-submit of #2251 100% made by @rgroothuijsen, rebased on master. See it for background and discussion. Includes a revert of #2270. The original PR body is below:


Purpose

This PR fixes #2198 by wrapping accessors if they are present, rather than the property they belong to.

Background

Because the current wrapMethod implementation only assumes the presence of a single method on a property, trying to set both a getter and a setter will cause the former to be overwritten by the latter, and not added as separate get and set methods. Furthermore, restore() is implemented on the property itself, which does not allow the spy.get.restore() and spy.set.restore() outlined in the documentation.

Solution

This problem was addressed by changing wrapMethod to handle multiple methods per property. This includes a check whether any particular method is an accessor or not, and a method-wrapping implementation is chosen based on this. If the method in question is not a "get" or "set" method, the existing implementation will be used.

In order to satisfy ESLint code quality requirements, some restructuring of wrapMethod has been performed as well. To the extent possible, however, the existing structure has been left alone.

  • restore() was moved because function declarations are not allowed inside a loop
  • Parts of the code have been separated into functions because the complexity of wrapMethod had become too high

How to verify

  1. Check out this branch
  2. npm install
  3. npm test (See included test for details)
@fatso83 fatso83 requested a review from mroderick May 25, 2021
@fatso83 fatso83 merged commit 0a92b86 into sinonjs:master May 25, 2021
3 checks passed
@fatso83 fatso83 deleted the SINON-2198 branch May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants