Skip to content
This repository was archived by the owner on Feb 2, 2024. It is now read-only.

Conversation

@yohayg
Copy link
Contributor

@yohayg yohayg commented Jul 10, 2020

Checklist

  • run npm run test and npm run benchmark
  • tests and/or benchmarks are included
  • [ NA] documentation is changed or added
  • commit message and code follows Code of conduct

@jkyberneees
Copy link
Contributor

LGTM +1

package.json Outdated
{
"name": "fast-proxy",
"version": "1.6.3",
"version": "1.6.4",
Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

const bodyParser = require('body-parser')
const expect = require('chai').expect

const sleep = (milliseconds) => new Promise((resolve) => setTimeout(resolve, milliseconds))
Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

res.send()
})
service.get('/service/timeout', async (req, res) => {
await sleep(2000)
Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved - reduced to 200ms

yohay-ma added 2 commits July 13, 2020 21:05
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
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@yohayg
Copy link
Contributor Author

yohayg commented Jul 16, 2020

hi @jkyberneees ,
Can you please merge this one?

@jkyberneees
Copy link
Contributor

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

…-timeout

# Conflicts:
#	test/5.undici.test.js
@yohayg
Copy link
Contributor Author

yohayg commented Jul 16, 2020

@jkyberneees - conflicts are resolved

@jkyberneees jkyberneees merged commit b5927ce into fastify:master Jul 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants