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

test: fix flaky child-process-fork-regr-gh-2847 by mscdex · Pull Request #4442 · nodejs/node · GitHub

/ node Public

test: fix flaky child-process-fork-regr-gh-2847 - #4442

Closed
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:fix-win-flaky-test-gh2847
ConversationCommits (1)ChecksFiles changed
Closed

test: fix flaky child-process-fork-regr-gh-2847#4442
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:fix-win-flaky-test-gh2847

Conversation

mscdex commented Dec 27, 2015

Copy link
Copy Markdown
Contributor

Windows would die with ECONNRESET most times when running this particular test. This commit makes handling these errors more tolerable.

I'm not sure if the error handling logic is 100% correct here. Is silencing ECONNRESET errors on server-side sockets ever acceptable, or are those particular errors relevant to this test?

/cc @mhdawson @jasnell @indutny @trevnorris

mscdex added child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform. test Issues and PRs related to the tests. labels Dec 27, 2015

indutny commented Dec 27, 2015

Copy link
Copy Markdown
Member

I think I'm fine with this change as long as it fails on unpatched node.js (i.e. before this regression was fixed).

Trott reviewed Dec 27, 2015
View reviewed changes
Comment thread test/parallel/test-child-process-fork-regr-gh-2847.js Outdated

Trott Dec 27, 2015

Copy link
Copy Markdown
Member

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

Maybe wrap this in an if (common.isWindows) so it only applies to the affected OS? Or not. Just throwing it out there.

mscdex commented Dec 27, 2015

Copy link
Copy Markdown
Contributor Author

@indutny Yes, the tests still properly fail before the fix (e.g. node v4.1.1).

indutny commented Dec 27, 2015

Copy link
Copy Markdown
Member

Ok, then one nit by @Trott . Otherwise LGTM

test: fix flaky child-process-fork-regr-nodejsgh-2847
57a400e 
Windows would die with ECONNRESET most times when running
this particular test. This commit makes handling these errors
more tolerable.
mscdex force-pushed the fix-win-flaky-test-gh2847 branch from af005ce to 57a400e Compare December 28, 2015 00:24

mscdex commented Dec 28, 2015

Copy link
Copy Markdown
Contributor Author

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

mscdex commented Dec 28, 2015

Copy link
Copy Markdown
Contributor Author

Whoa, that was weird... suddenly all kinds of seemingly unrelated issues on Windows.

mscdex commented Dec 28, 2015

Copy link
Copy Markdown
Contributor Author

I re-ran them all again and this time it's all green: https://ci.nodejs.org/job/node-test-pull-request/1086/

I'm guessing the previous time it was something CI related?

jbergstroem commented Dec 28, 2015

Copy link
Copy Markdown
Member

@mscdex yeah, not sure what was up with the windows slaves last run.

Trott commented Dec 28, 2015

Copy link
Copy Markdown
Member

Green! Another all green!!!! \o/

mscdex commented Dec 28, 2015

Copy link
Copy Markdown
Contributor Author

Stress test: https://ci.nodejs.org/job/node-stress-single-test/nodes=win2008r2/229/

mscdex commented Dec 29, 2015

Copy link
Copy Markdown
Contributor Author

Stress test is green! \o/

jbergstroem commented Dec 29, 2015

Copy link
Copy Markdown
Member

Trott commented Dec 29, 2015

Copy link
Copy Markdown
Member

LGTM

mscdex added a commit that referenced this pull request Dec 29, 2015
test: fix flaky child-process-fork-regr-gh-2847
30c0062 
Windows would die with ECONNRESET most times when running
this particular test. This commit makes handling these errors
more tolerable.

PR-URL: #4442
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

mscdex commented Dec 29, 2015

Copy link
Copy Markdown
Contributor Author

Landed in 30c0062.

mscdex closed this Dec 29, 2015
mscdex deleted the fix-win-flaky-test-gh2847 branch December 29, 2015 04:33
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
test: fix flaky child-process-fork-regr-nodejsgh-2847
03c4bc7 
Windows would die with ECONNRESET most times when running
this particular test. This commit makes handling these errors
more tolerable.

PR-URL: nodejs#4442
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins mentioned this pull request Jan 14, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
test: fix flaky child-process-fork-regr-gh-2847
ee0d8a2 
Windows would die with ECONNRESET most times when running
this particular test. This commit makes handling these errors
more tolerable.

PR-URL: #4442
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
santigimeno mentioned this pull request Feb 10, 2016
Closed
santigimeno added a commit to santigimeno/node that referenced this pull request Feb 10, 2016
test: fix child-process-fork-regr-nodejsgh-2847 again
3cf2461 
Windows is still sometimes failing with ECONNRESET. Bring back the
handling of this error as was initially introduced in PR nodejs#4442.
Trott pushed a commit to Trott/io.js that referenced this pull request Feb 10, 2016
test: fix child-process-fork-regr-nodejsgh-2847 again
24667ee 
Windows is still sometimes failing with ECONNRESET. Bring back the
handling of this error as was initially introduced in PR nodejs#4442.

PR-URL: nodejs#5179
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fixes: nodejs#3635
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
test: fix flaky child-process-fork-regr-gh-2847
849fda9 
Windows would die with ECONNRESET most times when running
this particular test. This commit makes handling these errors
more tolerable.

PR-URL: #4442
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
test: fix flaky child-process-fork-regr-nodejsgh-2847
ceb8460 
Windows would die with ECONNRESET most times when running
this particular test. This commit makes handling these errors
more tolerable.

PR-URL: nodejs#4442
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins mentioned this pull request Feb 11, 2016
Merged
rvagg pushed a commit that referenced this pull request Feb 15, 2016
test: fix child-process-fork-regr-gh-2847 again
7c88410 
Windows is still sometimes failing with ECONNRESET. Bring back the
handling of this error as was initially introduced in PR #4442.

PR-URL: #5179
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fixes: #3635
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
test: fix flaky child-process-fork-regr-nodejsgh-2847
6809c2b 
Windows would die with ECONNRESET most times when running
this particular test. This commit makes handling these errors
more tolerable.

PR-URL: nodejs#4442
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
test: fix child-process-fork-regr-gh-2847 again
424c569 
Windows is still sometimes failing with ECONNRESET. Bring back the
handling of this error as was initially introduced in PR #4442.

PR-URL: #5179
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fixes: #3635
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
test: fix child-process-fork-regr-gh-2847 again
b639c33 
Windows is still sometimes failing with ECONNRESET. Bring back the
handling of this error as was initially introduced in PR #4442.

PR-URL: #5179
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fixes: #3635
stefanmb pushed a commit to stefanmb/node that referenced this pull request Feb 23, 2016
test: fix child-process-fork-regr-nodejsgh-2847 again
ecff686 
Windows is still sometimes failing with ECONNRESET. Bring back the
handling of this error as was initially introduced in PR nodejs#4442.

PR-URL: nodejs#5179
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fixes: nodejs#3635
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
test: fix child-process-fork-regr-gh-2847 again
972c063 
Windows is still sometimes failing with ECONNRESET. Bring back the
handling of this error as was initially introduced in PR #4442.

PR-URL: #5179
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fixes: #3635
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
test: fix flaky child-process-fork-regr-nodejsgh-2847
a9c96b6 
Windows would die with ECONNRESET most times when running
this particular test. This commit makes handling these errors
more tolerable.

PR-URL: nodejs#4442
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This was referenced Oct 16, 2020
Merged
Open
Open
This was referenced Nov 18, 2020
Merged
Open
Open
leonardoadame mentioned this pull request May 16, 2024
Open
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

child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL