Skip to content

Skip socket sendrecv test (WIN32) not for Windows #3661

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

Conversation

mizunashi-mana
Copy link
Contributor

This seems to be for Unix test.
And, https://github.com/php/php-src/blob/8021e0f5dcc52af60d1b1b823c732e22a390d48a/ext/sockets/tests/socket_sendrecvmsg_multi_msg.phpt is for Windows, right?

So, we should skip the test for Windows on macOS or Linux.

@petk
Copy link
Member

petk commented Nov 9, 2018

Hello, thanks for all the patches @mizunashi-mana keeping tests in working condition is great.

Worth noting that this one has been previously changed to OS agnostic testing via 2f70038#diff-7e882b334d4823080a099828b9f9a16d

It seems that this test can be run under Windows after all, as it also says in the comment...
cc @carusogabriel

@mizunashi-mana
Copy link
Contributor Author

Additionally, this test fails on macOS because IPV6_RECVPKTINFO is not defined:

#define IPV6_RECVPKTINFO IPV6_PKTINFO
. The Unix test has IPV6_RECVPKTINFO test logic.

@petk
Copy link
Member

petk commented Nov 9, 2018

Just thinking out loud, wouldn't it be better to filter it using it the PHP 7.2+ constant PHP_OS_FAMILY something like this:

if (PHP_OS_FAMILY === 'Darwin') {
    die('skip non macOS test');
}

@petk
Copy link
Member

petk commented Nov 9, 2018

Worth noting that ext/sockets/tests/socket_sendrecvmsg_multi_msg.phpt is passing on Unix and Windows boxes. So it's failing only on macOS probably...

@cmb69
Copy link
Member

cmb69 commented Nov 9, 2018

Just thinking out loud, wouldn't it be better to filter it using it the PHP 7.2+ constant PHP_OS_FAMILY

Sounds reasonable. If we do it, than we should also update the documentation.

@carusogabriel
Copy link
Contributor

carusogabriel commented Nov 9, 2018

Sounds reasonable. If we do it, than we should also update the documentation.

Thanks for share this link @cmb69, there's some stuff I'd like to put into there and I didn't even know about it.

@mizunashi-mana
Copy link
Contributor Author

Hmm... This may not be the mistake of test, and be the bug of PHP...

We should add fallback for macOS by defining __APPLE_USE_RFC_3542 when we include in.h.

See also:

@mizunashi-mana
Copy link
Contributor Author

I reported on https://bugs.php.net/bug.php?id=77136 . May I close this PR?

@nikic
Copy link
Member

nikic commented Nov 20, 2018

Closing this as #3663 has landed, which also fixes this issue, if I understood correctly.

@nikic nikic closed this Nov 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants