Skip to content
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

Always use a template file for in process tests for much better DX #2548

Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Framework/TestCase.php
Expand Up @@ -14,6 +14,7 @@
use PHPUnit\Framework\Constraint\ExceptionCode;
use PHPUnit\Framework\Constraint\ExceptionMessage;
use PHPUnit\Framework\Constraint\ExceptionMessageRegularExpression;
use PHPUnit\Util\PHP;
use PHPUnit_Framework_MockObject_Generator;
use PHPUnit_Framework_MockObject_Matcher_AnyInvokedCount;
use PHPUnit_Framework_MockObject_Matcher_InvokedAtIndex;
Expand All @@ -34,7 +35,6 @@
use PHPUnit\Runner\PhptTestCase;
use PHPUnit\Util\GlobalState;
use PHPUnit\Util\InvalidArgumentHelper;
use PHPUnit\Util\PHP\AbstractPhpProcess;
use Prophecy;
use ReflectionClass;
use ReflectionException;
Expand Down Expand Up @@ -861,7 +861,7 @@ public function run(TestResult $result = null)

$this->prepareTemplate($template);

$php = AbstractPhpProcess::factory();
$php = new PHP();
$php->runTestJob($template->render(), $this, $result);
} else {
$result->run($this);
Expand Down
8 changes: 4 additions & 4 deletions src/Runner/PhptTestCase.php
Expand Up @@ -18,7 +18,7 @@
use PHPUnit\Framework\SkippedTestError;
use PHPUnit\Framework\SelfDescribing;
use PHPUnit\Util\InvalidArgumentHelper;
use PHPUnit\Util\PHP\AbstractPhpProcess;
use PHPUnit\Util\PHP;
use Throwable;

/**
Expand All @@ -32,7 +32,7 @@ class PhptTestCase implements Test, SelfDescribing
private $filename;

/**
* @var AbstractPhpProcess
* @var \PHPUnit\Util\PHP
*/
private $phpUtil;

Expand Down Expand Up @@ -66,7 +66,7 @@ class PhptTestCase implements Test, SelfDescribing
* Constructs a test case with the given filename.
*
* @param string $filename
* @param AbstractPhpProcess $phpUtil
* @param \PHPUnit\Util\PHP $phpUtil
Copy link
Contributor

Choose a reason for hiding this comment

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

changing param type for regular method is BC breaker.
@sebastianbergmann , do you treat changing __construct params as BC breaker ? I know some do, some don;t

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good question!

*
* @throws Exception
*/
Expand All @@ -86,7 +86,7 @@ public function __construct($filename, $phpUtil = null)
}

$this->filename = $filename;
$this->phpUtil = $phpUtil ?: AbstractPhpProcess::factory();
$this->phpUtil = $phpUtil ?: new PHP();
}

/**
Expand Down
246 changes: 206 additions & 40 deletions src/Util/PHP/AbstractPhpProcess.php → src/Util/PHP.php
Expand Up @@ -7,24 +7,27 @@
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace PHPUnit\Util\PHP;
namespace PHPUnit\Util;

use __PHP_Incomplete_Class;
use ErrorException;
use SebastianBergmann\Environment\Runtime;
use PHPUnit\Framework\Exception;
use PHPUnit\Framework\TestResult;
use PHPUnit\Framework\TestFailure;
use PHPUnit\Framework\Test;
use PHPUnit\Framework\SyntheticError;
use PHPUnit\Util\InvalidArgumentHelper;
use SebastianBergmann\Environment\Runtime;

/**
* Utility methods for PHP sub-processes.
* Default utility for PHP sub-processes.
*/
abstract class AbstractPhpProcess
class PHP
Copy link
Contributor

Choose a reason for hiding this comment

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

Name is too generic, when you give only a name to someone, he will have on idea what to expect.
for logic in this class - maybe replace most of it with symfony/process ?

Choose a reason for hiding this comment

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

See #1342. But that should not happen before #2015.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for switching to process component in separated PR/issue.
Yet, still, PHP is way too generic and meaningless name, it shall be modified

{
/**
* @var string
*/
protected $tempFile;

/**
* @var Runtime
*/
Expand Down Expand Up @@ -63,7 +66,7 @@ public function __construct()
$this->runtime = new Runtime();
}

/**
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the pointer! No reason of course. I fixed that.

* Defines if should use STDERR redirection or not.
*
* Then $stderrRedirection is TRUE, STDERR is redirected to STDOUT.
Expand Down Expand Up @@ -171,18 +174,6 @@ public function getTimeout()
return $this->timeout;
}

/**
* @return AbstractPhpProcess
*/
public static function factory()
{
if (DIRECTORY_SEPARATOR == '\\') {
return new WindowsPhpProcess;
}

return new DefaultPhpProcess;
}

