-
-
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: notifications should be fire&forget rather than having a timeout #9567
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Hmm, adding a test will actually fire a notification. Is that annoying? I pushed it for now |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
We can remove the test later if it turns out to be annoying |
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? |
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 #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...