-
-
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: skip ELDHISTOGRAM
for --detectOpenHandles
#10417
Conversation
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.
Thanks for sending a PR! This is missing a changelog entry and a test (as you mention in the OP). The test can bee added here: https://github.com/facebook/jest/blob/master/e2e/__tests__/detectOpenHandles.ts while fixtures are here: https://github.com/facebook/jest/tree/master/e2e/detect-open-handles
Thanks for pointing me in the right direction! I've added a test, but I'm not sure it's right - should a prom-client histogram object be created in the fixture? |
Not |
Thank you! I've updated it based on the understanding that |
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.
Thank you!
sorry I didn't see that you commented earlier! thanks for your patience, it was my first time ever doing a PR, apologies for the mistakes... 😬 |
No mistakes were made, this was a very solid contribution! Looking forward to future contributions 🙂 I'll release this tomorrow 👍 |
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
Fixes #10020 ?
I reproduced the bug and making the suggested change locally in
./node_modules/@jest/core/build/collectHandles.js
seemed to be fine.Test plan
Reproduced:
After the change, jest didn't hang:
I wasn't sure how or where to add tests for the PR - in
jest/packages/jest-core/src/__tests__
? apologies 😅...