Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

docs(JestObjectAPI.md): incorrect specification to the method jest.replaceProperty #14388

Merged
merged 2 commits into from
Aug 16, 2023
Merged

Conversation

vinicinbgs
Copy link
Contributor

Summary

The issue is correlated with the documentation that propose the use of jest.replaceProperty to mock a method inside a class

Screenshot 2023-08-04 at 16 37 11

If you follow this recommendation your test will output:

Cannot replace the `sum` property because it is a function. Use `jest.spyOn(object, 'sum')` instead.

Test plan

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 4, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: vinicinbgs / name: Vinicius Morais Dutra (33a56cd, 0e2b780)

@netlify
Copy link

netlify bot commented Aug 4, 2023

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 0e2b780
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/64db781ddfcd4b0008dea69b
😎 Deploy Preview https://deploy-preview-14388--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mrazauskas
Copy link
Contributor

mrazauskas commented Aug 5, 2023

Not sure. That can be a bug in code as well. Hm.. Perhaps not (see this test).

@michal-kocarek Would you remember why jest.replaceProperty() throws if a function is being replaced? Is this simply because a method is not a property? Somehow it got overlooked that code and docs in #13496 were contradicting.

@@ -669,7 +669,7 @@ Creates a mock function similar to `jest.fn` but also tracks calls to `object[me

:::note

By default, `jest.spyOn` also calls the **spied** method. This is different behavior from most other test libraries. If you want to overwrite the original function, you can use `jest.spyOn(object, methodName).mockImplementation(() => customImplementation)` or `jest.replaceProperty(object, methodName, jest.fn(() => customImplementation));`
By default, `jest.spyOn` also calls the **spied** method. This is different behavior from most other test libraries. If you want to overwrite the original function, you can use `jest.spyOn(object, methodName).mockImplementation(() => customImplementation)`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is how it sounded before #13496

Suggested change
By default, `jest.spyOn` also calls the **spied** method. This is different behavior from most other test libraries. If you want to overwrite the original function, you can use `jest.spyOn(object, methodName).mockImplementation(() => customImplementation)`.
By default, `jest.spyOn` also calls the **spied** method. This is different behavior from most other test libraries. If you want to overwrite the original function, you can use `jest.spyOn(object, methodName).mockImplementation(() => customImplementation)` or `object[methodName] = jest.fn(() => customImplementation)`.

Copy link
Contributor Author

@vinicinbgs vinicinbgs Aug 6, 2023

Choose a reason for hiding this comment

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

This behavior still working on "jest": "^29.6.2":

object[methodName] = jest.fn(() => customImplementation)

Let me show how the things are working in my example:

class Math {
  total;

  constructor() {}

  sum(num1, num2) {
    return num1 + num2;
  }
}

example.test.js

math["sum"] = (num1, num2) => num1 * num2; // works well
jest.spyOn(math, "sum").mockImplementation((num1, num2) => num1 * num2); // works well
jest.replaceProperty(math, "total", 20); // works well
jest.replaceProperty(math, "sum", jest.fn((num1, num2) => num1 * num2)); // Cannot replace the `sum` property because it is a function. Use `jest.spyOn(object, 'sum')` instead.

@vinicinbgs
Copy link
Contributor Author

Not sure. That can be a bug in code as well. Hm.. Perhaps not (see this test).

@michal-kocarek Would you remember why jest.replaceProperty() throws if a function is being replaced? Is this simply because a method is not a property? Somehow it got overlooked that code and docs in #13496 were contradicting.

I think maybe we just have a mismatch in the docs 🤔

@mrazauskas
Copy link
Contributor

Yes, it would be enough to change the sentence as it was before #13496. Just like in the suggestion which I left. Could you do that, please?

@vinicinbgs
Copy link
Contributor Author

Yes, it would be enough to change the sentence as it was before #13496. Just like in the suggestion which I left. Could you do that, please?

Of course! Done ✅

@mrazauskas
Copy link
Contributor

Perfect! Could you make the same change in all applicable versions in the versioned_docs folder, please?

@vinicinbgs
Copy link
Contributor Author

Perfect! Could you make the same change in all applicable versions in the versioned_docs folder, please?

Yes! Done too ✅

@vinicinbgs
Copy link
Contributor Author

vinicinbgs commented Aug 8, 2023

We have a fail on step Node CI / Lint (pull_request) in CI pipeline. But not related with this PR

@mrazauskas
Copy link
Contributor

mrazauskas commented Aug 9, 2023

That’s right. The fail is not related, #14371 should fix that.

@vinicinbgs vinicinbgs changed the title docs(JestObectAPI.md): incorrect specification to the method jest.replaceProperty docs(JestObjectAPI.md): incorrect specification to the method jest.replaceProperty Aug 13, 2023
@vinicinbgs
Copy link
Contributor Author

I merged with main and passed in all steps on CI. Maybe good to merge?

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.

Thanks!

@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 Sep 16, 2023
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants