Skip to content
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(node-http-handler): set maxSockets above 0 in test #1660

Merged
merged 1 commit into from
Nov 5, 2020
Merged

fix(node-http-handler): set maxSockets above 0 in test #1660

merged 1 commit into from
Nov 5, 2020

Conversation

mhart
Copy link
Contributor

@mhart mhart commented Nov 5, 2020

Issue #, if available:

#1539

Description of changes:

Just browsing, but it looks at first glance like the issue with the test is that maxSockets will sometimes be set to 0 – due to Math.random() and Math.round(). My guess is that a setting of maxSockets = 0 gets turned into Infinity – so the test will fail in these cases. This change ensures maxSockets will always be at least 1 in this test.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Member

@trivikr trivikr left a comment

Choose a reason for hiding this comment

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

Thank you @mhart for your first PR. I've added nits to use Math.ceil instead.

Tested in Chrome Developer Tools:

> Math.ceil(0.00001)
1

@trivikr trivikr changed the title Ensure maxSockets is always above 0 in NodeHttpHandler constructor tests test: set maxSockets above 0 in NodeHttpHandler constructor tests Nov 5, 2020
@trivikr trivikr changed the title test: set maxSockets above 0 in NodeHttpHandler constructor tests fix(node-http-handler): set maxSockets above 0 in test Nov 5, 2020
@mhart
Copy link
Contributor Author

mhart commented Nov 5, 2020

Thank you @mhart for your first PR. I've added nits to use Math.ceil instead.

Tested in Chrome Developer Tools:

> Math.ceil(0.00001)
1

Yeah, but Math.ceil(0) === 0 😉

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 10, 2021
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.

2 participants