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: notifications should be fire&forget rather than having a timeout #9567

Merged
merged 4 commits into from
Feb 12, 2020

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Feb 12, 2020

Summary

Fixes #8036

Test plan

Verified locally. Due to #9454 I didn't wanna add a test. Writing that justification now though, makes me wanna just skip the test on windows, though...

@drew-gross
Copy link

Codecov Report

Merging #9567 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9567      +/-   ##
==========================================
+ Coverage   65.05%   65.07%   +0.01%     
==========================================
  Files         286      286              
  Lines       12128    12128              
  Branches     3005     3005              
==========================================
+ Hits         7890     7892       +2     
+ Misses       3600     3599       -1     
+ Partials      638      637       -1     
Impacted Files Coverage Δ
packages/expect/src/utils.ts 96.22% <0.00%> (+1.25%) ⬆️

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 2793c67...016c873. Read the comment docs.

@SimenB
Copy link
Member Author

SimenB commented Feb 12, 2020

Hmm, adding a test will actually fire a notification. Is that annoying? I pushed it for now

@codecov-io
Copy link

Codecov Report

Merging #9567 into master will decrease coverage by <.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9567      +/-   ##
==========================================
- Coverage   65.05%   65.05%   -0.01%     
==========================================
  Files         286      286              
  Lines       12128    12127       -1     
  Branches     3005     3005              
==========================================
- Hits         7890     7889       -1     
  Misses       3600     3600              
  Partials      638      638
Impacted Files Coverage Δ
packages/jest-reporters/src/notify_reporter.ts 74.5% <66.66%> (-0.5%) ⬇️

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 2793c67...85aa69f. Read the comment docs.

@SimenB
Copy link
Member Author

SimenB commented Feb 12, 2020

We can remove the test later if it turns out to be annoying

@SimenB SimenB merged commit 00e9e09 into jestjs:master Feb 12, 2020
@SimenB SimenB deleted the notifier-timeout branch February 12, 2020 12:13
@shirshak55
Copy link

Thanks man this issue had wasted my whole day lol. And I was like if it was redis/ typeorm or something else and i was sure there should not be any stuff. And is there anyway to know how to debug such issue if we face it in future?

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

Passing a notify flag complains that jest doesn't exit after the test run has completed
6 participants