GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: remove common.PORT from http tests by Trott · Pull Request #7467 · nodejs/node

Conversation

@Trott

@Trott Trott commented Jun 29, 2016

Copy link
Copy Markdown
Member
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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Trott Trott added http Issues and PRs related to the http subsystem. test Issues and PRs related to Node.js core tests and test infrastructure. labels Jun 29, 2016
@addaleax

addaleax commented Jun 29, 2016

Copy link
Copy Markdown
Member

LGTM pending CI
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex

mscdex commented Jun 29, 2016

Copy link
Copy Markdown
Contributor

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fishrock123
Fishrock123 reviewed Jun 29, 2016
View reviewed changes
Comment thread test/parallel/test-http-client-keep-alive-release-before-finish.js Outdated

@Fishrock123 Fishrock123 Jun 29, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

can just be port,

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@bnoordhuis bnoordhuis Jun 29, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Fishrock123 Fishrock123 Jun 29, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lance lance Jun 29, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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'
});

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bnoordhuis

bnoordhuis commented Jun 29, 2016

Copy link
Copy Markdown
Member

LGTM. Maybe enforce with a lint rule?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@santigimeno

santigimeno commented Jun 29, 2016

Copy link
Copy Markdown
Member

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
1 similar comment
@cjihrig

cjihrig commented Jun 29, 2016

Copy link
Copy Markdown
Contributor

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott added 2 commits June 29, 2016 09:34
@Trott
test: remove common.PORT from http tests
515db4c
@Trott
squash: object shorthand nits
a3ff272
@Trott
Trott force-pushed the port-starboard branch from d6570f4 to a3ff272 Compare June 29, 2016 16:37
@Trott

Trott commented Jun 29, 2016

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Jun 29, 2016

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Jun 29, 2016

Copy link
Copy Markdown
Member

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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 <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott

Trott commented Jul 1, 2016

Copy link
Copy Markdown
Member Author

Landed in abce60c
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
Propose v6.3.0 (v2) #7550
Merged
@MylesBorins MylesBorins mentioned this pull request Jul 11, 2016
Audit commits not found on v4.x-staging #7661
Closed
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@Trott Myles Borins
test: remove common.PORT from http tests
11fcea4 
PR-URL: #7467
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins added the land-on-v4.x label Jul 12, 2016
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 <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@Trott Myles Borins
test: remove common.PORT from http tests
cf90bab 
PR-URL: #7467
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@Trott Myles Borins
test: remove common.PORT from http tests
78bbbf9 
PR-URL: #7467
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
v4.5.0 proposal #7688
Merged
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@Trott Myles Borins
test: remove common.PORT from http tests
fb73122 
PR-URL: #7467
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
@Trott Myles Borins
test: remove common.PORT from http tests
8273824 
PR-URL: #7467
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Colin Ihrig <[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

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

http Issues and PRs related to the http subsystem. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@Trott @addaleax @mscdex @bnoordhuis @santigimeno @cjihrig @jasnell @lance @Fishrock123 @MylesBorins
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.