Test suite now uses socket pairs instead of memory streams #66
Conversation
|
There is one failing test (testIgnoreRemovedCallback) remaining with lib ev loop. I think that this test is problematic because it assumes that the loop backend triggers events in the same order as writes within the test code. I've done some testing and was not able to get a reliable ordering with libev. |
|
The latest commit fixed the ordering problem with libev. The build failed due to a timing issue with the HHVM build, can't do anything about this... @WyriHaximus this PR should fix some tests failing due to invalid file descriptors in reactphp-async-interop-loop |
I've restarted the HHVM build and this seems to have fixed this for now 👍 |
|
Splitting it up sounds good to me. |
ghost
changed the title
tests/AbstractLoopTest.php
Outdated
| @@ -16,11 +16,39 @@ public function setUp() | |||
|
|
|||
| abstract public function createLoop(); | |||
|
|
|||
| /** | |||
| * @deprecated Use createSocketPair() instead. | |||
| */ | |||
| public function createStream() | |||
There was a problem hiding this comment.
Removing the method would be a better solution. I just marked it as deprecated to keep it backward compatible.
There was a problem hiding this comment.
Can these be removed now? Afaict they're not needed anymore and BC doesn't really apply to our test suite because it's not part of our public API 👍 (see also below)
tests/AbstractLoopTest.php
Outdated
| if ($this->isUnixSocketSupported()) { | ||
| $sockets = stream_socket_pair(STREAM_PF_UNIX, STREAM_SOCK_STREAM, STREAM_IPPROTO_IP); | ||
| } else { | ||
| $sockets = stream_socket_pair(STREAM_PF_INET, STREAM_SOCK_STREAM, STREAM_IPPROTO_IP); |
There was a problem hiding this comment.
How can I test this? Shouldn't this use STREAM_IPPROTO_TCP? Do we need both branches?
There was a problem hiding this comment.
I could turn this into one branch like this:
$domain = $this->isUnixSocketSupported() ? STREAM_PF_UNIX : STREAM_PF_INET;
$sockets = stream_socket_pair($domain, STREAM_SOCK_STREAM, STREAM_IPPROTO_IP);This ensures that it is allways the same function call.
tests/AbstractLoopTest.php
Outdated
| } | ||
|
|
||
| foreach ($sockets as $socket) { | ||
| if (function_exists('stream_set_read_buffer')) { |
There was a problem hiding this comment.
This check could be performed before the loop. However, this is lacking some documentation, why is this needed in the first place? And why do we not bother if it doesn't exist?
There was a problem hiding this comment.
Disabling the read buffers is necessary because some event loop extensions will not report streams as readable if PHP does read buffering. Setting the buffer to 0 will ensure that all backends will work on the socket directly.
tests/AbstractLoopTest.php
Outdated
|
|
||
| /** | ||
| * @deprecated Use createSocketPair() and fwrite() instead. | ||
| */ | ||
| public function writeToStream($stream, $content) |
tests/AbstractLoopTest.php
Outdated
| $this->loop->nextTick(function () { | ||
| $this->loop->stop(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Unfortunately this PR contains plenty of unrelated formatting changes (see above and below) which make this kind of hard to review. Can you revert these changes so we can focus on what's important here? Thanks!
There was a problem hiding this comment.
I will have to start from scratch to undo the formatting. I will look into it later today or maybe tomorrow.
|
I fixed the code formatting. Unfortunately the HHVM build failed again due to a timing problem... |
|
@martinschroeder You should probably rebase/merge the master branch to get b946301 in which should fix the HHVM tests. |
|
I merged the master branch, this fixed the HHVM test case. 👍 |
|
Ping @martinschroeder, what do you think about the above suggestion? Also, may I ask you to rebase this on the new "0.4" branch so we get this in for the upcoming v0.4.3 release? |
clue
changed the title
|
I rebased the PR on the 0.4 branch and removed the obsolete / deprecated methods |
|
Awesome, much appreciated! 👍 |
|
Looking good, thanks 👍 |
This PR contains 2 changes:Warnings when using undefined signal constants in a PHPUnit data provider are avoided. Missing constants for signals are defined as needed in the abstract test case.Streams used by test cases are created as connected socket pairs. Most event loop extension do not support arbitrary file descriptors but they all support socket pairs. The created socket pairs use unix domain sockets if they are available. The fallback to INET sockets is needed in order to support Windows.