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

Avoid calling curl from tests #5174

Closed
jbergstroem opened this issue Feb 10, 2016 · 4 comments
Closed

Avoid calling curl from tests #5174

jbergstroem opened this issue Feb 10, 2016 · 4 comments
Labels
http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.

Comments

@jbergstroem
Copy link
Member

Adding so I don't forget -- we currently invoke curl in two tests which implicitly pulls it as a dependency:

$ ag "curl " test
test/parallel/test-http-304.js
13:  childProcess.exec('curl -i http://127.0.0.1:' + common.PORT + '/',

test/parallel/test-http-curl-chunk-problem.js
19:  console.log('making curl request');
20:  var cmd = 'curl http://127.0.0.1:' + common.PORT + '/ | ' +

Suggesting we replace this with.. javascript?

@jbergstroem jbergstroem added test Issues and PRs related to the tests. good first issue Issues that are suitable for first-time contributors. labels Feb 10, 2016
@rvagg
Copy link
Member

rvagg commented Feb 10, 2016

curl isn't the only unix util in the tests, there's sed, tail, grep, cat and probably more, which is why we have "git bash" as one of the recommended prerequisites for setting up a dev environment. curl does have the nice side effect of validating Node's HTTP server implementation. Having said that, I don't think I'd object to someone writing a replacement in common.js for it.

On the two tests in question:

  • what is test/parallel/test-http-304.js even testing? It's not clear to me that it's got anything to do with 304 other than "I can set a 304 status code and not kill my http server".
  • test/parallel/test-http-curl-chunk-problem.js is piping curl output to openssl-cli at the heart of the test and there's a link pointing to https://groups.google.com/forum/#!topic/nodejs/9mzTyWBAaRk which implicates curl as a problematic client for node (hence the name of the test I think). So maybe it's not such a good target for rewriting although it could have a bail-if-no-curl clause like we do with common.opensslCli.

@jbergstroem jbergstroem removed the good first issue Issues that are suitable for first-time contributors. label Feb 10, 2016
@jbergstroem
Copy link
Member Author

curl lives outside of coreutils or what for instance busybox would offer (it has the other examples you brought up) hence my suggestion. I didn't really look into this other than running into the 'missing dependency' issue; the plan was to have a look at it a bit later.

Looking into

  • test-http-304; pretty pointless; not even checking status code or an empty body.
  • test-http-curl-chunk-problem.js; author suggests that more clients has the same issue, but its pretty obvious that curl is what we're after.

I'm not particularly keen on growing the common.js-convention of hasThis/isThat, but I'd take a skip over fail any day.

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Feb 10, 2016
@orangemocha
Copy link
Contributor

cc @nodejs/testing

@MylesBorins
Copy link
Contributor

I'd be up for trying to get this done if we want to do it

MylesBorins pushed a commit that referenced this issue 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 issue 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 issue 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 issue 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]>
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

No branches or pull requests

5 participants