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.

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

test: fix running child-process-uid-gid as root #8864

Closed
wants to merge 1 commit into from

Conversation

geek
Copy link
Member

@geek geek commented Sep 30, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

test-child-process-uid-gid fails when run as root. Seeing that other tests are skipped when running as root, adding the same behavior to this test as well.

Replaces #8794

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 30, 2016
@addaleax
Copy link
Member

LGTM

For whoever lands this: This change is identical to the one in #8794 which already received several approvals.

@addaleax addaleax added the child_process Issues and PRs related to the child_process subsystem. label Sep 30, 2016
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.

Carry over LGTM

@targos
Copy link
Member

targos commented Sep 30, 2016

I'm sorry I didn't see the original PR, but I wonder if it wouldn't be better to change the UID when it's 0 instead of skipping the test ?

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell
Copy link
Member

jasnell commented Sep 30, 2016

I'd prefer either skipping or failing than changing... changing the UID would mask the problem a bit if the test does happen to be run as root (it would pass when under tested conditions it typically would not)

Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

geek added a commit to geek/node that referenced this pull request Oct 3, 2016
This skips the child-test-uid-gid test when run as root.
Previously, the test failed if executed as root.

PR-URL: nodejs#8864
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@cjihrig
Copy link
Contributor

cjihrig commented Oct 3, 2016

CI failures look unrelated.

geek added a commit that referenced this pull request Oct 3, 2016
This skips the child-test-uid-gid test when run as root.
Previously, the test failed if executed as root.

PR-URL: #8864
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@geek
Copy link
Member Author

geek commented Oct 3, 2016

Landed in 8dc2b42

@geek geek closed this Oct 3, 2016
@geek geek deleted the test-child-process-uid branch October 3, 2016 14:24
@joaocgreis
Copy link
Member

Was there a CI run for this change? This test failed on all Windows configurations on the last daily run, can it be related to this?

targos added a commit that referenced this pull request Oct 4, 2016
The process.getuid method does not exist on this platform.

Ref: #8864
@targos
Copy link
Member

targos commented Oct 4, 2016

Was there a CI run for this change?

There was one in the original PR and it apparently failed on Windows.

Fix here: #8924

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 4, 2016
The process.getuid method does not exist on this platform.

Ref: nodejs#8864
PR-URL: nodejs#8924
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 6, 2016
This skips the child-test-uid-gid test when run as root.
Previously, the test failed if executed as root.

PR-URL: #8864
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
jasnell pushed a commit that referenced this pull request Oct 6, 2016
The process.getuid method does not exist on this platform.

Ref: #8864
PR-URL: #8924
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
This skips the child-test-uid-gid test when run as root.
Previously, the test failed if executed as root.

PR-URL: #8864
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
The process.getuid method does not exist on this platform.

Ref: #8864
PR-URL: #8924
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants