test: enable setuid/setgid test#12403
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Trott added process 
Trott commented Apr 13, 2017
Will be interested to see how this goes on CI.... |
Trott commented Apr 13, 2017
Fixed up a bit. Once again, CI: https://ci.nodejs.org/job/node-test-pull-request/7385/ |
Trott commented Apr 13, 2017
Fixed it up so it isn't skipped anymore. Would be nice if there was a way to specify tests that need superuser, but I guess we shouldn't expect devs to have local superuser necessarily. (Would be nice for CI, though!) New CI: https://ci.nodejs.org/job/node-test-pull-request/7387/ |
Trott commented Apr 13, 2017
Fixed up more to be forgiving if |
Trott commented Apr 13, 2017
SmartOS is letting me know that different OS's can have somewhat different error messages. Made the check a little more lenient. |
cjihrig left a comment
cjihrig left a comment There was a problem hiding this comment.
LGTM once the CI is straightened out.
Refactor test for situations where it was expected to fail. Move from disabled directory to parallel.
Trott commented Apr 14, 2017
Yet more lenient. |
Trott commented Apr 14, 2017
Single ARM failure is infrastructure-related. (That instance that failed doesn't even run this test.) CI is effectively green. |
| assert.throws( | ||
| ()=>{process.setgid('nobody');}, | ||
| /^Error:(EPERM,.+|setgidgroupiddoesnotexist)$/ |
There was a problem hiding this comment.
I am actually getting Error: EPERM, Operation not permitted for both the cases. Also, the capture group doesn't look correct to me.
There was a problem hiding this comment.
I am actually getting Error: EPERM, Operation not permitted for both the cases.
@thefourtheye Error message depends on your operating system. SmartOS gives Error: EPERM, Not owner here.
The regexp allows any error that starts with Error: EPERM, and also allows Error: setgid group id does not exist for the situation where nobody is not included on the operating system (which happens on ubuntu1404-64 in FIPS mode on our CI).
TL;DR: Test passes if you have group nobody and get EPERM back along with some text from the OS, and it will also pass if group nobody doesn't exist at all.
There was a problem hiding this comment.
Also, the capture group doesn't look correct to me.
@thefourtheye There's room for improvement. For one thing, it could be a non-capturing group. But the regexp seems to work to me. What situation are you concerned about?
> /^Error: (EPERM, .+|setgid group id does not exist)$/.test('Error: setgid group id does not exist')true > /^Error: (EPERM, .+|setgid group id does not exist)$/.test('Error: setgid group id does not exist adding some text so it will be false')false > /^Error: (EPERM, .+|setgid group id does not exist)$/.test('Error: EPERM, setgid group id does not exist adding some text so it will be false except now it as EPERM at the start so it should be true again')true > There was a problem hiding this comment.
Oh I thought EPERM is always going to be there. As "setgid group id does not exist" is present in all the cases, can we make EPERM, optional and check for the presence of "setgid group id does not exist"?
There was a problem hiding this comment.
The only thing that's there in all cases is Error: .
> /^Error: (EPERM, .+|setgid group id does not exist)$/.test('Error: EPERM, arbitrary text.')true >The regexp can be thought of as: String must start with Error: and then there are two options that will match after that. One is EPERM, followed by one or more characters of anything. The other is the string setgid group id does not exist precisely.
Refactor test for situations where it was expected to fail. Move from disabled directory to parallel. PR-URL: nodejs#12403 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
Trott commented Apr 16, 2017
Landed in 189afc8 |
Refactor test for situations where it was expected to fail. Move from disabled directory to parallel. PR-URL: #12403 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
Refactor test for situations where it was expected to fail. Move from disabled directory to parallel. PR-URL: #12403 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
Refactor test for situations where it was expected to fail. Move from disabled directory to parallel. PR-URL: #12403 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
gibfahn commented May 15, 2017
@Trott should this be backported to v6.x? |
Trott commented May 16, 2017
gibfahn commented May 16, 2017
Didn't backport cleanly, could you raise a PR? |
Trott commented May 16, 2017
Refactor test for situations where it was expected to fail. Move from disabled directory to parallel. PR-URL: nodejs#12403 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
Refactor test for situations where it was expected to fail. Move from disabled directory to parallel. PR-URL: nodejs#12403 Backport-PR-URL: nodejs#13060 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
Refactor test for situations where it was expected to fail. Move from disabled directory to parallel. PR-URL: #12403 Backport-PR-URL: #13060 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
Refactor test for situations where it was expected to fail. Move from disabled directory to parallel. PR-URL: #12403 Backport-PR-URL: #13060 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
Trott commented Jul 18, 2017
#13060 landed. Removed |
7 participants
Refactor test to be skipped in situations where it is expected to fail.
Move from disabled directory to parallel.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test process