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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: skip ELDHISTOGRAM for --detectOpenHandles #10417

Merged
merged 6 commits into from Aug 19, 2020

Conversation

0cl
Copy link
Contributor

@0cl 0cl commented Aug 17, 2020

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:
1

After the change, jest didn't hang:
2

I wasn't sure how or where to add tests for the PR - in jest/packages/jest-core/src/__tests__? apologies 馃槄...

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

@0cl 0cl marked this pull request as draft August 17, 2020 19:12
@0cl
Copy link
Contributor Author

0cl commented Aug 17, 2020

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?

@0cl 0cl marked this pull request as ready for review August 17, 2020 21:59
@SimenB
Copy link
Member

SimenB commented Aug 18, 2020

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 prom-client, but we can do what they do to create the handle: https://github.com/siimon/prom-client/blob/25f9c4059d8a0f2d02ccfec7acfb64157c8b72bf/lib/metrics/eventLoopLag.js#L45-L47

@0cl
Copy link
Contributor Author

0cl commented Aug 19, 2020

Thank you! I've updated it based on the understanding that monitorEventLoopDelay and Histogram were only added in Node v11.10, but Jest currently supports >= 10.13

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.

Thank you!

@SimenB SimenB merged commit 4a9ebb4 into jestjs:master Aug 19, 2020
@0cl
Copy link
Contributor Author

0cl commented Aug 19, 2020

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... 馃槵

@0cl 0cl deleted the skip-eldhistogram-handle branch August 19, 2020 22:06
@SimenB
Copy link
Member

SimenB commented Aug 20, 2020

No mistakes were made, this was a very solid contribution! Looking forward to future contributions 馃檪

I'll release this tomorrow 馃憤

@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 May 11, 2021
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.

ELDHISTOGRAM open handle
3 participants