Uh oh!
There was an error while loading. Please reload this page.
test: add test for uid/gid setting in spawn - #7084
Closed
Trott wants to merge 4 commits into
Closed
Conversation
Remove a disabled test in favor of one that expects an error. This validates (somewhat) that the underlying code is calling the correct system call for setting UID and GID. Unlike the formerly disabled test, it does not try to validate that the system UID/GID setting works.
Trott
commented
May 31, 2016
MemberAuthor
Windows might have a surprise on this, so running CI immediately: https://ci.nodejs.org/job/node-test-pull-request/2882/ |
| assert.throws(() => { | ||
| spawn('echo', ['fhqwhgads'], {gid: 0}); | ||
| }, /EPERM/, 'Setting GID should throw EPERM for unprivileged users.'); |
Member
There was a problem hiding this comment.
I'm reasonably sure these are going to fail with ENOTSUP on Windows.
MemberAuthor
There was a problem hiding this comment.
I'm reasonably sure these are going to fail with ENOTSUP on Windows.
Will add a check for common.isWindows and alter the expected error appropriately.
cjihrig
commented
Jun 2, 2016
Contributor
There is #7049 about spawning |
Trott
commented
Jun 2, 2016
MemberAuthor
Trott
commented
Jun 3, 2016
MemberAuthor
CI is green. Anyone feel good enough about this to give a |
Trott
commented
Jun 3, 2016
MemberAuthor
@cjihrig OK to leave this one as is and wait for the common helper function? Since the command doesn't actually ever get run--spawn throws in this test--it doesn't actually affect the test results. |
cjihrig
commented
Jun 3, 2016
Contributor
Yea, LGTM |
| assert.throws(() => { | ||
| spawn('echo', ['fhqwhgads'], {gid: 0}); | ||
| }, expectedError, 'Setting GID should throw EPERM for unprivileged users.'); |
Member
There was a problem hiding this comment.
Maybe changing the assert message depending if it's windows or not?
santigimeno
commented
Jun 3, 2016
Member
LGTM with one nit you can ignore. |
bnoordhuis
commented
Jun 3, 2016
Member
LGTM with a suggestion. |
jasnell
commented
Jun 3, 2016
Member
LGTM |
Trott added a commit
to Trott/io.js
that referenced
this pull request
Jun 3, 2016
Remove a disabled test in favor of one that expects an error. This validates (somewhat) that the underlying code is calling the correct system call for setting UID and GID. Unlike the formerly disabled test, it does not try to validate that the system UID/GID setting works. PR-URL: nodejs#7084 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
Trott
commented
Jun 3, 2016
MemberAuthor
Landed in d015169 |
evanlucas pushed a commit
that referenced
this pull request
Jun 15, 2016
Remove a disabled test in favor of one that expects an error. This validates (somewhat) that the underlying code is calling the correct system call for setting UID and GID. Unlike the formerly disabled test, it does not try to validate that the system UID/GID setting works. PR-URL: #7084 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
Merged
MylesBorins
commented
Jul 11, 2016
Contributor
@Trott lts? |
Trott
commented
Jul 11, 2016
MemberAuthor
@thealphanerd If it lands cleanly, yes. |
MylesBorins pushed a commit
that referenced
this pull request
Jul 11, 2016
Remove a disabled test in favor of one that expects an error. This validates (somewhat) that the underlying code is calling the correct system call for setting UID and GID. Unlike the formerly disabled test, it does not try to validate that the system UID/GID setting works. PR-URL: #7084 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit
that referenced
this pull request
Jul 12, 2016
Remove a disabled test in favor of one that expects an error. This validates (somewhat) that the underlying code is calling the correct system call for setting UID and GID. Unlike the formerly disabled test, it does not try to validate that the system UID/GID setting works. PR-URL: #7084 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit
that referenced
this pull request
Jul 12, 2016
Remove a disabled test in favor of one that expects an error. This validates (somewhat) that the underlying code is calling the correct system call for setting UID and GID. Unlike the formerly disabled test, it does not try to validate that the system UID/GID setting works. PR-URL: #7084 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
Merged
MylesBorins pushed a commit
that referenced
this pull request
Jul 14, 2016
Remove a disabled test in favor of one that expects an error. This validates (somewhat) that the underlying code is calling the correct system call for setting UID and GID. Unlike the formerly disabled test, it does not try to validate that the system UID/GID setting works. PR-URL: #7084 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit
that referenced
this pull request
Jul 14, 2016
Remove a disabled test in favor of one that expects an error. This validates (somewhat) that the underlying code is calling the correct system call for setting UID and GID. Unlike the formerly disabled test, it does not try to validate that the system UID/GID setting works. PR-URL: #7084 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
Checklist
Affected core subsystem(s)
test child_process
Description of change
Remove a disabled test in favor of one that expects an error.
This validates (somewhat) that the underlying code is calling the
correct system call for setting UID and GID. Unlike the formerly
disabled test, it does not try to validate that the system UID/GID
setting works.