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: add fix so that test exits if port 42 is unprivileged by 7suyash7 · Pull Request #45904 · nodejs/node

Conversation

@7suyash7

@7suyash7 7suyash7 commented Dec 18, 2022

Copy link
Copy Markdown
Contributor

fixes: #45838

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and benjamingr reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@7suyash7
test: add fix so that test exits if port 42 is unprivileged
cb00945
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure. labels Dec 18, 2022
@bnoordhuis

bnoordhuis commented Dec 18, 2022

Copy link
Copy Markdown
Member

The way the test is written now makes it possible for future regressions to go undetected so that's no good, unfortunately.

I'd simply call out to sysctl(1) with execSync() to check the value. There's at least one other instance of that in our test suite so it's not without precedent.
All reactions

Sorry, something went wrong.

Uh oh!

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

7suyash7 commented Dec 19, 2022

Copy link
Copy Markdown
Contributor Author

Okay, I'll make these changes
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@7suyash7
test: added execSync() call
df93981
@7suyash7

7suyash7 commented Dec 19, 2022

Copy link
Copy Markdown
Contributor Author

@bnoordhuis is this fine?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnoordhuis
bnoordhuis reviewed Dec 19, 2022
View reviewed changes
Comment thread test/parallel/test-cluster-bind-privileged-port.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/parallel/test-cluster-bind-privileged-port.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/parallel/test-cluster-bind-privileged-port.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@7suyash7
test: added suggested changes
2ba859a
@7suyash7

7suyash7 commented Dec 19, 2022

Copy link
Copy Markdown
Contributor Author

added these changes @bnoordhuis
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@7suyash7
test: fix js-lint errors
e9d8d6b
@7suyash7

7suyash7 commented Dec 21, 2022

Copy link
Copy Markdown
Contributor Author

Can you try merging again? Had some lint errors, hopefully, they are fixed now. @bnoordhuis
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnoordhuis
bnoordhuis reviewed Dec 21, 2022
View reviewed changes
Comment thread test/parallel/test-cluster-bind-privileged-port.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/parallel/test-cluster-bind-privileged-port.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/parallel/test-cluster-bind-privileged-port.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/parallel/test-cluster-bind-privileged-port.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@7suyash7
test: add suggested changes
2b31776
@7suyash7

7suyash7 commented Dec 21, 2022

Copy link
Copy Markdown
Contributor Author

Can you please take a look?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnoordhuis
bnoordhuis reviewed Dec 22, 2022
View reviewed changes
Comment thread test/parallel/test-cluster-bind-privileged-port.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@7suyash7
test: fix index for unprivilegedPort
c8d3d79
bnoordhuis
bnoordhuis approved these changes Dec 22, 2022
View reviewed changes
benjamingr
benjamingr reviewed Dec 22, 2022
View reviewed changes
Comment thread test/parallel/test-cluster-bind-privileged-port.js
const { execSync } = require('child_process');

if (common.isLinux) {
const sysctlOutput = execSync('sysctl net.ipv4.ip_unprivileged_port_start').toString();

@benjamingr benjamingr Dec 22, 2022

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.

Add a comment?

Sorry, something went wrong.

Uh oh!

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

@7suyash7 7suyash7 Dec 22, 2022

Copy link
Copy Markdown
Contributor Author

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.

Sure, should I add one now? (PR is closing to merging I think...)

Sorry, something went wrong.

Uh oh!

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

@benjamingr benjamingr Dec 22, 2022

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.

PR is closing to merging I think...

I am not sure what you mean here, you can still push changes and I'll re-run CI after :]

If you prefer to put it in another change to practice making changes in Node.js as a new contributor - that's also fine with me.

Sorry, something went wrong.

Uh oh!

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

@7suyash7 7suyash7 Dec 24, 2022

Copy link
Copy Markdown
Contributor Author

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 would like it if I could put it in another change so I can practice making changes, that would be really helpful!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
benjamingr
benjamingr approved these changes Dec 22, 2022
View reviewed changes
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 22, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 22, 2022
@nodejs-github-bot

nodejs-github-bot commented Dec 22, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/48695/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lpinca
lpinca approved these changes Jan 10, 2023
View reviewed changes
@lpinca lpinca added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Jan 10, 2023
@lpinca lpinca added the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 18, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 18, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 60cc1ba into nodejs:main Jan 18, 2023
@nodejs-github-bot

nodejs-github-bot commented Jan 18, 2023

Copy link
Copy Markdown
Collaborator

Landed in 60cc1ba
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
@7suyash7 @RafaelGSS
test: add fix so that test exits if port 42 is unprivileged
4f491d3 
PR-URL: #45904
Fixes: #45838
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
v19.5.0 proposal #46286
Merged
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
@7suyash7 @juanarbol
test: add fix so that test exits if port 42 is unprivileged
c1f2c8d 
PR-URL: #45904
Fixes: #45838
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
V18.14.0 proposal #46396
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
@7suyash7 @juanarbol
test: add fix so that test exits if port 42 is unprivileged
896027c 
PR-URL: #45904
Fixes: #45838
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes
@bnoordhuis bnoordhuis bnoordhuis approved these changes
@lpinca lpinca lpinca approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

commit-queue-squash PRs the Commit Queue should land as one squashed commit. needs-ci PRs that need a full CI run. 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.

test: check net.ipv4.ip_unprivileged_port_start in parallel/test-cluster-bind-privileged-port

Uh oh!

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

5 participants

@7suyash7 @bnoordhuis @nodejs-github-bot @benjamingr @lpinca
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.