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: check allowed port argument values #14232

Closed
wants to merge 1 commit into from
Closed

test: check allowed port argument values #14232

wants to merge 1 commit into from

Conversation

sam-github
Copy link
Contributor

Existing test only covered ports pass as options, add tests for
argument behaviour to avoid regressions.

Ref: #14205

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)

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. v4.x labels Jul 14, 2017
@sam-github sam-github mentioned this pull request Jul 14, 2017
Closed
4 tasks
@sam-github
Copy link
Contributor Author

@nodejs/lts @cjihrig PTAL, this is a regression test to prevent #14205, and the identical (modulo the es6-ization or the tests) test code is PRed against 6.x in #14234

@sam-github
test: check allowed port argument values
c8cf0ce 
Existing test only covered ports pass as options, add tests for
argument behaviour to avoid regressions.

Ref: nodejs#14205
@sam-github
Copy link
Contributor Author

Landed in c8cf0ce

@sam-github sam-github closed this Jul 17, 2017
@sam-github sam-github deleted the 14205-tests-for-4.x branch July 17, 2017 16:49
@MylesBorins
Copy link
Contributor

This should not have landed. Our LTS contract for v4.x maintenance does not cover additional tests like this.

Further the test was landed without the appropriate meta data.

I am backing this out of v4.x-staging.

@refack
Copy link
Contributor

refack commented Jul 17, 2017 •
edited
Loading

This should not have landed. Our LTS contract for v4.x maintenance does not cover additional tests like this.

Further the test was landed without the appropriate meta data.

I am backing this out of v4.x-staging.

A general inquiry regarding LTS maintenance phase, so @MylesBorins or @nodejs/release please answer at your leisure. Regarding the "LTS Contract" I'm assuming the best reference is https://github.com/nodejs/LTS/#lts-plan, which states:

Once a release moves into Maintenance mode, only ***critical*** bugs,
***critical*** security fixes, and documentation updates will be permitted.

So was backing this out just a "by the book" move?

@MylesBorins
Copy link
Contributor

Backing this out was indeed a "by the book" move. We had closed and not landed other changes to tests in the v4.x branch in the past

sam-github added a commit that referenced this pull request Jul 21, 2017
@sam-github
net: support passing undefined to listen()
0b17a45 
For consistency with 4.x and 8.x.

This commit also contains a forward port of
#14232 to confirm that 4.x and 6.x
behave identically with respect to the port argument.

PR-URL: #14234
Refs: #14205
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
@sam-github @MylesBorins
net: support passing undefined to listen()
ccc0ab7 
For consistency with 4.x and 8.x.

This commit also contains a forward port of
#14232 to confirm that 4.x and 6.x
behave identically with respect to the port argument.

PR-URL: #14234
Refs: #14205
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 1, 2017
@sam-github @MylesBorins
net: support passing undefined to listen()
b80aedf 
For consistency with 4.x and 8.x.

This commit also contains a forward port of
#14232 to confirm that 4.x and 6.x
behave identically with respect to the port argument.

PR-URL: #14234
Refs: #14205
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
sam-github added a commit to sam-github/node that referenced this pull request Sep 19, 2017
@sam-github
net: support passing undefined to listen()
e925c97 
For consistency with 4.x and 8.x.

This commit also contains a forward port of
nodejs#14232 to confirm that 4.x and 6.x
behave identically with respect to the port argument.

PR-URL: nodejs#14234
Refs: nodejs#14205
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 20, 2017
@sam-github @MylesBorins
net: support passing undefined to listen()
caeee38 
For consistency with 4.x and 8.x.

This commit also contains a forward port of
#14232 to confirm that 4.x and 6.x
behave identically with respect to the port argument.

Backport-PR-URL: #14234
PR-URL: #14234
Refs: #14205
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sam-github @MylesBorins @refack @jasnell @lpinca @cjihrig @tniessen @nodejs-github-bot