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

Don't include unref'd timers in --detectOpenHandles results #8941

Merged
merged 4 commits into from
Sep 13, 2019
Merged

Don't include unref'd timers in --detectOpenHandles results #8941

merged 4 commits into from
Sep 13, 2019

Conversation

pimterry
Copy link
Contributor

Fix #8939

Note that this shouldn't affect any behaviour in Node < 11, and because of that the test is skipped in older versions. It passes happily for me locally with Node 12.9.1 though.

It would be possible to extend this to cover old versions later on (by wrapping ref()/unref()) if that seems worthwhile for somebody.

@codecov-io
Copy link

codecov-io commented Sep 11, 2019

Codecov Report

Merging #8941 into master will decrease coverage by 0.05%.
The diff coverage is 6.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8941      +/-   ##
==========================================
- Coverage   64.23%   64.18%   -0.06%     
==========================================
  Files         276      276              
  Lines       11700    11712      +12     
  Branches     2864     2867       +3     
==========================================
+ Hits         7516     7517       +1     
- Misses       3552     3563      +11     
  Partials      632      632
Impacted Files Coverage Δ
packages/test-utils/src/ConditionalTest.ts 0% <0%> (ø) ⬆️
packages/jest-core/src/collectHandles.ts 2.08% <9.09%> (+2.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 736edd2...36b58c8. Read the comment docs.

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Awesome, thanks a lot @pimterry!
I've verified locally that the test fails with collectHandles.ts@master

e2e/__tests__/detectOpenHandles.ts Outdated Show resolved Hide resolved
@jeysal jeysal requested review from SimenB and thymikee September 11, 2019 14:46
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.

This is great, thanks for tackling it! I didn't know about hasRef

@pimterry
Copy link
Contributor Author

Now updated with some more general conditional testing logic.

I couldn't quite follow the existing pattern (.only seems to disable all the other tests in the file, regardless of describe), but I've added some conditional logic that gives the same effect, showing the tests in the output as explicitly skipped.

In Node 10:

 PASS  e2e/__tests__/detectOpenHandles.ts (5.171s)
  ✓ prints message about flag on slow tests (1779ms)
  ✓ prints message about flag on forceExit (750ms)
  ✓ prints out info about open handlers (706ms)
  ✓ does not report promises (726ms)
  ✓ prints out info about open handlers from inside tests (711ms)
  [SKIP] tests that require node >=11
    ○ skipped does not report timeouts using unref

Test Suites: 1 passed, 1 total
Tests:       1 skipped, 5 passed, 6 total
Snapshots:   4 passed, 4 total
Time:        5.201s

In Node 12 the [SKIP] wrapper doesn't appear, and it runs like any other test:

 PASS  e2e/__tests__/detectOpenHandles.ts (5.241s)
  ✓ prints message about flag on slow tests (1655ms)
  ✓ prints message about flag on forceExit (704ms)
  ✓ prints out info about open handlers (621ms)
  ✓ does not report promises (642ms)
  ✓ does not report timeouts using unref (638ms)
  ✓ prints out info about open handlers from inside tests (642ms)

Test Suites: 1 passed, 1 total
Tests:       6 passed, 6 total
Snapshots:   4 passed, 4 total
Time:        5.268s, estimated 6s

@pimterry pimterry requested review from SimenB and jeysal September 13, 2019 09:15
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.

Barring lint (prettier) errors reported by CI, this LGTM. Super awesome contribution, thank you!

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

Unref'd timeouts should not be reported as open handles
5 participants