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

test: retry on smartos if ECONNREFUSED by Trott · Pull Request #3941 · nodejs/node · GitHub

/ node Public

test: retry on smartos if ECONNREFUSED - #3941

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

test: retry on smartos if ECONNREFUSED#3941
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:retry-smartos

Conversation

Trott commented Nov 20, 2015

Copy link
Copy Markdown
Member

SmartOS has a bug that causes unexpected ECONNREFUSED errors.

See https://smartos.org/bugview/OS-2767

If ECONNREFUSED on SmartOS, retry the test one time.

Fixes: #3864

Trott added test Issues and PRs related to the tests. smartos Issues and PRs related to the SmartOS platform. labels Nov 20, 2015
Trott mentioned this pull request Nov 20, 2015
Closed

Trott commented Nov 20, 2015

Copy link
Copy Markdown
Member Author

R: @indutny

indutny commented Nov 20, 2015

Copy link
Copy Markdown
Member

Is retrying just once generally enough? Perhaps it should log something to stderr?

Trott mentioned this pull request Nov 20, 2015
Closed
Trott added a commit to Trott/io.js that referenced this pull request Nov 20, 2015
test: retry on smartos if ECONNREFUSED
c5eed86 
SmartOS has a bug that causes unexpected ECONNREFUSED errors.

See https://smartos.org/bugview/OS-2767

If ECONNREFUSED on SmartOS, retry the test one time.

Fixes: nodejs#3864
Fixes: nodejs#2815
PR-URL: nodejs#3941

Trott commented Nov 20, 2015

Copy link
Copy Markdown
Member Author

In most cases, once is probably enough. If stress testing a single test, we typically have to run hundreds of times before we see a failure like this. The exception might be the max-connections test that opens 200 connection attempts, so it may fail much more often. But that test has a fix for this problem within itself, which is probably appropriate. It's a special case.

I skipped logging to stderr because I wasn't sure it wouldn't mess up TAP output etc. But I can take a closer look and sort that out...

Trott commented Nov 20, 2015

Copy link
Copy Markdown
Member Author

Bikeshed question, feel free to ignore: Should the commit message be test: or tool: or test,tool:....

indutny commented Nov 20, 2015

Copy link
Copy Markdown
Member

You are right, logging it this way will break TAP (most likely). Let's keep it as it is for now, and then fix somewhere later.

indutny commented Nov 20, 2015

Copy link
Copy Markdown
Member

I think test.

indutny reviewed Nov 20, 2015
View reviewed changes
Comment thread tools/test.py Outdated

indutny Nov 20, 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

Sorry, but I think I was wrong about it. This sounds like a bad idea. We should fix it later.

Trott Nov 20, 2015

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

I created a new branch and moved the write() statement outside of the if so that it triggered every time. It didn't seem to mess up TAP. https://ci.nodejs.org/job/node-test-commit-smartos/387/nodes=smartos14-32/console

So, we could leave it in if the info is helpful. (I'll have to add a \n to the print string, but that's easy enough of course.)

indutny Nov 20, 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

It actually should use some of the classes in this file to produce correct output, be it TAP or anything else. Right now it doesn't fit into the global scheme of test.py, this is why I am a bit worried about leaving it as it is.

Trott Nov 20, 2015

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

Cool, I'm happy to take it out.

indutny Nov 20, 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

Yep, let's do it this way!

indutny Nov 20, 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

I mean remove it for now, and open PR later

Trott commented Nov 20, 2015

Copy link
Copy Markdown
Member Author

Logging to stderr removed, will figure that out in a subsequent PR.

CI: https://ci.nodejs.org/job/node-accept-pull-request/131/

indutny commented Nov 20, 2015

Copy link
Copy Markdown
Member

LGTM, if CI is green

indutny commented Nov 20, 2015

Copy link
Copy Markdown
Member

@Trott I think you started wrong CI ;)

Trott commented Nov 20, 2015

Copy link
Copy Markdown
Member Author

@indutny Indeed. Let's try again...

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

indutny commented Nov 20, 2015

Copy link
Copy Markdown
Member

Thanks!

test: retry on smartos if ECONNREFUSED
dcac25b 
SmartOS has a bug that causes unexpected ECONNREFUSED errors.

See https://smartos.org/bugview/OS-2767

If ECONNREFUSED on SmartOS, retry the test one time.

Fixes: nodejs#3864
Fixes: nodejs#2815
PR-URL: nodejs#3941
Trott mentioned this pull request Nov 21, 2015
Merged

Trott commented Nov 23, 2015

Copy link
Copy Markdown
Member Author

