-
Notifications
You must be signed in to change notification settings - Fork 6.3k
feat: add tests for node/heart.ts #5122
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
Conversation
✨ code-server docs for PR #5122 is ready! It will be updated on every commit.
- Host: https://coder.com/docs/code-server/ea86d4c
- Last deploy status: success
- Commit: 7b11efb
- Workflow status: https://github.com/coder/code-server/actions/runs/2227964356
Codecov Report
@@ Coverage Diff @@ ## main #5122 +/- ## ========================================== + Coverage 71.33% 71.73% +0.39% ========================================== Files 30 30 Lines 1685 1684 -1 Branches 374 374 ========================================== + Hits 1202 1208 +6 + Misses 413 407 -6 + Partials 70 69 -1
Continue to review full report at Codecov.
|
436fca7 to
0e4c062
Compare
To make it easier to test, I extract heartbeatTimer into it's own function.
5024c67 to
3934ca3
Compare
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.
🎉
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.
This looks good to me for now. Maybe long-term we can make beat async, at least up to the file write.
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.
Yes! I briefly looked into it for this PR but since it happens in a request, I wasn't sure if changing it here to make the handler async would causes so I didn't.
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.
Ah yeah I think the move would be to only await in tests. For other uses we can leave them as-is (since we do not want to delay the request to write to the heartbeat file).
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.
Ah gotcha! I did use await when I was first writing the tests but TS said it would have no effect lol but I guess that's because it couldn't infer the callback logic? I'm not sure lol
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.
Ahh yup we would need to make beat async first and then add the await in the tests.
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.
But I think the current workaround is chill
Co-authored-by: Asher <ash@coder.com>
This PR adds tests to increase coverage for
src/node/heart.ts.Related to #5121