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: add http_incoming's matchKnownFields test #10811

Conversation

hiroppy
Copy link
Member

@hiroppy hiroppy commented Jan 14, 2017

Verify all cases.
Place: https://github.com/nodejs/node/blob/master/lib/_http_incoming.js#L136
Coverage: https://coverage.nodejs.org/coverage-57f6a106fbc69a47/root/_http_incoming.js.html

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

test

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Jan 14, 2017
@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Jan 14, 2017
@hiroppy hiroppy force-pushed the feature/create-http-incoming-matchKnownFields.js branch from e94e7a9 to 3392d40 Compare January 15, 2017 06:56
if (!value) dest = {};
else dest = {
[field]: 'test'
};
Copy link
Member

Choose a reason for hiding this comment

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

You could simplify this to:

const dest = {};
if (value)
  dest[field] = 'test';

[field]: 'test'
};
const incomingMessage = new IncomingMessage(field);
// dest is changed by Incomingmessage._addHeaderLine
Copy link
Member

Choose a reason for hiding this comment

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

Typo: s/Incomingmessage/IncomingMessage

@santigimeno
Copy link
Member

@hiroppy hiroppy force-pushed the feature/create-http-incoming-matchKnownFields.js branch from 3392d40 to fe0f0c2 Compare January 15, 2017 09:00
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

jasnell pushed a commit that referenced this pull request Jan 16, 2017
https://github.com/nodejs/node/blob/master/lib/_http_incoming.js#L136
Coverage: https://coverage.nodejs.org/coverage-57f6a106fbc69a47/root/_http_incoming.js.html

PR-URL: #10811
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@jasnell
Copy link
Member

jasnell commented Jan 16, 2017

Landed in dd629ba

@jasnell jasnell closed this Jan 16, 2017
@hiroppy hiroppy deleted the feature/create-http-incoming-matchKnownFields.js branch January 17, 2017 13:50
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
https://github.com/nodejs/node/blob/master/lib/_http_incoming.js#L136
Coverage: https://coverage.nodejs.org/coverage-57f6a106fbc69a47/root/_http_incoming.js.html

PR-URL: nodejs#10811
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017
https://github.com/nodejs/node/blob/master/lib/_http_incoming.js#L136
Coverage: https://coverage.nodejs.org/coverage-57f6a106fbc69a47/root/_http_incoming.js.html

PR-URL: nodejs#10811
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
https://github.com/nodejs/node/blob/master/lib/_http_incoming.js#L136
Coverage: https://coverage.nodejs.org/coverage-57f6a106fbc69a47/root/_http_incoming.js.html

PR-URL: nodejs#10811
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
https://github.com/nodejs/node/blob/master/lib/_http_incoming.js#L136
Coverage: https://coverage.nodejs.org/coverage-57f6a106fbc69a47/root/_http_incoming.js.html

PR-URL: nodejs#10811
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
https://github.com/nodejs/node/blob/master/lib/_http_incoming.js#L136
Coverage: https://coverage.nodejs.org/coverage-57f6a106fbc69a47/root/_http_incoming.js.html

PR-URL: #10811
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@MylesBorins
Copy link
Contributor

This test is failing on v4.x

MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
https://github.com/nodejs/node/blob/master/lib/_http_incoming.js#L136
Coverage: https://coverage.nodejs.org/coverage-57f6a106fbc69a47/root/_http_incoming.js.html

PR-URL: #10811
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 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.

7 participants