test: add test for uid/gid setting in spawn#7084
Closed
+14 −60
Conversation
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
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 added child_process 
MemberAuthor
Trott commented May 31, 2016
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.
Contributor
cjihrig commented Jun 2, 2016
There is #7049 about spawning |
MemberAuthor
Trott commented Jun 2, 2016
MemberAuthor
Trott commented Jun 3, 2016
CI is green. Anyone feel good enough about this to give a |
MemberAuthor
Trott commented Jun 3, 2016
@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. |
Contributor
cjihrig commented Jun 3, 2016
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?
Member
santigimeno commented Jun 3, 2016
LGTM with one nit you can ignore. |
Member
bnoordhuis commented Jun 3, 2016
LGTM with a suggestion. |
Member
jasnell commented Jun 3, 2016
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 <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
MemberAuthor
Trott commented Jun 3, 2016
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 <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
Merged
Contributor
MylesBorins commented Jul 11, 2016
@Trott lts? |
MemberAuthor
Trott commented Jul 11, 2016
@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 <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
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.