Landed in 8bc8038

Trott closed this Nov 23, 2015
Trott added a commit that referenced this pull request Nov 23, 2015
test: retry on smartos if ECONNREFUSED
8bc8038 
SmartOS has a bug that causes unexpected ECONNREFUSED errors.

See https://smartos.org/bugview/OS-2767

If ECONNREFUSED on SmartOS, retry the test one time.

Fixes: #3864
Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Fishrock123 mentioned this pull request Nov 26, 2015
Closed
Trott added a commit that referenced this pull request Dec 1, 2015
test: retry on smartos if ECONNREFUSED
9bc9a2b 
SmartOS has a bug that causes unexpected ECONNREFUSED errors.

See https://smartos.org/bugview/OS-2767

If ECONNREFUSED on SmartOS, retry the test one time.

Fixes: #3864
Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Trott added a commit that referenced this pull request Dec 4, 2015
test: retry on smartos if ECONNREFUSED
e2dee8d 
SmartOS has a bug that causes unexpected ECONNREFUSED errors.

See https://smartos.org/bugview/OS-2767

If ECONNREFUSED on SmartOS, retry the test one time.

Fixes: #3864
Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Trott added a commit that referenced this pull request Dec 5, 2015
test: retry on smartos if ECONNREFUSED
1fe4d30 
SmartOS has a bug that causes unexpected ECONNREFUSED errors.

See https://smartos.org/bugview/OS-2767

If ECONNREFUSED on SmartOS, retry the test one time.

Fixes: #3864
Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <fedor@indutny.com>
This was referenced Dec 8, 2015
Closed
Merged
jasnell mentioned this pull request Dec 17, 2015
Closed
Trott added a commit that referenced this pull request Dec 17, 2015
test: retry on smartos if ECONNREFUSED
5a27274 
SmartOS has a bug that causes unexpected ECONNREFUSED errors.

See https://smartos.org/bugview/OS-2767

If ECONNREFUSED on SmartOS, retry the test one time.

Fixes: #3864
Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Trott added a commit that referenced this pull request Dec 23, 2015
test: retry on smartos if ECONNREFUSED
ff2266a 
SmartOS has a bug that causes unexpected ECONNREFUSED errors.

See https://smartos.org/bugview/OS-2767

If ECONNREFUSED on SmartOS, retry the test one time.

Fixes: #3864
Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Trott mentioned this pull request Feb 26, 2016
Closed
Trott mentioned this pull request Feb 26, 2016
Closed
Trott added a commit to Trott/io.js that referenced this pull request Mar 1, 2016
test: retry on known SmartOS bug
bb388bc 
There is a known issue with SmartOS that is generally worked around
in `tools/test.py`. However, a more robust workaround is required for
some tests that open many network connections.

`test-http-regr-nodejsgh-2928` is one such test.

Fixes: nodejs#5445
Refs: nodejs#3941
PR-URL: nodejs#5454
Trott added a commit to Trott/io.js that referenced this pull request Mar 1, 2016
test: retry on known SmartOS bug
8592697 
There is a known issue with SmartOS that is generally worked around
in `tools/test.py`. However, a more robust workaround is required for
some tests that open many network connections.

`test-http-regr-nodejsgh-2928` is one such test.

Fixes: nodejs#5445
Refs: nodejs#3941
PR-URL: nodejs#5454
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Fishrock123 pushed a commit that referenced this pull request Mar 2, 2016
test: retry on known SmartOS bug
d1a82c6 
There is a known issue with SmartOS that is generally worked around
in `tools/test.py`. However, a more robust workaround is required for
some tests that open many network connections.

`test-http-regr-gh-2928` is one such test.

Fixes: #5445
Refs: #3941
PR-URL: #5454
Reviewed-By: Fedor Indutny <fedor@indutny.com>
MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
test: retry on known SmartOS bug
7c0fc04 
There is a known issue with SmartOS that is generally worked around
in `tools/test.py`. However, a more robust workaround is required for
some tests that open many network connections.

`test-http-regr-gh-2928` is one such test.

Fixes: #5445
Refs: #3941
PR-URL: #5454
Reviewed-By: Fedor Indutny <fedor@indutny.com>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
test: retry on known SmartOS bug
b4ef644 
There is a known issue with SmartOS that is generally worked around
in `tools/test.py`. However, a more robust workaround is required for
some tests that open many network connections.

`test-http-regr-gh-2928` is one such test.

Fixes: #5445
Refs: #3941
PR-URL: #5454
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Trott deleted the retry-smartos branch January 13, 2022 22:30
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

smartos Issues and PRs related to the SmartOS platform. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL