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.

Sign up for GitHub

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

Jump to bottom

test: remove common.PORT from http tests #7467

Closed
wants to merge 2 commits into from
Closed

test: remove common.PORT from http tests #7467

wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 29, 2016

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

test http

Description of change

/cc @mscdex @nodejs/testing

@Trott Trott added http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. labels Jun 29, 2016
@addaleax
Copy link
Member

LGTM pending CI

@mscdex
Copy link
Contributor

mscdex commented Jun 29, 2016

LGTM

Fishrock123
Fishrock123 reviewed Jun 29, 2016
View reviewed changes
test/parallel/test-http-client-keep-alive-release-before-finish.js
const post = http.request({
agent: agent,
method: 'POST',
port: common.PORT,
port: port,
Copy link
Contributor

@Fishrock123 Fishrock123 Jun 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

can just be port,

Copy link
Member

Choose a reason for hiding this comment

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

I'd say that's less readable if the surrounding lines are all name: value.

Copy link
Contributor

Choose a reason for hiding this comment

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

¯_(ツ)_/¯ we've already started moving to using value, elsewhere.

Copy link
Member

Choose a reason for hiding this comment

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

Same could be said for agent as well.

const post = http.request({
  agent,
  port,
  method: 'POST'
});

@bnoordhuis
Copy link
Member

LGTM. Maybe enforce with a lint rule?

@santigimeno
Copy link
Member

LGTM

1 similar comment
@cjihrig
Copy link
Contributor

cjihrig commented Jun 29, 2016

LGTM

@Trott
Copy link
Member Author

Trott commented Jun 29, 2016

Rebased, took care of the object shorthand nits, force pushed, CI: https://ci.nodejs.org/job/node-test-pull-request/3126/

@Trott
Copy link
Member Author

Trott commented Jun 29, 2016

Maybe enforce with a lint rule?

Definitely, although if I recall correctly, I think we'd want #7043 to make it possible to remove common.PORT from 20 or 30 cluster tests and #5025 to land first so we can remove it from a smaller number of debugger tests.

@jasnell
Copy link
Member

jasnell commented Jun 29, 2016

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Jul 1, 2016
@Trott
test: remove common.PORT from http tests
abce60c 
PR-URL: nodejs#7467
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Jul 1, 2016

Landed in abce60c

@Trott Trott closed this Jul 1, 2016
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
@Trott @Fishrock123
test: remove common.PORT from http tests
b383fdd 
PR-URL: #7467
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
Merged
@MylesBorins MylesBorins mentioned this pull request Jul 11, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@Trott
test: remove common.PORT from http tests
11fcea4 
PR-URL: #7467
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@Trott @MylesBorins
test: remove common.PORT from http tests
5542971 
PR-URL: #7467
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@Trott
test: remove common.PORT from http tests
cf90bab 
PR-URL: #7467
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@Trott
test: remove common.PORT from http tests
78bbbf9 
PR-URL: #7467
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@Trott
test: remove common.PORT from http tests
fb73122 
PR-URL: #7467
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@Trott
test: remove common.PORT from http tests
8273824 
PR-URL: #7467
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott deleted the port-starboard branch January 13, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@Trott @addaleax @mscdex @bnoordhuis @santigimeno @cjihrig @jasnell @lance @Fishrock123 @MylesBorins