Detect jest custom promisify handlers and replace with separate mock function

[leftshift]

Summary

Fixes the issue addressed in #14511: Currently, when automocking functions that provide a custom implementation for node's util.promisify(), the type hints for working with the mock function are confusing and must be explicitly ignored if the return types of the promisified variant differ. More in-depth explanation and a minimum example for illustrating the problem in the mentioned issue.

With this fix, the mock function for the non-promisified and promisified variant are now independent. Mocking the implementation of one will not affect the other. Previously, the non-promisified variant would be put though the default util.promisify() logic.

The workaround for this was used in one of the jest-haste-map tests. Similarly, this workaround would have to be removed in a similar way in downstream test code, because this change breaks that workaround.

Test plan

The whole jest test suite runs successfully, so this change probably doesn't fundamentally break the jest mocking system.

The minimal reproduction example now behaves as I would expect. The type hints and actual functionality now match up, instead of silently timing out. https://github.com/leftshift/jest-promisify-mre

If this has any chance of getting merged, I would provide additional tests, probably based on the mre.

[linux-foundation-easycla[bot]]

• :x: - login: @leftshift / name: floy . The commit (305ad5d1c71157b1148b8d316a0126bac7c7b4cd, 2d65ad0272945a105f22b6fbe815d29fc220695f, f3f2a25dda8e90d0e455d361d0561349c172c320, a9289c69e651a71f67cc1684a4302cdec70f3bd0) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

[netlify[bot]]

✅ Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	a9289c69e651a71f67cc1684a4302cdec70f3bd0
🔍 Latest deploy log	https://app.netlify.com/sites/jestjs/deploys/6508246695d2cc000856a2bd
😎 Deploy Preview	https://deploy-preview-14538--jestjs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[leftshift]

Hrm, I had seen those Max Call Stack Size Exceeded errors before, but had hoped I fixed them in a9289c6. Unfortunately pretty hard to replicate, but I'll try and take another look

[mrazauskas]

If an existing test has to be tweak, sound like this is breaking. The behaviour is changing.

For my eye it looks strange to require to use promisify() in a test. I mean, currently the test does not know that promisify() is used in the implementation and for my understanding that looks right. Any custom Proxy based utility can be used by a user.

Someone will definitely want the old behaviour back. Reading the changed test, the behaviour makes sense. It is nor broken, neither is that a workaround.

This is nice idea, but I still see it as an add-on utility. Or it should be implemented behind a flag. Perhaps that sounds good? I mean, if an option could be passed to jest.mock this is fine.

[SimenB]

Breaking changes in at of itself should be fine, I'll start landing breaking changes this week. But the changes should be good changes, og course 😃

[leftshift]

I mean, currently the test does not know that promisify() is used in the implementation

It does know that, though. The interface of child_process.execFile is this: execFile(cmd, args, (error, stdout, stderr) => {})

Note how stout, stderr are separate arguments. That is not what the mockImplementation in the watchman test is actually doing though! It is specifically mocking the interface of the promisified variant of execFile by returning an object with the properties stdout, stderr instead. However, I agree this is not at all obvious from a quick look at the test.

Hence, I think splitting this out into two mock functions that can be separately used will make the test code a lot clearer in cases like this.

[leftshift]

The actual failure seems to happen somewhere in the mocking logic of jest-mock. My best guess is, under some circumstances it enters an infinite recursion somehow, which leads to the tests expecting stuff to be mocked to fail.

Interestingly, it passed in the Node CI / test-macos / Node v16.x on macos-latest (2/4) (pull_request), and I've not been able to consistently reproduce it locally so far. I'll try to add some more instrumentation and poke around over the next days

[leftshift]

I'll hereby release the changes proposed in this PR as public domain/CC0. If someone wants to continue work on this and debug the remaining issues, feel free.

After mostly receiving pushback and a lack of trying to understand the actual issue at hand, I've somewhat lost motivation to continue work on this.

[github-actions[bot]]

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[github-actions[bot]]

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

[github-actions[bot]]

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

[github-actions[bot]]

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.