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.

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

test: move back test-http-dump-req-when-res-ends #19866

Closed

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Apr 7, 2018

Move back test-http-dump-req-when-res-ends to test/parallel/

Refs: #19823

$ tools/test.py -j 96 --repeat 192 test/parallel/test-http-dump-req-when-res-ends.js
[00:08|% 100|+ 192|-   0]: Done
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 7, 2018
@lpinca lpinca requested review from mcollina and Trott April 7, 2018 12:56
@lpinca
Copy link
Member Author

lpinca commented Apr 7, 2018

@BridgeAR BridgeAR added 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 Apr 7, 2018
@mcollina
Copy link
Member

mcollina commented Apr 7, 2018

Can you run that same command on our CI. I never remember the task.

@lpinca
Copy link
Member Author

lpinca commented Apr 7, 2018

This is on Ubuntu 16.04: https://ci.nodejs.org/job/node-stress-single-test/1797/ https://ci.nodejs.org/job/node-stress-single-test/1798/
It is recommended to not use All.

@Trott
Copy link
Member

Trott commented Apr 7, 2018

The previous stress test didn't use parallelism. If you don't specify -j and --repeat in the stress test parameters, it's like running it in sequential.

Here's a stress test that runs the test with multiple tests running simultaneously: https://ci.nodejs.org/job/node-stress-single-test/1799/

@Trott
Copy link
Member

Trott commented Apr 7, 2018

Removing fast-track label. This should be evaluated carefully.

@Trott Trott removed the fast-track PRs that do not need to wait for 48 hours to land. label Apr 7, 2018
@lpinca
Copy link
Member Author

lpinca commented Apr 7, 2018

@Trott on what platform it fails? The above stress test passed.

@Trott
Copy link
Member

Trott commented Apr 7, 2018

The ubuntu stress test was successful. Original reports for this test failing were on alpine and freebsd and not ubuntu. Running on freebsd:

https://ci.nodejs.org/job/node-stress-single-test/1805/

@lpinca
Copy link
Member Author

lpinca commented Apr 7, 2018

@Trott
Copy link
Member

Trott commented Apr 8, 2018

Stress tests are looking good. Which is contradictory to what I'm seeing locally... I wonder if I'm doing something wrong... Investigating....

@Trott
Copy link
Member

Trott commented Apr 8, 2018

OK, it definitely fails on OS X. I see that locally and here it is in CI: https://ci.nodejs.org/job/node-stress-single-test/1808/nodes=osx1010/console

@Trott
Copy link
Member

Trott commented Apr 8, 2018

Failing, although less frequently, on freebsd11-x64 too: https://ci.nodejs.org/job/node-stress-single-test/1810/nodes=freebsd11-x64/console

@lpinca
Copy link
Member Author

lpinca commented Apr 8, 2018

Ok, I guess that by the time res.end() is called the data chunk sent by the other peer has not been read yet.
I'll see if the test can be improved further. If not I'll close this PR.

Fix test-http-dump-req-when-res-ends and move it back to test/parallel/

Refs: nodejs#19823
@lpinca lpinca force-pushed the move/test-http-dump-req-when-res-ends branch from c38808f to d6c4cbe Compare April 8, 2018 10:30
@lpinca
Copy link
Member Author

lpinca commented Apr 8, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/14126/
Stress test macOS 10.10: https://ci.nodejs.org/job/node-stress-single-test/1811/

@Trott
Copy link
Member

Trott commented Apr 8, 2018

This version passes for me. Probably a good idea for @BridgeAR to review again since this now includes changes to the test and isn't simply a file move. @nodejs/testing @nodejs/http

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina mcollina mentioned this pull request Apr 9, 2018
3 tasks
@lpinca
Copy link
Member Author

lpinca commented Apr 10, 2018

@BridgeAR can you please update or remove your approval? Thanks.

@lpinca
Copy link
Member Author

lpinca commented Apr 11, 2018

Landed in 96e82be.

lpinca added a commit that referenced this pull request Apr 11, 2018
Fix test-http-dump-req-when-res-ends and move it back to test/parallel/

PR-URL: #19866
Refs: #19823
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@lpinca lpinca closed this Apr 11, 2018
@lpinca lpinca deleted the move/test-http-dump-req-when-res-ends branch April 11, 2018 07:32
@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 12, 2018
jasnell pushed a commit that referenced this pull request Apr 16, 2018
Fix test-http-dump-req-when-res-ends and move it back to test/parallel/

PR-URL: #19866
Refs: #19823
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@BridgeAR
Copy link
Member

BridgeAR commented May 1, 2018

I added the do not land labels as this is based on a semver-major

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants