Skip to content

test: retry on smartos if ECONNREFUSED - #3941

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:retry-smartos
Closed

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

Conversation

@Trott

@TrottTrott 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

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

Trott commented Nov 20, 2015

Copy link
Copy Markdown
MemberAuthor

R: @indutny

@indutny

indutny commented Nov 20, 2015

Copy link
Copy Markdown
Member

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

@TrottTrott mentioned this pull request Nov 20, 2015
Closed
Trott added a commit to Trott/io.js that referenced this pull request Nov 20, 2015
@Trott
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#3864Fixes: nodejs#2815
PR-URL: nodejs#3941
@Trott

Trott commented Nov 20, 2015

Copy link
Copy Markdown
MemberAuthor

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

Trott commented Nov 20, 2015

Copy link
Copy Markdown
MemberAuthor

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

@indutny

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

indutny commented Nov 20, 2015

Copy link
Copy Markdown
Member

I think test.

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

@indutnyindutnyNov 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.

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

@TrottTrottNov 20, 2015

Copy link
Copy Markdown
MemberAuthor

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

@indutnyindutnyNov 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.

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.

@TrottTrottNov 20, 2015

Copy link
Copy Markdown
MemberAuthor

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.

@indutnyindutnyNov 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.

Yep, let's do it this way!

@indutnyindutnyNov 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.

I mean remove it for now, and open PR later

@Trott

Trott commented Nov 20, 2015

Copy link
Copy Markdown
MemberAuthor

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

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

@indutny

indutny commented Nov 20, 2015

Copy link
Copy Markdown
Member

LGTM, if CI is green

@indutny

indutny commented Nov 20, 2015

Copy link
Copy Markdown
Member

@Trott I think you started wrong CI ;)

@Trott

Trott commented Nov 20, 2015

Copy link
Copy Markdown
MemberAuthor

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

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

@indutny

indutny commented Nov 20, 2015

Copy link
Copy Markdown
Member

Thanks!

@Trott
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#3864Fixes: nodejs#2815
PR-URL: nodejs#3941
@TrottTrott mentioned this pull request Nov 21, 2015
Merged
@Trott

Trott commented Nov 23, 2015

Copy link
Copy Markdown
MemberAuthor

Landed in 8bc8038

@TrottTrott closed this Nov 23, 2015
Trott added a commit that referenced this pull request Nov 23, 2015
@Trott
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: #3864Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@Fishrock123Fishrock123 mentioned this pull request Nov 26, 2015
Closed
Trott added a commit that referenced this pull request Dec 1, 2015
@TrottMyles Borins
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: #3864Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Trott added a commit that referenced this pull request Dec 4, 2015
@Trott@rvagg
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: #3864Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Trott added a commit that referenced this pull request Dec 5, 2015
@Trott@rvagg
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: #3864Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <fedor@indutny.com>
This was referenced Dec 8, 2015
Closed
Merged
@jasnelljasnell mentioned this pull request Dec 17, 2015
Closed
Trott added a commit that referenced this pull request Dec 17, 2015
@Trott@jasnell
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: #3864Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Trott added a commit that referenced this pull request Dec 23, 2015
@Trott@jasnell
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: #3864Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@TrottTrott mentioned this pull request Feb 26, 2016
Closed
@TrottTrott mentioned this pull request Feb 26, 2016
Closed
Trott added a commit to Trott/io.js that referenced this pull request Mar 1, 2016
@Trott
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
@Trott
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
@Trott@Fishrock123
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
@TrottMyles Borins
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
@TrottMyles Borins
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
Trott deleted the retry-smartos branch January 13, 2022 22:30
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

smartosIssues and PRs related to the SmartOS platform.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Trott@indutny@jasnell@MylesBorins