/**
* Runs a single test in a separate PHP process.
*
Expand All @@ -209,25 +200,21 @@ public function runTestJob($job, Test $test, TestResult $result)
/**
* Returns the command based into the configurations.
*
* @param array $settings
* @param string|null $file
* @param array $settings
* @param string $file
*
* @return string
*/
public function getCommand(array $settings, $file = null)
public function getCommand(array $settings, $file)
{
$command = $this->runtime->getBinary();
$command .= $this->settingsToParameters($settings);

if ('phpdbg' === PHP_SAPI) {
$command .= ' -qrr ';

if ($file) {
$command .= '-e ' . escapeshellarg($file);
} else {
$command .= escapeshellarg(__DIR__ . '/PHP/eval-stdin.php');
}
} elseif ($file) {
$command .= '-e ' . escapeshellarg($file);
} else {
$command .= ' -f ' . escapeshellarg($file);
}

Expand All @@ -239,21 +226,14 @@ public function getCommand(array $settings, $file = null)
$command .= ' 2>&1';
}

// Special case windows.
if (DIRECTORY_SEPARATOR == '\\') {
$command = '"' . $command . '"';
}

return $command;
}

/**
* Runs a single job (PHP code) using a separate PHP process.
*
* @param string $job
* @param array $settings
*
* @return array
*
* @throws Exception
*/
abstract public function runJob($job, array $settings = []);

/**
* @param array $settings
*
Expand Down Expand Up @@ -417,4 +397,190 @@ private function getException(TestFailure $error)

return $exception;
}

/**
* Runs a single job (PHP code) using a separate PHP process.
*
* @param string $job
* @param array $settings
*
* @return array
*
* @throws Exception
*/
public function runJob($job, array $settings = [])
{
if (!($this->tempFile = tempnam(sys_get_temp_dir(), 'PHPUnit')) ||
file_put_contents($this->tempFile, $job) === false
) {
throw new Exception(
'Unable to write temporary file'
);
}

if ($this->stdin) {
$job = $this->stdin;
}

return $this->runProcess($job, $settings);
}

/**
* Returns an array of file handles to be used in place of pipes
*
* @return array
*/
protected function getHandles()
{
return [];
}

/**
* Handles creating the child process and returning the STDOUT and STDERR
*
* @param string $job
* @param array $settings
*
* @return array
*
* @throws Exception
*/
protected function runProcess($job, $settings)
{
$handles = $this->getHandles();

$env = null;
if ($this->env) {
$env = isset($_SERVER) ? $_SERVER : [];
unset($env['argv'], $env['argc']);
$env = array_merge($env, $this->env);

foreach ($env as $envKey => $envVar) {
if (is_array($envVar)) {
unset($env[$envKey]);
}
}
}

$pipeSpec = [
0 => isset($handles[0]) ? $handles[0] : ['pipe', 'r'],
1 => isset($handles[1]) ? $handles[1] : ['pipe', 'w'],
2 => isset($handles[2]) ? $handles[2] : ['pipe', 'w'],
];
$process = proc_open(
$this->getCommand($settings, $this->tempFile),
$pipeSpec,
$pipes,
null,
$env
);

if (!is_resource($process)) {
throw new Exception(
'Unable to spawn worker process'
);
}

if ($job) {
$this->process($pipes[0], $job);
}
fclose($pipes[0]);

if ($this->timeout) {
$stderr = $stdout = '';
unset($pipes[0]);

while (true) {
$r = $pipes;
$w = null;
$e = null;

$n = @stream_select($r, $w, $e, $this->timeout);

if ($n === false) {
break;
} elseif ($n === 0) {
proc_terminate($process, 9);
throw new Exception(sprintf('Job execution aborted after %d seconds', $this->timeout));
} elseif ($n > 0) {
foreach ($r as $pipe) {
$pipeOffset = 0;
foreach ($pipes as $i => $origPipe) {
if ($pipe == $origPipe) {
$pipeOffset = $i;
break;
}
}

if (!$pipeOffset) {
break;
}

$line = fread($pipe, 8192);
if (strlen($line) == 0) {
fclose($pipes[$pipeOffset]);
unset($pipes[$pipeOffset]);
} else {
if ($pipeOffset == 1) {
$stdout .= $line;
} else {
$stderr .= $line;
}
}
}

if (empty($pipes)) {
break;
}
}
}
} else {
if (isset($pipes[1])) {
$stdout = stream_get_contents($pipes[1]);
fclose($pipes[1]);
}

if (isset($pipes[2])) {
$stderr = stream_get_contents($pipes[2]);
fclose($pipes[2]);
}
}

if (isset($handles[1])) {
rewind($handles[1]);
$stdout = stream_get_contents($handles[1]);
fclose($handles[1]);
}

if (isset($handles[2])) {
rewind($handles[2]);
$stderr = stream_get_contents($handles[2]);
fclose($handles[2]);
}

proc_close($process);
$this->cleanup();

return ['stdout' => $stdout, 'stderr' => $stderr];
}

/**
* @param resource $pipe
* @param string $job
*
* @throws Exception
*/
protected function process($pipe, $job)
{
fwrite($pipe, $job);
}

/**
*/
protected function cleanup()
{
if ($this->tempFile) {
unlink($this->tempFile);
}
}
}