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: invalid chars in http client path #11964

Closed

Conversation

lucamaraschi
Copy link
Contributor

This test adds coverage for all the characters which are considered
invalid in a http path.

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

test http-client

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 21, 2017
const server = http.createServer((req, res) => {
res.end();
server.close();
}).listen(0, test);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add common.mustCall() around the callback.

@Fishrock123
Copy link
Contributor

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Mar 21, 2017
@mscdex
Copy link
Contributor

mscdex commented Mar 21, 2017

I think it would be better to try a path string for each control character instead of all of them together.

Also, I don't think we need to spin up a server just to check for the TypeError.

@jasnell
Copy link
Member

jasnell commented Mar 21, 2017

Definitely agree with @mscdex. Testing each individually would be more robust.

This test adds coverage for all the characters which are considered
invalid in a http path.
@lpinca
Copy link
Member

lpinca commented Mar 23, 2017

@jasnell
Copy link
Member

jasnell commented Mar 23, 2017

extremely minor nit: for the "experimentally" determined number, it may be useful to include a comment that describes the process you followed to find that number :-)

@lpinca
Copy link
Member

lpinca commented Mar 23, 2017

I guess the comment should be added here

if (path.length <= 39) { // Determined experimentally in V8 5.4
:)

jasnell pushed a commit that referenced this pull request Mar 24, 2017
This test adds coverage for all the characters which are considered
invalid in a http path.

PR-URL: #11964
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 24, 2017

Landed in 30f1e8e

@jasnell jasnell closed this Mar 24, 2017
MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
This test adds coverage for all the characters which are considered
invalid in a http path.

PR-URL: #11964
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 28, 2017
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
2 tasks
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
This test adds coverage for all the characters which are considered
invalid in a http path.

PR-URL: #11964
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
This test adds coverage for all the characters which are considered
invalid in a http path.

PR-URL: #11964
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
This test adds coverage for all the characters which are considered
invalid in a http path.

PR-URL: nodejs/node#11964
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[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

Successfully merging this pull request may close these issues.

8 participants