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

test: handle SmartOS bug in test-tls-session-cache by Trott · Pull Request #7505 · nodejs/node · GitHub

/ node Public

test: handle SmartOS bug in test-tls-session-cache - #7505

Closed
Trott wants to merge 3 commits into
nodejs:masterfrom
Trott:opensslcli
ConversationCommits (3)ChecksFiles changed
Closed

test: handle SmartOS bug in test-tls-session-cache#7505
Trott wants to merge 3 commits into
nodejs:masterfrom
Trott:opensslcli

Conversation

Trott commented Jul 1, 2016

Copy link
Copy Markdown
Member
Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test tls

Description of change

Sometimes, a SmartOS bug results in ECONNREFUSED when trying to connect
to the TLS server that the test starts. Retry in that situation.

Fixes: #5111
Refs: https://smartos.org/bugview/OS-2767

Trott added tls Issues and PRs related to the tls subsystem. test Issues and PRs related to the tests. smartos Issues and PRs related to the SmartOS platform. labels Jul 1, 2016
Trott mentioned this pull request Jul 1, 2016
Closed

Trott commented Jul 1, 2016

Copy link
Copy Markdown
Member Author

/cc @nodejs/testing

Trott commented Jul 1, 2016

Copy link
Copy Markdown
Member Author

Stress test so-far showing (and hopefully will continue to show) that this version of the test is robust: https://ci.nodejs.org/job/node-stress-single-test/768/nodes=smartos14-64/console

Stress test showing the version on master branch is flaky: https://ci.nodejs.org/job/node-stress-single-test/767/nodes=smartos14-64/console

Trott commented Jul 1, 2016

Copy link
Copy Markdown
Member Author

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

Trott commented Jul 1, 2016

Copy link
Copy Markdown
Member Author

Stress test failed but now at a later part of the test, I think...

test: handle SmartOS bug in test-tls-session-cache
2acb294 
Sometimes, a SmartOS bug results in ECONNREFUSED when trying to connect
to the TLS server that the test starts. Retry in that situation.

Fixes: nodejs#5111
Refs: https://smartos.org/bugview/OS-2767

Trott commented Jul 1, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

Rebased, force pushed, changed test to decrement a counter on failure, and trying stress test again:
https://ci.nodejs.org/job/node-stress-single-test/769/nodes=smartos14-64/console

Trott commented Jul 2, 2016

Copy link
Copy Markdown
Member Author

That wasn't quite the right fix. Let's try again:

https://ci.nodejs.org/job/node-stress-single-test/775/nodes=smartos14-64/console

Trott commented Jul 2, 2016

Copy link
Copy Markdown
Member Author

Hooray, that seems to have fixed it. PTAL @nodejs/testing

santigimeno commented Jul 2, 2016

Copy link
Copy Markdown
Member

LGTM. One (maybe evident) question though: how is that the process.exit is emitted twice?

Trott commented Jul 2, 2016

Copy link
Copy Markdown
Member Author

@santigimeno exit is being emitted on process only once, but there are two listeners. The process.on('exit', ...) gets run twice because it's inside doTest() which is called twice. So, the listener gets added twice. It's behavior depends on testOptions which is scoped to doTest() so it will have different values (and therefore the listeners will behave differently) even though they are both being triggered by the same event.

santigimeno commented Jul 2, 2016

Copy link
Copy Markdown
Member

Oh. It was pretty obvious in hindsight. Thanks for the explanation @Trott

Trott commented Jul 3, 2016

Copy link
Copy Markdown
Member Author

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

Trott commented Jul 4, 2016

Copy link
Copy Markdown
Member Author

One build issue on CI but no issues with this test.

Trott added a commit to Trott/io.js that referenced this pull request Jul 4, 2016
test: handle SmartOS bug in test-tls-session-cache
fb4c022 
Sometimes, a SmartOS bug results in ECONNREFUSED when trying to connect
to the TLS server that the test starts. Retry in that situation.

Fixes: nodejs#5111
Refs: https://smartos.org/bugview/OS-2767
PR-URL: nodejs#7505
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

Trott commented Jul 4, 2016

Copy link
Copy Markdown
Member Author

Landed in fb4c022

Trott closed this Jul 4, 2016
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
test: handle SmartOS bug in test-tls-session-cache
6da49ac 
Sometimes, a SmartOS bug results in ECONNREFUSED when trying to connect
to the TLS server that the test starts. Retry in that situation.

Fixes: #5111
Refs: https://smartos.org/bugview/OS-2767
PR-URL: #7505
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Fishrock123 mentioned this pull request Jul 5, 2016
Merged
MylesBorins mentioned this pull request Jul 11, 2016
Closed

MylesBorins commented Jul 12, 2016

Copy link
Copy Markdown
Contributor

@Trott lts?

Trott commented Jul 12, 2016

Copy link
Copy Markdown
Member Author

@thealphanerd Yes if it lands cleanly.

MylesBorins commented Jul 12, 2016

Copy link
Copy Markdown
Contributor

does not land cleanly 😢

Trott deleted the opensslcli branch January 13, 2022 22:43
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. tls Issues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Investigate flaky test-tls-session-cache

3 participants

Back | FazBrowse Home | New Git URL