FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: fix flaky test-net-socket-timeout-unref by Trott · Pull Request #6003 · nodejs/node · GitHub

/ node Public

test: fix flaky test-net-socket-timeout-unref - #6003

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:log-it
ConversationCommits (1)ChecksFiles changed
Closed

test: fix flaky test-net-socket-timeout-unref#6003
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:log-it

Conversation

Trott commented Apr 1, 2016

Copy link
Copy Markdown
Member

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?

Affected core subsystem(s)

test, net

Description of change

Throw immediately on socket timeout rather than checking boolean in exit
handler.

Fixes: #5128

test: fix flaky test-net-socket-timeout-unref
b7c64fb 
Throw immediately on socket timeout rather than checking boolean in exit
handler.

Fixes: nodejs#5128
Trott added net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests. lts-watch-v4.x labels Apr 1, 2016

Trott commented Apr 1, 2016

Copy link
Copy Markdown
Member Author

Stress test confirming flakiness of current version on master: https://ci.nodejs.org/job/node-stress-single-test/584/nodes=pi2-raspbian-wheezy/console

Stress test showing this version is not flaky: https://ci.nodejs.org/job/node-stress-single-test/583/nodes=pi2-raspbian-wheezy/console

CI: https://ci.nodejs.org/job/node-test-pull-request/2128/

Trott commented Apr 1, 2016

Copy link
Copy Markdown
Member Author

@nodejs/testing

Trott mentioned this pull request Apr 1, 2016
Closed

santigimeno commented Apr 1, 2016

Copy link
Copy Markdown
Member

LGTM

MylesBorins reviewed Apr 1, 2016
View reviewed changes
Comment thread test/parallel/test-net-socket-timeout-unref.js
process.on('exit', function() {
assert.strictEqual(timedout, false,
'Socket timeout should not hold loop open');
sockets.push({socket: socket, timeout: T * 1000});

MylesBorins Apr 1, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

should this timout be platform specific?

Trott Apr 1, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is a socket timeout and not a timer, so I'm inclined to not make it platform dependent if it doesn't need to be.

MylesBorins Apr 1, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ahhh... /me leaves and reads more about socket time outs

MylesBorins commented Apr 1, 2016

Copy link
Copy Markdown
Contributor

LGTM with above nit assuming CI is green

Trott added a commit to Trott/io.js that referenced this pull request Apr 4, 2016
test: fix flaky test-net-socket-timeout-unref
c60faf6 
Throw immediately on socket timeout rather than checking boolean in exit
handler.

PR-URL: nodejs#6003
Fixes: nodejs#5128
Reviewed-By: Myles Borins <myles.borins@gmail.com>

Trott commented Apr 4, 2016

Copy link
Copy Markdown
Member Author

Landed in c60faf6

Trott closed this Apr 4, 2016
MylesBorins pushed a commit that referenced this pull request Apr 5, 2016
test: fix flaky test-net-socket-timeout-unref
2ab1237 
Throw immediately on socket timeout rather than checking boolean in exit
handler.

PR-URL: #6003
Fixes: #5128
Reviewed-By: Myles Borins <myles.borins@gmail.com>
This was referenced Apr 5, 2016
Closed
Merged
MylesBorins pushed a commit that referenced this pull request Apr 11, 2016
test: fix flaky test-net-socket-timeout-unref
e7877e6 
Throw immediately on socket timeout rather than checking boolean in exit
handler.

PR-URL: #6003
Fixes: #5128
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins mentioned this pull request Apr 11, 2016
Merged
Trott deleted the log-it branch January 13, 2022 22:42
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL