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: add fix so that test exits if port 42 is unprivileged #45904

Merged
merged 6 commits into from
Jan 18, 2023
Merged

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

merged 6 commits into from
Jan 18, 2023

Conversation

7suyash7
Copy link
Contributor

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

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

Okay, I'll make these changes

@7suyash7
Copy link
Contributor Author

@bnoordhuis is this fine?

@7suyash7
Copy link
Contributor Author

added these changes @bnoordhuis

@7suyash7
Copy link
Contributor Author

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

@7suyash7
Copy link
Contributor Author

Can you please take a look?

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

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

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

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

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

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

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