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: fix memory leak when using nock with jest #2572

Merged
merged 1 commit into from Jan 28, 2024

Conversation

VladimirChuprazov
Copy link
Contributor

Just importing nock in a Jest test is enough to have a memory leak.
Fix for #2358

@gr2m
Copy link
Member

gr2m commented Jan 28, 2024

Thank you for taking on this problem! Unfortunately your change seems to have broken the Node 10 and 12 tests (I know).

Maybe only run test:jest in CI when Node version is at least 14? We will bump to Node 20 as part of fixing #2397

@VladimirChuprazov
Copy link
Contributor Author

Hey @gr2m, just updated the job

@VladimirChuprazov
Copy link
Contributor Author

VladimirChuprazov commented Jan 28, 2024

Looks like need to fix the command for windows as well 😄

@VladimirChuprazov
Copy link
Contributor Author

@gr2m should be ok now, checked in my repo

Copy link
Member

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Thank you!

@gr2m gr2m merged commit 7468cf1 into nock:main Jan 28, 2024
19 checks passed
Copy link

🎉 This PR is included in version 13.5.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@perrin4869
Copy link

hm... the removal of

    const module = {
      http: require('http'),
      https: require('https'),
    }[moduleName]

broke my tests. Patching it back in fixed it. I didn't look into the root causes but obviously http and https need to be preloaded...

@VladimirChuprazov
Copy link
Contributor Author

@perrin4869 could you create a minimal reproduction repository?

@perrin4869
Copy link

perrin4869 commented Jan 31, 2024

well of course I'd like to setup a minimal reproduction and write a regression test to make sure it doesn't break again in the future ^^;;
I expect it wouldn't be easy to get a minimal reproduction, since the code that broke actually runs inside a lambda so I cannot easily reproduce it locally, I may try today or this week

@gr2m
Copy link
Member

gr2m commented Feb 1, 2024

it's most likely weird lambda code bundling ... I've been bitten by this before. I'd suggest we revert this particular change and add a comment with a warning for future contributors to ... just leave it alone 😁

@perrin4869
Copy link

@gr2m that's be super helpful! I'm sure it was coded that way for a reason 😅

@markmssd
Copy link

Thanks for the fix @VladimirChuprazov !

I'm curious whether https://www.npmjs.com/package/nock#memory-issues-with-jest is still relevant with this fix now, would you know?

@Uzlopak
Copy link
Member

Uzlopak commented Feb 25, 2024

@markmssd
How about you satisfy your curiosity and check if that is still relevant and report it back? ;)

@markmssd
Copy link

Still required :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants