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

test: fix `http-upgrade-agent` flakiness by santigimeno · Pull Request #4520 · nodejs/node · GitHub

/ node Public

test: fix http-upgrade-agent flakiness - #4520

Closed
santigimeno wants to merge 1 commit into
nodejs:masterfrom
santigimeno:test-http-upgrade-agent
ConversationCommits (1)ChecksFiles changed
Closed

test: fix http-upgrade-agent flakiness#4520
santigimeno wants to merge 1 commit into
nodejs:masterfrom
santigimeno:test-http-upgrade-agent

Conversation

santigimeno commented Jan 3, 2016

Copy link
Copy Markdown
Member

It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the data event to make sure all
the data is received. Modify the test so it does not pass without
applying this change.

I was getting this error from time to time on OS X:

assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: <Buffer > == 'nurtzo'
    at ClientRequest.<anonymous> (/Users/sgimeno/node/node/test/parallel/test-http-upgrade-agent.js:51:12)
    at emitThree (events.js:98:13)
    at ClientRequest.emit (events.js:176:7)
    at Socket.socketOnData (_http_client.js:342:11)
    at emitOne (events.js:78:13)
    at Socket.emit (events.js:170:7)
    at readableAddChunk (_stream_readable.js:146:16)
    at Socket.Readable.push (_stream_readable.js:110:10)
    at TCP.onread (net.js:523:20)

Trott commented Jan 3, 2016

Copy link
Copy Markdown
Member

Modify the test so it does not pass without applying this change.

This PR only has a change to the test. Is this sentence in error or is there a missing modified file that needs to be added?

brendanashworth added the test Issues and PRs related to the tests. label Jan 4, 2016
mscdex added the http Issues or PRs related to the http subsystem. label Jan 4, 2016
santigimeno force-pushed the test-http-upgrade-agent branch from 8b6e674 to c595680 Compare January 7, 2016 06:47

santigimeno commented Jan 7, 2016

Copy link
Copy Markdown
Member Author

Yes, that sentence was wrong. I rewrote the commit message. PTAL

Trott commented Jan 8, 2016

Copy link
Copy Markdown
Member

The changes to the upgrade event handler looks good to me. I don't think we need the setTimeout() in the test setup. If you do want to force the data in two writes like that, maybe put it in a different test so we have one test that's straightforward and another test that's specific to the data definitely always arriving in two pieces?

santigimeno force-pushed the test-http-upgrade-agent branch from c595680 to 8ac8915 Compare January 8, 2016 21:56

santigimeno commented Jan 8, 2016

Copy link
Copy Markdown
Member Author

@Trott PR updated with your comments. Thanks

Trott commented Jan 9, 2016

Copy link
Copy Markdown
Member

LGTM if CI is happy.

CI: https://ci.nodejs.org/job/node-test-commit/1664/

We need to get you onboarded so you can start these CI jobs yourself!

Minor nit that can be ignored if you want: I don't think we need the gotData variable and check in the new test since we're guaranteeing the check of the value of recvData which can't possibly be set correctly unless gotData is true. But it doesn't hurt anything either, I guess.

Trott commented Jan 9, 2016

Copy link
Copy Markdown
Member

One other comment that you can ignore if you want: The same issue you're fixing here seems to afflict test-http-upgrade-client.js as well. Maybe fix that too, either here or in a separate PR? (I'm a fan of separate PR, but either way.)

santigimeno force-pushed the test-http-upgrade-agent branch from 8ac8915 to d710d9c Compare January 9, 2016 11:20

santigimeno commented Jan 9, 2016

Copy link
Copy Markdown
Member Author

@Trott I've removed the gotData variable.
I'll fix the other test in a separate PR

Trott commented Jan 9, 2016

Copy link
Copy Markdown
Member

Looks like the modified test fails (or is flaky?) on FreeBSD and Raspberry Pi:

santigimeno commented Jan 9, 2016

Copy link
Copy Markdown
Member Author

@Trott It seems the CI was started with the previous version of the PR: 8ac8915 (the one with gotData) and not the current one: santigimeno@d710d9c. Anyway, it's been helpful as it has shown that in those environments all the upgrade data was sent in only 1 chunk. I'll increase the timeout a little bit and maybe bring back the gotData variable as it has proven to be useful. What do you think?

santigimeno commented Jan 9, 2016

Copy link
Copy Markdown
Member Author

Nah, increasing the timeout won't work either as the client is closing the connection when it receives the upgrade request. I think I'm removing the test altogether as it may happen that the second write could fail due to a race condition with the client closing the connection.

santigimeno force-pushed the test-http-upgrade-agent branch from d710d9c to 2368c95 Compare January 9, 2016 23:30

santigimeno commented Jan 9, 2016

Copy link
Copy Markdown
Member Author

PR updated. I think this should work

Trott commented Jan 10, 2016

Copy link
Copy Markdown
Member

One nit, but LGTM if CI doesn't uncover anything.

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

Trott reviewed Jan 10, 2016
View reviewed changes
Comment thread test/parallel/test-http-upgrade-agent.js Outdated

Trott Jan 10, 2016

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

Nit: As mscdex noted in the other PR that's similar to this, this callback might benefit from being wrapped in common.mustCall() to guarantee that it fires.

test: fix http-upgrade-agent flakiness
6e84dda 
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.
santigimeno force-pushed the test-http-upgrade-agent branch from 2368c95 to 6e84dda Compare January 11, 2016 20:21

santigimeno commented Jan 11, 2016

Copy link
Copy Markdown
Member Author

PR updated. Thanks

Trott commented Jan 11, 2016

Copy link
Copy Markdown
Member

LGTM.

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

jasnell commented Jan 12, 2016

Copy link
Copy Markdown
Member

LGTM

jasnell pushed a commit that referenced this pull request Jan 12, 2016
test: fix http-upgrade-agent flakiness
6018fa1 
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: #4520
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell commented Jan 12, 2016

Copy link
Copy Markdown
Member

Landed in 6018fa1

jasnell closed this Jan 12, 2016
rvagg pushed a commit that referenced this pull request Jan 14, 2016
test: fix http-upgrade-agent flakiness
afa4c1a 
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: #4520
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas mentioned this pull request Jan 18, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
test: fix http-upgrade-agent flakiness
06f118f 
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: #4520
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
test: fix http-upgrade-agent flakiness
6ef981e 
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: #4520
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
test: fix http-upgrade-agent flakiness
f10c937 
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: nodejs#4520
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins mentioned this pull request Feb 11, 2016
Merged
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
test: fix http-upgrade-agent flakiness
3d5bc69 
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: nodejs#4520
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
test: fix http-upgrade-agent flakiness
046cfc4 
It's not guaranteed that the socket data is received in the same chunk
as the upgrade response. Listen for the `data` event to make sure all
the data is received.

PR-URL: nodejs#4520
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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

http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL