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

Test suite now uses socket pairs instead of memory streams #66

Merged
merged 1 commit into from Feb 12, 2017

Conversation

Projects
None yet
4 participants
@martinschroeder
Contributor

martinschroeder commented Jan 9, 2017

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.

@martinschroeder

This comment has been minimized.

Contributor

martinschroeder commented Jan 9, 2017

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.

@martinschroeder

This comment has been minimized.

Contributor

martinschroeder 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
as well.

@clue

This comment has been minimized.

Member

clue commented Jan 10, 2017

The build failed due to a timing issue with the HHVM build, can't do anything about this...

I've restarted the HHVM build and this seems to have fixed this for now 👍

@clue

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?

@martinschroeder

This comment has been minimized.

Contributor

martinschroeder commented Jan 10, 2017

Splitting it up sounds good to me.
Should i modify this PR to just address the socket pair stuff and create a new PR for the signal handling later? You could squash to commits during a merge to get rid of the signal stuff.

@martinschroeder martinschroeder changed the title from Ensure signal constants are defined. Test using socket pairs. to Test using socket pairs instead of memory streams Jan 11, 2017

@martinschroeder

This comment has been minimized.

Contributor

martinschroeder commented Jan 11, 2017

I removed the signal-related part of this PR. Using real sockets in tests enables IO testing using the abstract test class in #25 and #12.

@clue

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().

@@ -16,11 +16,39 @@ public function setUp()
abstract public function createLoop();
/**
* @deprecated Use createSocketPair() instead.
*/
public function createStream()

This comment has been minimized.

@clue

clue Jan 11, 2017

Member

Should we remove this altogether?

This comment has been minimized.

@martinschroeder

martinschroeder Jan 11, 2017

Contributor

Removing the method would be a better solution. I just marked it as deprecated to keep it backward compatible.

This comment has been minimized.

@clue

clue Feb 8, 2017

Member

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)

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);

This comment has been minimized.

@clue

clue Jan 11, 2017

Member

How can I test this? Shouldn't this use STREAM_IPPROTO_TCP? Do we need both branches?

This comment has been minimized.

@martinschroeder

martinschroeder Jan 11, 2017

Contributor

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 ($sockets as $socket) {
if (function_exists('stream_set_read_buffer')) {

This comment has been minimized.

@clue

clue Jan 11, 2017

Member

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?

This comment has been minimized.

@martinschroeder

martinschroeder Jan 11, 2017

Contributor

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.

/**
* @deprecated Use createSocketPair() and fwrite() instead.
*/
public function writeToStream($stream, $content)

This comment has been minimized.

@clue

clue Jan 11, 2017

Member

Should we remove this altogether?

$this->loop->nextTick(function () {
$this->loop->stop();
});

This comment has been minimized.

@clue

clue Jan 11, 2017

Member

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!

This comment has been minimized.

@martinschroeder

martinschroeder Jan 11, 2017

Contributor

I will have to start from scratch to undo the formatting. I will look into it later today or maybe tomorrow.

@martinschroeder

This comment has been minimized.

Contributor

martinschroeder commented Jan 11, 2017

I fixed the code formatting. Unfortunately the HHVM build failed again due to a timing problem...

@jsor

This comment has been minimized.

Member

jsor commented Feb 3, 2017

@martinschroeder You should probably rebase/merge the master branch to get b946301 in which should fix the HHVM tests.

@martinschroeder

This comment has been minimized.

Contributor

martinschroeder commented Feb 3, 2017

I merged the master branch, this fixed the HHVM test case. 👍

@clue clue added the maintenance label Feb 8, 2017

@clue

This comment has been minimized.

Member

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? :shipit:

@clue clue added this to the v0.4.3 milestone Feb 8, 2017

@clue clue changed the title from Test using socket pairs instead of memory streams to Test suite now uses socket pairs instead of memory streams Feb 8, 2017

@clue clue changed the base branch from master to 0.4 Feb 12, 2017

@martinschroeder

This comment has been minimized.

Contributor

martinschroeder commented Feb 12, 2017

I rebased the PR on the 0.4 branch and removed the obsolete / deprecated methods createStream() and writeToStream(). I squashed all my commits into a single commit to avoid work-in-progress commits in the history.

@clue

This comment has been minimized.

Member

clue commented Feb 12, 2017

Awesome, much appreciated! 👍

@clue

clue approved these changes Feb 12, 2017

@jsor

jsor approved these changes Feb 12, 2017

@WyriHaximus WyriHaximus merged commit d6e47c5 into reactphp:0.4 Feb 12, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@WyriHaximus

This comment has been minimized.

Member

WyriHaximus commented Feb 12, 2017

Looking good, thanks 👍

@jsor jsor referenced this pull request Feb 15, 2017

Merged

Update master from 0.4 #81

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment