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

test: retry on smartos if ECONNREFUSED #3941

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 20, 2015

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 Trott added test Issues and PRs related to the tests. smartos Issues and PRs related to the SmartOS platform. labels Nov 20, 2015
@Trott
Copy link
Member Author

Trott commented Nov 20, 2015

R: @indutny

@indutny
Copy link
Member

indutny commented Nov 20, 2015

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

Trott added a commit to Trott/io.js that referenced this pull request Nov 20, 2015
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
Copy link
Member Author

Trott commented Nov 20, 2015

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
Copy link
Member Author

Trott commented Nov 20, 2015

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

@indutny
Copy link
Member

indutny commented Nov 20, 2015

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
Copy link
Member

indutny commented Nov 20, 2015

I think test.

if (output.UnexpectedOutput() and
sys.platform == 'sunos5' and
'ECONNREFUSED' in output.output.stderr):
sys.stderr.write('ECONNREFUSED encountered on SmartOS; retrying test.')
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, I'm happy to take it out.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, let's do it this way!

Copy link
Member

Choose a reason for hiding this comment

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

I mean remove it for now, and open PR later

@Trott
Copy link
Member Author

Trott commented Nov 20, 2015

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

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

@indutny
Copy link
Member

indutny commented Nov 20, 2015

LGTM, if CI is green

@indutny
Copy link
Member

indutny commented Nov 20, 2015

@Trott I think you started wrong CI ;)

@Trott
Copy link
Member Author

Trott commented Nov 20, 2015

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

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

@indutny
Copy link
Member

indutny commented Nov 20, 2015

Thanks!

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
Copy link
Member Author

Trott commented Nov 23, 2015

Landed in 8bc8038

@Trott Trott closed this Nov 23, 2015
Trott added a commit that referenced this pull request Nov 23, 2015
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 1, 2015
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
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
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>
@jasnell jasnell mentioned this pull request Dec 17, 2015
Trott added a commit that referenced this pull request Dec 17, 2015
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
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 to Trott/io.js that referenced this pull request Mar 1, 2016
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
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
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
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
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 Trott deleted the retry-smartos branch January 13, 2022 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
smartos Issues and PRs related to the SmartOS platform. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants