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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic test for posix_setegid function #2541

Closed
wants to merge 1 commit into from

Conversation

6 participants
@shaikhul
Copy link
Contributor

commented May 26, 2017

馃帀 Added basic test for posix_setegid 馃槑

This wasn't covered in gcov test coverage report for posix.c

gcov coverage

Thanks @SammyK for your inspiring presentation at #phptek 2017 馃憦 馃憦 馃憦

?>
--FILE--
<?php
var_dump(posix_setegid(posix_getegid()));

This comment has been minimized.

Copy link
@andrewnester

andrewnester May 26, 2017

Contributor

could you please add something like this

$oldId = posix_getegid();
posix_setegid(42);
var_dump(posix_getegid() === 42);
posix_setegid($oldId);
var_dump(posix_getegid() === $oldId);

at least we will make sure that it really works as expected

This comment has been minimized.

Copy link
@shaikhul

shaikhul May 26, 2017

Author Contributor

@andrewnester it needs root privileges to actually change the group ID.

// content of simple posix_test.php
echo 'My real group id is '.posix_getgid() . PHP_EOL; //20
posix_setegid(40);
echo 'My real group id is '.posix_getgid() . PHP_EOL; //20
echo 'My effective group id is '.posix_getegid() . PHP_EOL; //40

run it without sudo

鉃  php posix_test.php
My real group id is 20
My real group id is 20
My effective group id is 20

run it with sudo

鉃  sudo php posix_test.php
My real group id is 0
My real group id is 0
My effective group id is 40

How can I tell the test runner to run it with root privilege, any specific section I need to mention in the test?

I see most of the posix_set*_basic tests are usually doing the same pattern, thoughts?

This comment has been minimized.

Copy link
@andrewnester

andrewnester May 26, 2017

Contributor

@shaikhul yes, right, I forgot about sudo stuff. then your one should be fine. Thanks!

@tpunt

This comment has been minimized.

Copy link
Contributor

commented May 26, 2017

Given that this is only testing ZPP (the parameter parsing API for Zend), I don't think that this test adds much value. The given functions are really simple (they basically just map to the underlying POSIX implementation), so there really isn't much to test here.

@shaikhul

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

@tpunt are you talking about PHP_POSIX_SINGLE_ARG_FUNC macro? looks like its executing whatever given in func_name and return a boolean, so testing does add some values unless I am missing anything internal here.

Also I would like to see how to tell the test runner to use sudo privilege, any suggestion would be much appreciated.

@tpunt

This comment has been minimized.

Copy link
Contributor

commented May 27, 2017

are you talking about PHP_POSIX_SINGLE_ARG_FUNC macro? looks like its executing whatever given in func_name and return a boolean, so testing does add some values unless I am missing anything internal here.

Yes, that's exactly what it's doing. The test is testing more on the side of the underlying posix implementation, though. The PHP side of things (parameter parsing and a simple IF condition) is trivial enough not to really warrant any testing here.

Also I would like to see how to tell the test runner to use sudo privilege, any suggestion would be much appreciated.

I'm not sure that is possible.

@KalleZ

This comment has been minimized.

Copy link
Member

commented May 27, 2017

@tpunt If there is no coverage for the ZPP part, I would still welcome the change, tho like you said, I would also like to see some more coverage.

@shaikhul You can find our GCOV site over at: http://gcov.php.net/ to see if that potion of the code you are testing was not yet tested, and thanks for helping to make PHP even better!

@shaikhul

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2017

Thanks @tpunt @KalleZ
@KalleZ yeah I have added this test after checking gcov coverage report for posix.c

gcov coverage

@nikic

This comment has been minimized.

Copy link
Member

commented Jun 2, 2017

Merged as 6ee932c. Thanks!

@nikic nikic closed this Jun 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.