-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: remove the use of curl in the test suite #5750
Conversation
There were 2 tests using curl: `test-http-304.js` is removed because it was initially included to test that the 304 response does not contain a body, and this is already covered by `test-http-chunked-304.js`. `test-http-curl-chunk-problem` has been renamed and refactored so instead of using curl, it uses 2 child node processes: one for sending the HTTP request and the other to calculate the sha1sum. Originally, this test was introduced to fix a bug in `nodejs@0.2.x`, and it was not fixed until `nodejs@0.2.5`. A modified version of this test has been run with `nodejs@0.2.0` and reproduces the problem. This same test has been run with `nodejs@0.2.6` and runs correctly.
It tries to fix #5174. |
I'm a little hesitant to move forward with removing the explicit curl test from test-http-curl-chunk-problem but if what you say is correct that you can reproduce the problem with this new test case then (1) impressive work to go back so far and (2) I guess that's a positive sign that this is OK to move ahead, (although not definitive). /cc @jbergstroem @nodejs/testing thoughts? |
LGTM. I'm fine with renaming test-http-curl-chunk-problem.js; it's poorly named because the real issue wasn't with curl but with node. |
LGTM me too. If the curl test was specific to curl and not node it shouldn't have belong in our test suite anyway. |
LGTM |
There were 2 tests using curl: `test-http-304.js` is removed because it was initially included to test that the 304 response does not contain a body, and this is already covered by `test-http-chunked-304.js`. `test-http-curl-chunk-problem` has been renamed and refactored so instead of using curl, it uses 2 child node processes: one for sending the HTTP request and the other to calculate the sha1sum. Originally, this test was introduced to fix a bug in `nodejs@0.2.x`, and it was not fixed until `nodejs@0.2.5`. A modified version of this test has been run with `nodejs@0.2.0` and reproduces the problem. This same test has been run with `nodejs@0.2.6` and runs correctly. Fixes: #5174 PR-URL: #5750 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Thanks, landed in 82fdaae!
@santigimeno could you please set |
@Fishrock123 will do. Thanks |
There were 2 tests using curl: `test-http-304.js` is removed because it was initially included to test that the 304 response does not contain a body, and this is already covered by `test-http-chunked-304.js`. `test-http-curl-chunk-problem` has been renamed and refactored so instead of using curl, it uses 2 child node processes: one for sending the HTTP request and the other to calculate the sha1sum. Originally, this test was introduced to fix a bug in `nodejs@0.2.x`, and it was not fixed until `nodejs@0.2.5`. A modified version of this test has been run with `nodejs@0.2.0` and reproduces the problem. This same test has been run with `nodejs@0.2.6` and runs correctly. Fixes: #5174 PR-URL: #5750 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
There were 2 tests using curl: `test-http-304.js` is removed because it was initially included to test that the 304 response does not contain a body, and this is already covered by `test-http-chunked-304.js`. `test-http-curl-chunk-problem` has been renamed and refactored so instead of using curl, it uses 2 child node processes: one for sending the HTTP request and the other to calculate the sha1sum. Originally, this test was introduced to fix a bug in `nodejs@0.2.x`, and it was not fixed until `nodejs@0.2.5`. A modified version of this test has been run with `nodejs@0.2.0` and reproduces the problem. This same test has been run with `nodejs@0.2.6` and runs correctly. Fixes: #5174 PR-URL: #5750 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
There were 2 tests using curl: `test-http-304.js` is removed because it was initially included to test that the 304 response does not contain a body, and this is already covered by `test-http-chunked-304.js`. `test-http-curl-chunk-problem` has been renamed and refactored so instead of using curl, it uses 2 child node processes: one for sending the HTTP request and the other to calculate the sha1sum. Originally, this test was introduced to fix a bug in `nodejs@0.2.x`, and it was not fixed until `nodejs@0.2.5`. A modified version of this test has been run with `nodejs@0.2.0` and reproduces the problem. This same test has been run with `nodejs@0.2.6` and runs correctly. Fixes: #5174 PR-URL: #5750 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
There were 2 tests using curl: `test-http-304.js` is removed because it was initially included to test that the 304 response does not contain a body, and this is already covered by `test-http-chunked-304.js`. `test-http-curl-chunk-problem` has been renamed and refactored so instead of using curl, it uses 2 child node processes: one for sending the HTTP request and the other to calculate the sha1sum. Originally, this test was introduced to fix a bug in `nodejs@0.2.x`, and it was not fixed until `nodejs@0.2.5`. A modified version of this test has been run with `nodejs@0.2.0` and reproduces the problem. This same test has been run with `nodejs@0.2.6` and runs correctly. Fixes: #5174 PR-URL: #5750 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Pull Request check-list
Please make sure to review and check all of these items:
make -j8 test
(UNIX) orvcbuild test nosign
(Windows) pass withthis change (including linting)?
test (or a benchmark) included?
existing APIs, or introduces new ones)?
Affected core subsystem(s)
test
Description of change
There were 2 tests using curl:
test-http-304.js
is removed because it was initially included to testthat the 304 response does not contain a body, and this is already
covered by
test-http-chunked-304.js
.test-http-curl-chunk-problem
has been refactored so instead of usingcurl, it uses 2 child node processes: one for sending the HTTP request
and the other to calculate the sha1sum. Originally, this test was
introduced to fix a bug in
nodejs@0.2.x
, and it was not fixed untilnodejs@0.2.5
. A modified version of this test has been run withnodejs@0.2.0
and reproduces the problem. This same test has been runwith
nodejs@0.2.6
and runs correctly.