Skip to content

test: add fix so that test exits if port 42 is unprivileged#45904

Merged
nodejs-github-bot merged 6 commits intonodejs:mainfrom
7suyash7:test
Jan 18, 2023
Merged

test: add fix so that test exits if port 42 is unprivileged#45904
nodejs-github-bot merged 6 commits intonodejs:mainfrom
7suyash7:test

Conversation

@7suyash7
Copy link
Contributor

@7suyash7 7suyash7 commented Dec 18, 2022

fixes: #45838

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Dec 18, 2022
@bnoordhuis
Copy link
Member

bnoordhuis commented Dec 18, 2022

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.

@7suyash7
Copy link
Contributor Author

7suyash7 commented Dec 19, 2022

Okay, I'll make these changes

@7suyash7
Copy link
Contributor Author

7suyash7 commented Dec 19, 2022

@bnoordhuis is this fine?

bnoordhuis
bnoordhuis reviewed Dec 19, 2022
View reviewed changes
@7suyash7
Copy link
Contributor Author

7suyash7 commented Dec 19, 2022

added these changes @bnoordhuis

@7suyash7
Copy link
Contributor Author

7suyash7 commented Dec 21, 2022

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

bnoordhuis
bnoordhuis reviewed Dec 21, 2022
View reviewed changes
@7suyash7
Copy link
Contributor Author

7suyash7 commented Dec 21, 2022

Can you please take a look?

bnoordhuis
bnoordhuis reviewed Dec 22, 2022
View reviewed changes
benjamingr
benjamingr reviewed Dec 22, 2022
View reviewed changes
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();
Copy link
Member

@benjamingr benjamingr Dec 22, 2022

Choose a reason for hiding this comment

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

Add a comment?

Copy link
Contributor Author

@7suyash7 7suyash7 Dec 22, 2022

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...)

Copy link
Member

@benjamingr benjamingr Dec 22, 2022

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.

Copy link
Contributor Author

@7suyash7 7suyash7 Dec 24, 2022

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!

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 22, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 22, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 22, 2022

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

@lpinca lpinca added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jan 10, 2023
@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 18, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 18, 2023
@nodejs-github-bot nodejs-github-bot merged commit 60cc1ba into nodejs:main Jan 18, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 18, 2023

Landed in 60cc1ba

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 <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
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 <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
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 <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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

Assignees

No one assigned

Labels

commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

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

5 participants

@7suyash7 @bnoordhuis @nodejs-github-bot @benjamingr @lpinca