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.

Sign up for GitHub

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

Jump to bottom

http2,test: mark test-http2-client-upload flaky on win32 #17356

Closed
wants to merge 1 commit into from
Closed

http2,test: mark test-http2-client-upload flaky on win32 #17356

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Nov 27, 2017

Test recently started failing only in CI, unable to recreate locally on multiple test machines. Need to investigate further.

/cc @addaleax

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

http2, test

@jasnell
http2,test: mark test-http2-client-upload flaky on win32
f40ffea 
Test recently started failing only in CI, unable
to recreate locally on multiple test machines.
Need to investigate further.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 27, 2017
@jasnell
Copy link
Member Author

jasnell commented Nov 27, 2017

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

@addaleax addaleax added fast-track PRs that do not need to wait for 48 hours to land. http2 Issues or PRs related to the http2 subsystem. labels Nov 27, 2017
@jasnell
Copy link
Member Author

jasnell commented Nov 27, 2017

Assuming CI comes back good, I'd like to fast track this to get CI green again.

refack
refack approved these changes Nov 27, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Just about to suggest exactly this

@refack refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 27, 2017
@addaleax addaleax removed fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 27, 2017
addaleax added a commit to addaleax/node that referenced this pull request Nov 27, 2017
@addaleax
test: fix flaky parallel/test-http2-client-upload
31861a7 
In parallel/test-http2-client-upload, the `client.destroy()`
call could terminate the connection before all data was sent
over the wire successfully.

Using `client.shutdown()` removes the flakiness.

Also, listen on `req.on('finish')` rather than the file stream’s
`end` event, since we’re not interested in when the source stream
finishes, but rather when the HTTP/2 stream finishes.

Refs: nodejs#17356
@addaleax addaleax mentioned this pull request Nov 27, 2017
Closed
3 tasks
@addaleax
Copy link
Member

Alternatively, here’s a fix: #17361

@addaleax addaleax closed this Nov 28, 2017
addaleax added a commit that referenced this pull request Nov 28, 2017
@addaleax
test: fix flaky parallel/test-http2-client-upload
6fc8765 
In parallel/test-http2-client-upload, the `client.destroy()`
call could terminate the connection before all data was sent
over the wire successfully.

Using `client.shutdown()` removes the flakiness.

Also, listen on `req.on('finish')` rather than the file stream’s
`end` event, since we’re not interested in when the source stream
finishes, but rather when the HTTP/2 stream finishes.

PR-URL: #17361
Refs: #17356
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@addaleax @MylesBorins
test: fix flaky parallel/test-http2-client-upload
17f39b2 
In parallel/test-http2-client-upload, the `client.destroy()`
call could terminate the connection before all data was sent
over the wire successfully.

Using `client.shutdown()` removes the flakiness.

Also, listen on `req.on('finish')` rather than the file stream’s
`end` event, since we’re not interested in when the source stream
finishes, but rather when the HTTP/2 stream finishes.

PR-URL: #17361
Refs: #17356
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@addaleax @MylesBorins
test: fix flaky parallel/test-http2-client-upload
ee25bc1 
In parallel/test-http2-client-upload, the `client.destroy()`
call could terminate the connection before all data was sent
over the wire successfully.

Using `client.shutdown()` removes the flakiness.

Also, listen on `req.on('finish')` rather than the file stream’s
`end` event, since we’re not interested in when the source stream
finishes, but rather when the HTTP/2 stream finishes.

PR-URL: #17361
Refs: #17356
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@addaleax @MylesBorins
test: fix flaky parallel/test-http2-client-upload
1e3aed0 
In parallel/test-http2-client-upload, the `client.destroy()`
call could terminate the connection before all data was sent
over the wire successfully.

Using `client.shutdown()` removes the flakiness.

Also, listen on `req.on('finish')` rather than the file stream’s
`end` event, since we’re not interested in when the source stream
finishes, but rather when the HTTP/2 stream finishes.

PR-URL: #17361
Refs: #17356
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@addaleax @gibfahn
test: fix flaky parallel/test-http2-client-upload
0b45b42 
In parallel/test-http2-client-upload, the `client.destroy()`
call could terminate the connection before all data was sent
over the wire successfully.

Using `client.shutdown()` removes the flakiness.

Also, listen on `req.on('finish')` rather than the file stream’s
`end` event, since we’re not interested in when the source stream
finishes, but rather when the HTTP/2 stream finishes.

PR-URL: #17361
Refs: #17356
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@addaleax @gibfahn
test: fix flaky parallel/test-http2-client-upload
341f0c5 
In parallel/test-http2-client-upload, the `client.destroy()`
call could terminate the connection before all data was sent
over the wire successfully.

Using `client.shutdown()` removes the flakiness.

Also, listen on `req.on('finish')` rather than the file stream’s
`end` event, since we’re not interested in when the source stream
finishes, but rather when the HTTP/2 stream finishes.

PR-URL: #17361
Refs: #17356
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
http2 Issues or PRs related to the http2 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.
4 participants
@jasnell @addaleax @refack @nodejs-github-bot