-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
fix(jest-mock): improve user input validation and error messages of spyOn and replaceProperty methods
#14087
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
Conversation
…spyOn` and `replaceProperty` methods
| moduleMocker.spyOn(null, 'method'); | ||
| }).toThrow('spyOn could not find an object to spy on for method'); | ||
| }).toThrow('Cannot use spyOn on a primitive value; null given'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null should be treated as primitive. Or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I think that's fine
| moduleMocker.spyOn({}, 'method'); | ||
| }).toThrow( | ||
| "Cannot spy on the method property because it is not a function; undefined given instead. If you are trying to mock a property, use `jest.replaceProperty(object, 'method', value)` instead.", | ||
| ); | ||
| }).toThrow('Property `method` does not exist in the provided object'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm.. It does not exist simply.
| ${Symbol()} | ${'symbol'} | ||
| ${true} | ${'boolean'} | ||
| ${false} | ${'boolean'} | ||
| ${() => {}} | ${'function'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A function can have properties. Removed this case and moved null and undefined here from the above test.
| ); | ||
| } | ||
|
|
||
| if (!object) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null is handled above. All what is possible here are objects and function. Looks like this check is not needed.
spyOn and replaceProperty methodsspyOn and replaceProperty methods
SimenB
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sweet 👍
…spyOn` and `replaceProperty` methods (jestjs#14087)
|
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. |
Fixes #14077
Closes #14082
Summary
Currently validation of user input and error messages differs between
spyOn,spyOnfor get/set accessors andreplacePropertymethods. Some message are somewhat incorrect (see tests). This PR cleans up the validation logic and error messages.Test plan
Unit tests added.