Test suite now uses socket pairs instead of memory streams#66
Uh oh!
There was an error while loading. Please reload this page.
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. |
ghost commented Jan 10, 2017
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 |
clue commented Jan 10, 2017
I've restarted the HHVM build and this seems to have fixed this for now 👍 |
clue left a comment
clue left a comment There was a problem hiding this comment.
This PR contains 2 changes […]
My main concern with this PR is that it actually addresses multiple things at once. Does it make sense to split this up into two dedicated PRs?
Splitting it up sounds good to me. |
ghost changed the title ghost commented Jan 11, 2017
clue left a comment
clue left a comment There was a problem hiding this comment.
Thanks for updating this! I think it makes sense to test this against real socket streams instead of dummy in-memory streams which aren't even supported by stream_select().
tests/AbstractLoopTest.php Outdated
| /** | ||
| * @deprecated Use createSocketPair() instead. | ||
| */ | ||
| publicfunctioncreateStream() |
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.
| } | ||
| foreach ($socketsas$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. | ||
| */ | ||
| publicfunctionwriteToStream($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.
ghost commented Jan 11, 2017
I fixed the code formatting. Unfortunately the HHVM build failed again due to a timing problem... |
jsor commented Feb 3, 2017
@martinschroeder You should probably rebase/merge the master branch to get b946301 in which should fix the HHVM tests. |
ghost commented Feb 3, 2017
I merged the master branch, this fixed the HHVM test case. 👍 |
clue commented Feb 8, 2017
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 
ghost commented Feb 12, 2017
I rebased the PR on the 0.4 branch and removed the obsolete / deprecated methods |
clue commented Feb 12, 2017
Awesome, much appreciated! 👍 |
WyriHaximus commented Feb 12, 2017
Looking good, thanks 👍 |
4 participants
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.