-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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: mocking of getters/setters on automatically mocked classes #13145
Conversation
related issue #13140 |
Thanks for sending a PR @staplespeter! There are conflicts - would you be able to resolve them? I assume the conflicts come from #13247 |
Hi @SimenB, yes I'll update the pull request. |
# Conflicts: # packages/jest-mock/src/index.ts
@SimenB Pull request updated. I've made some assumptions about the type changes in #13247 and dynamically typecast Mocked to PropertyDescriptor to get things to work. TBH this is from a position of mostly ignorance, however the existing tests for my changes still pass. Any questions do please ask of course. |
Can spot this typecast ;D Could you point me to the line, please? Regarding the type changes from #13247. All should be good if running |
Co-authored-by: Tom Mrazauskas <[email protected]>
Apologies, I got dragged out the door for dinner and hadn't made the commit... The changes should be apparent now, in the latest commit. I have run |
Checked out the branch and looked through your change. I think all is fine with types. Looks right for my eye. |
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.
exciting stuff! I'd love to explore the __esModule
thing you mention. But if it proves breaking, I'm happy to land this as-is 🙂
I have updated the code comments as requested. The removal of the "object._esModule" check in getSlots() did not effect any test results. This case is handled later when assigning the metadata in "getMetadata()" |
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.
this is awesome work, thanks!
@staplespeter I've had to revert this as it broke mocking in a work project. import { Datastore } from '@google-cloud/datastore';
jest.mock('@google-cloud/datastore'); So it seems we're missing a test 🙂 Could you revisit? |
…es (jestjs#13145)" This reverts commit 82e503f.
@SimenB Yes of course, I'll take a look ASAP |
Great, thanks! No immediate rush 👍 |
@SimenB can you tell me how to recreate the problem you encountered? I didn't receive any exceptions when executing |
Just need to instantiate it import { Datastore } from '@google-cloud/datastore';
jest.mock('@google-cloud/datastore');
new Datastore(); |
…ed classes (jestjs#13145)"" This reverts commit 52d45be. # Conflicts: # CHANGELOG.md
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. |
Summary
Setting HEAD to commit bf4d2d6 and running the tests will show that mocking of getters/setters on automatically mocked classes did not work at all. This request fixes that problem whilst maintaining existing functionality. All (non-mercurial) tests currently pass.
This is my first pull request so I do not know if the request number is the same as the issue number and have thus not updated the Changelog.md file with this fix. Apologies for any inconvenience.
Fixes #13140
Test plan
Test coverage can be seen in the aforementioned commit.