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: remove common.isOSXMojave #25658

Closed
wants to merge 1 commit into from
Closed

test: remove common.isOSXMojave #25658

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 23, 2019

common.isOSXMojave was added because it was believed that there was a
bug in macOS Mojave that allowed unprivileged users to bind to
privileged ports. As it turns out, that was a feature not a bug. It is
likely to be in all future versions of macOS. Remove isOSXMojave and
skip appropriate tests based on isOSX.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@Trott
test: remove common.isOSXMojave
6b7431e 
common.isOSXMojave was added because it was believed that there was a
bug in macOS Mojave that allowed unprivileged users to bind to
privileged ports. As it turns out, that was a feature not a bug. It is
likely to be in all future versions of macOS. Remove isOSXMojave and
skip appropriate tests based on isOSX.
@nodejs-github-bot
Copy link
Collaborator

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2353/pipeline

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 23, 2019
@Trott
Copy link
Member Author

Trott commented Jan 23, 2019

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

@refack
Copy link
Contributor

refack commented Jan 23, 2019

Is there a reference we can link too?

cjihrig
cjihrig approved these changes Jan 23, 2019
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM, but it looks like it overlaps with #25649.

@Trott
Copy link
Member Author

Trott commented Jan 23, 2019

Is there a reference we can link too?

Refs: https://news.ycombinator.com/item?id=18302380

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 23, 2019
@Trott
Copy link
Member Author

Trott commented Jan 25, 2019

Landed in 399ef4f

@Trott Trott closed this Jan 25, 2019
Trott added a commit to Trott/io.js that referenced this pull request Jan 25, 2019
@Trott
test: remove common.isOSXMojave
399ef4f 
common.isOSXMojave was added because it was believed that there was a
bug in macOS Mojave that allowed unprivileged users to bind to
privileged ports. As it turns out, that was a feature not a bug. It is
likely to be in all future versions of macOS. Remove isOSXMojave and
skip appropriate tests based on isOSX.

Refs: https://news.ycombinator.com/item?id=18302380
PR-URL: nodejs#25658
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
addaleax pushed a commit that referenced this pull request Jan 28, 2019
@Trott @addaleax
test: remove common.isOSXMojave
1905f8e 
common.isOSXMojave was added because it was believed that there was a
bug in macOS Mojave that allowed unprivileged users to bind to
privileged ports. As it turns out, that was a feature not a bug. It is
likely to be in all future versions of macOS. Remove isOSXMojave and
skip appropriate tests based on isOSX.

Refs: https://news.ycombinator.com/item?id=18302380
PR-URL: #25658
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@targos targos mentioned this pull request Jan 29, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Apr 29, 2019
@Trott @BethGriggs
test: remove common.isOSXMojave
1cc771e 
common.isOSXMojave was added because it was believed that there was a
bug in macOS Mojave that allowed unprivileged users to bind to
privileged ports. As it turns out, that was a feature not a bug. It is
likely to be in all future versions of macOS. Remove isOSXMojave and
skip appropriate tests based on isOSX.

Refs: https://news.ycombinator.com/item?id=18302380
PR-URL: #25658
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Merged
BethGriggs pushed a commit that referenced this pull request May 10, 2019
@Trott @BethGriggs
test: remove common.isOSXMojave
13ee984 
common.isOSXMojave was added because it was believed that there was a
bug in macOS Mojave that allowed unprivileged users to bind to
privileged ports. As it turns out, that was a feature not a bug. It is
likely to be in all future versions of macOS. Remove isOSXMojave and
skip appropriate tests based on isOSX.

Refs: https://news.ycombinator.com/item?id=18302380
PR-URL: #25658
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@Trott @MylesBorins
test: remove common.isOSXMojave
52d321d 
common.isOSXMojave was added because it was believed that there was a
bug in macOS Mojave that allowed unprivileged users to bind to
privileged ports. As it turns out, that was a feature not a bug. It is
likely to be in all future versions of macOS. Remove isOSXMojave and
skip appropriate tests based on isOSX.

Refs: https://news.ycombinator.com/item?id=18302380
PR-URL: #25658
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
@Trott Trott deleted the rm-isosxmojave branch January 13, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BethGriggs BethGriggs BethGriggs approved these changes

@devsnek devsnek devsnek approved these changes

@JungMinu JungMinu JungMinu approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Trott @nodejs-github-bot @refack @benjamingr @lpinca @cjihrig @JungMinu @devsnek @BethGriggs