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: mocking of getters/setters on automatically mocked classes #13145

Merged
merged 12 commits into from
Sep 28, 2022

Conversation

staplespeter
Copy link
Contributor

@staplespeter staplespeter commented Aug 17, 2022

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.

@staplespeter
Copy link
Contributor Author

related issue #13140

@staplespeter staplespeter changed the title Bug 13140 Bug 13140 - Mocking of getters/setters on automatically mocked classes does not work Aug 22, 2022
@SimenB
Copy link
Member

SimenB commented Sep 15, 2022

Thanks for sending a PR @staplespeter! There are conflicts - would you be able to resolve them? I assume the conflicts come from #13247

@staplespeter
Copy link
Contributor Author

Hi @SimenB, yes I'll update the pull request.

# Conflicts:
#	packages/jest-mock/src/index.ts
@staplespeter
Copy link
Contributor Author

@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.

@mrazauskas
Copy link
Contributor

I've made some assumptions about the type changes in #13247 and dynamically typecast Mocked to PropertyDescriptor to get things to work.

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 yarn build and yarn test-types pass without errors. Note that type tests run agains the build, so remember to rebuild each time you make changes.

@staplespeter
Copy link
Contributor Author

I've made some assumptions about the type changes in #13247 and dynamically typecast Mocked to PropertyDescriptor to get things to work.

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 yarn build and yarn test-types pass without errors. Note that type tests run agains the build, so remember to rebuild each time you make changes.

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 yarn build, yarn jest and yarn test-types, with passes. Mercurial tests were skipped.

@mrazauskas
Copy link
Contributor

Checked out the branch and looked through your change. I think all is fine with types. Looks right for my eye.

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.

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 🙂

packages/jest-mock/src/__tests__/index.test.ts Outdated Show resolved Hide resolved
packages/jest-mock/src/index.ts Outdated Show resolved Hide resolved
packages/jest-mock/src/index.ts Outdated Show resolved Hide resolved
packages/jest-mock/src/index.ts Outdated Show resolved Hide resolved
packages/jest-mock/src/index.ts Outdated Show resolved Hide resolved
@staplespeter
Copy link
Contributor Author

staplespeter commented Sep 20, 2022

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()"

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.

this is awesome work, thanks!

packages/jest-mock/src/index.ts Outdated Show resolved Hide resolved
packages/jest-mock/src/index.ts Outdated Show resolved Hide resolved
packages/jest-mock/src/index.ts Outdated Show resolved Hide resolved
@SimenB SimenB changed the title Bug 13140 - Mocking of getters/setters on automatically mocked classes does not work fix: mocking of getters/setters on automatically mocked classes Sep 28, 2022
@SimenB SimenB merged commit 82e503f into jestjs:main Sep 28, 2022
@SimenB
Copy link
Member

SimenB commented Sep 28, 2022

@SimenB
Copy link
Member

SimenB commented Sep 28, 2022

@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?

SimenB added a commit to SimenB/jest that referenced this pull request Sep 28, 2022
@staplespeter
Copy link
Contributor Author

@SimenB Yes of course, I'll take a look ASAP

@SimenB
Copy link
Member

SimenB commented Sep 28, 2022

Great, thanks! No immediate rush 👍

@staplespeter
Copy link
Contributor Author

@SimenB can you tell me how to recreate the problem you encountered? I didn't receive any exceptions when executing jest.mock('@google-cloud/datastore');. Apologies if i have missed something obvious

@SimenB
Copy link
Member

SimenB commented Oct 2, 2022

Just need to instantiate it

import { Datastore } from '@google-cloud/datastore';

jest.mock('@google-cloud/datastore');

new Datastore();

@staplespeter
Copy link
Contributor Author

@SimenB I have added a fix and a test, ran the local build and tests, and created a new pull request - #13391

staplespeter added a commit to staplespeter/jest that referenced this pull request Oct 7, 2022
…ed classes (jestjs#13145)""

This reverts commit 52d45be.

# Conflicts:
#	CHANGELOG.md
@github-actions
Copy link

github-actions bot commented Nov 5, 2022

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 Nov 5, 2022
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]: Mocking of getters/setters on automatically mocked classes does not work
4 participants