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

Make sure function mocks match original arity #4170

Merged
merged 3 commits into from
Aug 2, 2017

Conversation

palmerj3
Copy link
Contributor

@palmerj3 palmerj3 commented Aug 1, 2017

Summary

As reported in #3997, Jest mocks currently do not have the same arity as their underlying functions. This PR attempts to fix that.

Test plan

Run unit tests locally.
Test in a local project using tests provided in issue #3997
Write unit test in jest-mock to ensure arity is set properly

@cpojer
Copy link
Member

cpojer commented Aug 1, 2017

Thanks for sending a PR, including tests. I think to get this merged it needs to be correct and support a variable length. After 10, we could code-gen and use "new Function", what do you think?

@codecov-io
Copy link

codecov-io commented Aug 1, 2017

Codecov Report

Merging #4170 into master will decrease coverage by 0.13%.
The diff coverage is 34.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4170      +/-   ##
==========================================
- Coverage   60.55%   60.42%   -0.14%     
==========================================
  Files         196      196              
  Lines        6776     6809      +33     
  Branches        6        6              
==========================================
+ Hits         4103     4114      +11     
- Misses       2670     2692      +22     
  Partials        3        3
Impacted Files Coverage Δ
packages/jest-mock/src/index.js 84.67% <34.28%> (-7.44%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9665de2...e0a26b2. Read the comment docs.

@palmerj3
Copy link
Contributor Author

palmerj3 commented Aug 1, 2017

@cpojer I did attempt this several times. I'm not sure it's possible.. also this is very similar to how sinon solves this issue: https://github.com/sinonjs/sinon/blob/master/lib/sinon/spy.js#L61

@cpojer
Copy link
Member

cpojer commented Aug 1, 2017

We could try-catch for this and fall-back to the solution that doesn't have proper arity. It means it'll work properly in most environments, and fall back to a working solution at least in an environment without eval.

@palmerj3
Copy link
Contributor Author

palmerj3 commented Aug 1, 2017

@cpojer perhaps the default case should be the same as the 0 case. This way if it's out of range, as you say, it would return a 0 length function like it does right now.

@palmerj3
Copy link
Contributor Author

palmerj3 commented Aug 2, 2017

@cpojer let me know if my current solution works for you or if I need to make other changes. I'm not sure a dynamic case will work... but if you could point me in the right direction I'm willing to try.

@cpojer cpojer merged commit 99d9cff into jestjs:master Aug 2, 2017
@cpojer
Copy link
Member

cpojer commented Aug 2, 2017

Works for me, thank you!

@palmerj3 palmerj3 deleted the mockArity branch August 2, 2017 16:10
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
* Make sure function mocks match original arity

* Mocks maintain function arity with 9 or less arguments

* Update index.js
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants