chore: removes done callbacks in tests [CHORE-1870]#1875
chore: removes done callbacks in tests [CHORE-1870]#1875yowainwright merged 13 commits intomasterfrom
Conversation
efa8258 to
cf39839
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1875 +/- ##
=======================================
Coverage 99.90% 99.90%
=======================================
Files 9 9
Lines 2049 2049
=======================================
Hits 2047 2047
Misses 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cf39839 to
09614e8
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR cleans up the test suite by refactoring tests to use async/await instead of callback-based done functions. Key changes include refactoring the test cases in multiple test files to an async style, removing outdated done callback usages, and updating event handling accordingly.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/response/writable.test.js | Converted tests to async/await and removed done callbacks. |
| tests/response/redirect.test.js | Updated redirect test from callback to async/await style. |
| tests/response/flushHeaders.test.js | Refactored tests to async with updated error handling. |
| tests/request/href.test.js | Changed test callback style to async/await. |
| tests/context/onerror.test.js | Converted error-handling tests to async/await style. |
| tests/application/respond.test.js | Switched from callback to async/await in error emission tests. |
| tests/application/index.test.js | Updated socket error tests to async style for consistency. |
6ff69b2 to
609aa63
Compare
609aa63 to
83e2601
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors test files to remove done callbacks in favor of async/await. The changes streamline asynchronous operations and simplify error handling across various test suites.
- Converted tests from callback-based patterns to async/await.
- Replaced custom request functions with supertest’s promise-based API.
- Reorganized error handling for improved consistency.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/response/writable.test.js | Refactored continuous requests test to use async/await and once events. |
| tests/response/redirect.test.js | Converted redirect tests to async/await and removed done callbacks. |
| tests/response/flushHeaders.test.js | Updated tests for flushHeaders with async/await and improved error triggering. |
| tests/request/href.test.js | Modernized GET request test by replacing HTTP module code with supertest. |
| tests/context/onerror.test.js | Refactored error handling tests to use async/await in onerror scenarios. |
| tests/application/respond.test.js | Updated error emission tests to employ async/await. |
| tests/application/index.test.js | Replaced callback style with async/await for socket error handling. |
| tests/application/flushHeaders.test.js | Added a new async/await test file for flushHeaders functionality. |
Co-authored-by: jongleberry <jonathanrichardong@gmail.com>
| port | ||
| }) | ||
| .on('response', res => { | ||
| const onData = () => done(new Error('boom')) |
There was a problem hiding this comment.
hmmm I'm not sure what this test is doing, I need to look deeper
| const onData = () => done(new Error('boom')) | ||
| res.on('data', onData) | ||
|
|
||
| // shouldn't receive any data for a while |
There was a problem hiding this comment.
this test is not valid since the listener is added and removed and the test is complete on the same tick - the logic only happens on next tick. we can keep it for now but it's a bad test
There was a problem hiding this comment.
I updated this test! 👀
Checklist
Chore: removes done callback; prefer async