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 test for http outgoing internal headers #13980

Closed
wants to merge 1 commit into from
Closed

test: add test for http outgoing internal headers #13980

wants to merge 1 commit into from

Conversation

gergelyke
Copy link
Contributor

@gergelyke gergelyke commented Jun 29, 2017

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)

http

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 29, 2017
@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Jun 29, 2017

const outHeadersKey = require('internal/http').outHeadersKey;
const http = require('http');
const OutgoingMessage = http.OutgoingMessage;
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps just:

const { OutgoingMessage } = require('http');

@Trott
Copy link
Member

Trott commented Jun 30, 2017

The first word after the : in the commit message should be an imperative verb. So maybe something like test: add test for http outgoing internal headers?

This can be fixed by whoever lands the pull request, but if you do it for them, it will save them a few keystrokes.

See commit message guidelines at https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#commit-message-guidelines. (You probably saw them and were trying to follow them but there's a lot of stuff there so it's easy to miss a small thing here or there.)

@gergelyke
Copy link
Contributor Author

@Trott @jasnell thanks, fixed!

@gergelyke gergelyke changed the title test: http outgoing _headers setter/getter test: add test for http outgoing internal headers Jul 3, 2017
@gergelyke
Copy link
Contributor Author

is there anything else I should fix?

@Trott
Copy link
Member

Trott commented Jul 4, 2017

@nodejs/testing @nodejs/http

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

My only comment would be to separate the tests using block scopes. It prevents the tests from interacting with each other and eliminates numbers on the end of the variable names (outgoingMessage2).

@gergelyke
Copy link
Contributor Author

should be fixed @cjihrig

const common = require('../common');
const assert = require('assert');

const outHeadersKey = require('internal/http').outHeadersKey;
Copy link
Member

Choose a reason for hiding this comment

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

For consistency...

const { outHeadersKey } = require('internal/http');

host: ['host', 'risingstack.com'],
origin: ['Origin', 'localhost']
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

lint: file needs to end with \n

@refack
Copy link
Contributor

refack commented Jul 17, 2017

@jasnell
Copy link
Member

jasnell commented Jul 17, 2017

Looks like this needs a make lint check.

@gergelyke
Copy link
Contributor Author

should be all fixed :)

@refack
Copy link
Contributor

refack commented Jul 18, 2017

@gergelyke
Copy link
Contributor Author

@refack is it me who should fi things, or the CI has some issues?

@refack
Copy link
Contributor

refack commented Jul 18, 2017

@refack is it me who should fi things, or the CI has some issues?

@gergelyke CI had issues. AFAICT the jobs that did finish were green.
New CI anyway: https://ci.nodejs.org/job/node-test-commit/11221/

@refack refack self-assigned this Jul 18, 2017
refack pushed a commit to refack/node that referenced this pull request Jul 18, 2017
PR-URL: nodejs#13980
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@refack
Copy link
Contributor

refack commented Jul 18, 2017

Landed in f406a7e

@refack refack closed this Jul 18, 2017
@refack
Copy link
Contributor

refack commented Jul 18, 2017

Extra sanity test on master: https://ci.nodejs.org/job/node-test-commit-linuxone/7394/

@addaleax addaleax mentioned this pull request Jul 18, 2017
addaleax pushed a commit that referenced this pull request Jul 18, 2017
PR-URL: #13980
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
PR-URL: #13980
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@refack refack removed their assignment Oct 20, 2018
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