Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
"php": ">=5.4.0",
"evenement/evenement": "~2.0",
"react/event-loop": "0.4.*",
"react/stream": "0.4.*"
"react/stream": "0.4.*",
"phpunit/phpunit": "~4.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be necessary, as sebastian/environment was already a dev dependency. Even if we were to add PHPUnit as a dependency, it should be included under require-dev. That said, I'm of the opinion that it's redundant though, as we can expect developers and Travis to have it already.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, even if sebastian/environment has a dev dependency on phpunit, dev-dependencies of required packages are not included (only the root package dev-dependencies are included by Composer). Anyway, you are right to say I should have put this in the require-dev, and anyway, it is a mistake from me. I never intended to commit this change. I'll revert it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I wasn't clear, but I meant sebastian/environment was a dev dependency because our tests needed it to create command line strings. It was listed explicitly because some dated versions of PHPUnit (likely what Travis CI had at the time it was first added) did not depend on it, so we couldn't guarantee it would be available. PHPUnit was never a dev dependency because we assume it's available as a global binary (I realize other projects hold different opinions about that, though).

},
"require-dev": {
"sebastian/environment": "~1.0"
Expand Down
101 changes: 93 additions & 8 deletions tests/AbstractProcessTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,22 +68,29 @@ public function testGetTermSignalWhenRunning($process)

public function testProcessWithDefaultCwdAndEnv()
{
$cmd = $this->getPhpBinary() . ' -r ' . escapeshellarg('echo getcwd(), PHP_EOL, count($_SERVER), PHP_EOL;');
$cmd = $this->getPhpCommandLine('echo getcwd(), PHP_EOL, count($_SERVER), PHP_EOL;');

$loop = $this->createLoop();
$process = new Process($cmd);

$output = '';
$error = '';

$loop->addTimer(0.001, function(Timer $timer) use ($process, &$output) {
$loop->addTimer(0.001, function(Timer $timer) use ($process, &$output, &$error) {
$process->start($timer->getLoop());
$process->stdout->on('data', function () use (&$output) {
$output .= func_get_arg(0);
$process->stdout->on('data', function ($data) use (&$output) {
$output .= $data;
});
$process->stderr->on('data', function ($data) use (&$error) {
$error .= $data;
});
});

$loop->run();

$this->assertEmpty($error);
$this->assertNotEmpty($output);

list($cwd, $envCount) = explode(PHP_EOL, $output);

/* Child process should inherit the same current working directory and
Expand All @@ -96,10 +103,16 @@ public function testProcessWithDefaultCwdAndEnv()

public function testProcessWithCwd()
{
$cmd = $this->getPhpBinary() . ' -r ' . escapeshellarg('echo getcwd(), PHP_EOL;');
$cmd = $this->getPhpCommandLine('echo getcwd(), PHP_EOL;');

if (defined('PHP_WINDOWS_VERSION_BUILD')) {
$testCwd = 'C:\\';
} else {
$testCwd = '/';
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary? Does getcwd() end up returning a Windows driver letter when we assert on it later even if we pass in /?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly. on Windows , getcwd() will return "C:", even if you pass '/' as the default cwd for the command.


$loop = $this->createLoop();
$process = new Process($cmd, '/');
$process = new Process($cmd, $testCwd);

$output = '';

Expand All @@ -112,7 +125,7 @@ public function testProcessWithCwd()

$loop->run();

$this->assertSame('/' . PHP_EOL, $output);
$this->assertSame($testCwd . PHP_EOL, $output);
}

public function testProcessWithEnv()
Expand All @@ -121,7 +134,13 @@ public function testProcessWithEnv()
$this->markTestSkipped('Cannot execute PHP processes with custom environments on Travis CI.');
}

$cmd = $this->getPhpBinary() . ' -r ' . escapeshellarg('echo getenv("foo"), PHP_EOL;');
$cmd = $this->getPhpCommandLine('echo getenv("foo"), PHP_EOL;');

if (defined('PHP_WINDOWS_VERSION_BUILD')) {
// Windows madness! escapeshellarg seems to completely remove double quotes in Windows!
// We need to use simple quotes in our PHP code!
$cmd = $this->getPhpCommandLine('echo getenv(\'foo\'), PHP_EOL;');
}

$loop = $this->createLoop();
$process = new Process($cmd, null, array('foo' => 'bar'));
Expand Down Expand Up @@ -173,6 +192,10 @@ public function testStartAndAllowProcessToExitSuccessfullyUsingEventLoop()

public function testStartInvalidProcess()
{
if (defined('PHP_WINDOWS_VERSION_BUILD')) {
$this->markTestSkipped('Windows does not have an executable flag. This test does not make sense on Windows.');
}

$cmd = tempnam(sys_get_temp_dir(), 'react');

$loop = $this->createLoop();
Expand Down Expand Up @@ -298,6 +321,56 @@ public function testTerminateWithStopAndContinueSignalsUsingEventLoop()
$this->assertFalse($process->isTerminated());
}

public function testProcessSmallOutput() {
$this->processOutputOfSize(1000);
}

public function testProcessMediumOutput() {
$this->processOutputOfSize(10000);
}

public function testProcessBigOutput() {
$this->processOutputOfSize(100000);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three tests would be better rewritten with the data provider pattern (using the output size as a single argument).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wooh, great idea! Will do!


public function processOutputOfSize($size, $expectedMaxDuration = 5)
{
// Note: very strange behaviour of Windows (PHP 5.5.6):
// on a 1000 long string, Windows succeeds.
// on a 10000 long string, Windows fails to output anything.
// On a 100000 long string, it takes a lot of time but succeeds.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean we expect a test failure on Windows?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, just saw your comment in the OP.

$cmd = $this->getPhpBinary() . ' -r ' . escapeshellarg('echo str_repeat(\'o\', '.$size.'), PHP_EOL;');

if (defined('PHP_WINDOWS_VERSION_BUILD')) {
// Windows madness! for some obscure reason, the whole command lines needs to be
// wrapped in quotes (?!?)
$cmd = '"'.$cmd.'"';
}

$loop = $this->createLoop();
$process = new Process($cmd);

$output = '';

$loop->addTimer(0.001, function(Timer $timer) use ($process, &$output) {
$process->start($timer->getLoop());
$process->stdout->on('data', function () use (&$output) {
$output .= func_get_arg(0);
});
});

$startTime = time();

$loop->run();

$endTime = time();

$this->assertEquals($size + strlen(PHP_EOL), strlen($output));
$this->assertSame(str_repeat('o', $size) . PHP_EOL, $output);
$this->assertLessThanOrEqual($expectedMaxDuration, $endTime - $startTime, "Process took longer than expected.");
}


/**
* Execute a callback at regular intervals until it returns successfully or
* a timeout is reached.
Expand Down Expand Up @@ -333,4 +406,16 @@ private function getPhpBinary()

return $runtime->getBinary();
}

private function getPhpCommandLine($phpCode)
{
$cmd = $this->getPhpBinary() . ' -r ' . escapeshellarg($phpCode);

if (defined('PHP_WINDOWS_VERSION_BUILD')) {
// Windows madness! for some obscure reason, the whole command lines needs to be
// wrapped in quotes (?!?)
$cmd = '"'.$cmd.'"';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another tip from @auroraeosrose: rather than do this, we should use the bypass_shell option for proc_open(), which is only relevant to Windows and ignored for *nix systems.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Tip by @auroraeosrose did the trick. Indeed, I can remove the quotes around the command by passing this parameter. I fixed the unit test to reflect that.

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this documented anywhere as a known PHP quirk? I don't recall seeing this done in Symfony's Process component, from which much of the React code was ported.

return $cmd;
}
}