GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: retry on smartos if ECONNREFUSED by Trott · Pull Request #3941 · nodejs/node

Conversation

@Trott

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Trott Trott added test Issues and PRs related to Node.js core tests and test infrastructure. smartos Issues and PRs related to the SmartOS platform. labels Nov 20, 2015
@Trott Trott mentioned this pull request Nov 20, 2015
test: fix flaky test-http-chunked-304 on SmartOS #3903
Closed
@Trott

Trott commented Nov 20, 2015

Copy link
Copy Markdown
Member Author

R: @indutny
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Nov 20, 2015

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott mentioned this pull request Nov 20, 2015
failed test: test-http-server-multiheaders on smartos #2815
Closed
@Trott
Trott force-pushed the retry-smartos branch from 6449048 to c5eed86 Compare November 20, 2015 18:29
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#3864
Fixes: nodejs#2815
PR-URL: nodejs#3941
@Trott

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...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

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:....
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Nov 20, 2015

Copy link
Copy Markdown
Member

I think test.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
indutny
indutny reviewed Nov 20, 2015
View reviewed changes
Comment thread tools/test.py Outdated

@indutny indutny Nov 20, 2015

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Trott Trott Nov 20, 2015

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@indutny indutny Nov 20, 2015

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Trott Trott Nov 20, 2015

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@indutny indutny Nov 20, 2015

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@indutny indutny Nov 20, 2015

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Trott

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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Nov 20, 2015

Copy link
Copy Markdown
Member

LGTM, if CI is green
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Nov 20, 2015

Copy link
Copy Markdown
Member

@Trott I think you started wrong CI ;)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Nov 20, 2015

Copy link
Copy Markdown
Member

Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added the lts-watch-v4.x label Nov 20, 2015
@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#3864
Fixes: nodejs#2815
PR-URL: nodejs#3941
@Trott
Trott force-pushed the retry-smartos branch from d44501d to dcac25b Compare November 21, 2015 04:33
@Trott Trott mentioned this pull request Nov 21, 2015
Diagnostic message for retried tests #3960
Merged
@Trott

Trott commented Nov 23, 2015

Copy link
Copy Markdown
Member Author

Landed in 8bc8038
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott 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: #3864
Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Nov 26, 2015
deps: upgrade npm to 3.5.0 #4032
Closed
@MylesBorins MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Dec 1, 2015
Trott added a commit that referenced this pull request Dec 1, 2015
@Trott Myles 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: #3864
Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <[email protected]>
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: #3864
Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <[email protected]>
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: #3864
Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <[email protected]>
This was referenced Dec 8, 2015
Release proposal: v5.2.0 #4180
Closed
Release proposal: v5.2.0 (Stable) #4181
Merged
@MylesBorins MylesBorins added land-on-v4.x and removed lts-landed-on-v4.x labels Dec 17, 2015
@jasnell jasnell mentioned this pull request Dec 17, 2015
Release 4.2.4 Planning #4321
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: #3864
Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <[email protected]>
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: #3864
Fixes: #2815
PR-URL: #3941
Reviewed-By: Fedor Indutny <[email protected]>
@Trott Trott mentioned this pull request Feb 26, 2016
Investigate flaky test-http-regr-gh-2928 #5445
Closed
@Trott Trott mentioned this pull request Feb 26, 2016
test: retry on known SmartOS bug #5454
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 <[email protected]>
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 <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
@Trott Myles 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 <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
@Trott Myles 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 <[email protected]>
@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

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

smartos Issues and PRs related to the SmartOS platform. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@Trott @indutny @jasnell @MylesBorins
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.