-
-
Notifications
You must be signed in to change notification settings - Fork 30
issue: #33 - Should support undici timeout err.code #34
Conversation
update upstream master
|
LGTM +1 |
package.json
Outdated
| { | ||
| "name": "fast-proxy", | ||
| "version": "1.6.3", | ||
| "version": "1.6.4", |
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.
I prefer we bump this during release.
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.
resolved
test/5.undici.test.js
Outdated
| const bodyParser = require('body-parser') | ||
| const expect = require('chai').expect | ||
|
|
||
| const sleep = (milliseconds) => new Promise((resolve) => setTimeout(resolve, milliseconds)) |
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.
I would use promisify(setTimeout) instead.
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.
resolved
test/5.undici.test.js
Outdated
| res.send() | ||
| }) | ||
| service.get('/service/timeout', async (req, res) => { | ||
| await sleep(2000) |
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.
why do we need 2 seconds of wait? I would prefer not to increase the slow down the testing.
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.
resolved - reduced to 200ms
1. use promisify for sleep function 2. reduce sleep time from 2 sec to 200 ms
1. use promisify for sleep function 2. reduce sleep time from 2 sec to 200 ms 3. removed test timeout
mcollina
left a comment
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.
lgtm
|
hi @jkyberneees , |
|
Hi @yohayg sorry for the delay on this, crazy busy on my side. Could you please resolve the conflicts here so I can merge and release both commits. |
…-timeout # Conflicts: # test/5.undici.test.js
|
@jkyberneees - conflicts are resolved |
Checklist
npm run testandnpm run benchmark