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: remove the use of curl in the test suite #5750

Closed
wants to merge 1 commit into from

Conversation

santigimeno
Copy link
Member

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    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 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 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 [email protected], and it was not fixed until
[email protected]. A modified version of this test has been run with
[email protected] and reproduces the problem. This same test has been run
with [email protected] and runs correctly.

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 `[email protected]`, and it was not
fixed until `[email protected]`. A modified version of this test has been run
with `[email protected]` and reproduces the problem. This same test has been
run with `[email protected]` and runs correctly.
@mscdex mscdex added test Issues and PRs related to the tests. http Issues or PRs related to the http subsystem. labels Mar 16, 2016
@santigimeno
Copy link
Member Author

It tries to fix #5174.
In case someone wants to test the new test with [email protected] and [email protected], I have setup a Dockerfile with wheezy (It installs initially [email protected]. [email protected] can be installed by running: apt-get install nodejs=0.2.6-5) and the test modified so it runs with [email protected] here: https://gist.github.com/santigimeno/4b534c2c2f77f3d90b29

@rvagg
Copy link
Member

rvagg commented Mar 17, 2016

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?

@bnoordhuis
Copy link
Member

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.

@jbergstroem
Copy link
Member

LGTM me too. If the curl test was specific to curl and not node it shouldn't have belong in our test suite anyway.

@Trott
Copy link
Member

Trott commented Mar 23, 2016

LGTM

@Trott
Copy link
Member

Trott commented Mar 23, 2016

Fishrock123 pushed a commit that referenced this pull request Mar 29, 2016
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 `[email protected]`, and it was not
fixed until `[email protected]`. A modified version of this test has been run
with `[email protected]` and reproduces the problem. This same test has been
run with `[email protected]` and runs correctly.

Fixes: #5174
PR-URL: #5750
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@Fishrock123
Copy link
Contributor

Thanks, landed in 82fdaae!


/Users/Jeremiah/Documents/node/.git/rebase-apply/patch:133: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

@santigimeno could you please set git config --global --add core.whitespace fix? Thanks! :D

@santigimeno
Copy link
Member Author

@santigimeno could you please set git config --global --add core.whitespace fix? Thanks! :D

@Fishrock123 will do. Thanks

MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
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 `[email protected]`, and it was not
fixed until `[email protected]`. A modified version of this test has been run
with `[email protected]` and reproduces the problem. This same test has been
run with `[email protected]` and runs correctly.

Fixes: #5174
PR-URL: #5750
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
evanlucas pushed a commit that referenced this pull request Mar 30, 2016
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 `[email protected]`, and it was not
fixed until `[email protected]`. A modified version of this test has been run
with `[email protected]` and reproduces the problem. This same test has been
run with `[email protected]` and runs correctly.

Fixes: #5174
PR-URL: #5750
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
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 `[email protected]`, and it was not
fixed until `[email protected]`. A modified version of this test has been run
with `[email protected]` and reproduces the problem. This same test has been
run with `[email protected]` and runs correctly.

Fixes: #5174
PR-URL: #5750
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
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 `[email protected]`, and it was not
fixed until `[email protected]`. A modified version of this test has been run
with `[email protected]` and reproduces the problem. This same test has been
run with `[email protected]` and runs correctly.

Fixes: #5174
PR-URL: #5750
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
@nodejs nodejs deleted a comment Sep 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants