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

fix(jest-mock): improve user input validation and error messages of spyOn and replaceProperty methods #14087

Merged
merged 4 commits into from
Apr 20, 2023
Merged

fix(jest-mock): improve user input validation and error messages of spyOn and replaceProperty methods #14087

merged 4 commits into from
Apr 20, 2023

Conversation

mrazauskas
Copy link
Contributor

Fixes #14077
Closes #14082

Summary

Currently validation of user input and error messages differs between spyOn, spyOn for get/set accessors and replaceProperty methods. Some message are somewhat incorrect (see tests). This PR cleans up the validation logic and error messages.

Test plan

Unit tests added.

Comment on lines 1788 to +1789
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');
Copy link
Contributor Author

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?

Copy link
Member

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

Comment on lines 1791 to +1792
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');
Copy link
Contributor Author

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'}
Copy link
Contributor Author

@mrazauskas mrazauskas Apr 19, 2023

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.

throw new Error(
`Cannot use spyOn on a primitive value; ${this._typeOf(object)} given`,
);
}

if (!object) {
Copy link
Contributor Author

@mrazauskas mrazauskas Apr 19, 2023

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.

@mrazauskas mrazauskas changed the title fix(jest-mock): fix(jest-mock): improve user input validation and error messages of spyOn and replaceProperty methods fix(jest-mock): improve user input validation and error messages of spyOn and replaceProperty methods Apr 19, 2023
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet 👍

@SimenB SimenB merged commit 268afca into jestjs:main Apr 20, 2023
@mrazauskas mrazauskas deleted the mock-validation-and-messages branch April 20, 2023 08:16
DmitryMakhnev pushed a commit to DmitryMakhnev/jest that referenced this pull request May 5, 2023
@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 21, 2023
@SimenB
Copy link
Member

SimenB commented Jul 4, 2023

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.

[Bug]: jest-mock/spyOn property name check is "truthy" instead of "defined" or "existent"
3 participants