Skip to content

Fix #79639 - Preserving the html report options in parallel worker mode #5632

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fix #79639 - Preserving the html report options in parallel worker mode #5632

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
181 changes: 137 additions & 44 deletions run-tests.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,23 +137,25 @@ function main()
global $DETAILED, $PHP_FAILED_TESTS, $SHOW_ONLY_GROUPS, $argc, $argv, $cfg,
$cfgfiles, $cfgtypes, $conf_passed, $end_time, $environment,
$exts_skipped, $exts_tested, $exts_to_test, $failed_tests_file,
$html_file, $html_output, $ignored_by_ext, $ini_overwrites, $is_switch,
$ignored_by_ext, $ini_overwrites, $is_switch,
$just_save_results, $log_format, $matches, $no_clean, $no_file_cache,
$optionals, $output_file, $pass_option_n, $pass_options,
$pattern_match, $php, $php_cgi, $phpdbg, $preload, $redir_tests,
$repeat, $result_tests_file, $slow_min_ms, $start_time, $switch,
$temp_source, $temp_target, $temp_urlbase, $test_cnt, $test_dirs,
$test_files, $test_idx, $test_list, $test_results, $testfile,
$user_tests, $valgrind, $sum_results, $shuffle, $file_cache;
$user_tests, $valgrind, $sum_results, $shuffle, $file_cache, $outputHandler;
// Parallel testing
global $workers, $workerID;

define('IS_WINDOWS', substr(PHP_OS, 0, 3) == "WIN");

$outputHandler = new OutputHandler();

$workerID = 0;
if (getenv("TEST_PHP_WORKER")) {
if (is_worker_mode()) {
$workerID = intval(getenv("TEST_PHP_WORKER"));
run_worker();
run_worker($workerID);
return;
}

Expand Down Expand Up @@ -378,8 +380,6 @@ function main()

$just_save_results = false;
$valgrind = null;
$html_output = false;
$html_file = null;
$temp_source = null;
$temp_target = null;
$temp_urlbase = null;
Expand Down Expand Up @@ -607,8 +607,9 @@ function main()
}
break;
case '--html':
$html_file = fopen($argv[++$i], 'wt');
$html_output = is_resource($html_file);
$html_output_filename = $argv[++$i];
$outputHandler->setHtmlOutputFilename($html_output_filename);
$outputHandler->clearHtmlFile();
break;
case '--version':
echo '$Id$' . "\n";
Expand Down Expand Up @@ -691,7 +692,7 @@ function main()
usort($test_files, "test_sort");
$start_time = time();

if (!$html_output) {
if (!$outputHandler->isHtmlEnabled()) {
echo "Running selected tests.\n";
} else {
show_start($start_time);
Expand All @@ -701,7 +702,7 @@ function main()
run_all_tests($test_files, $environment);
$end_time = time();

if ($html_output) {
if ($outputHandler->isHtmlEnabled()) {
show_end($end_time);
}

Expand All @@ -719,14 +720,14 @@ function main()
}

compute_summary();
if ($html_output) {
fwrite($html_file, "<hr/>\n" . get_summary(false, true));
if ($outputHandler->isHtmlEnabled()) {
$outputHandler->writeHtml("<hr/>\n" . get_summary(false, true));
}
echo "=====================================================================";
echo get_summary(false, false);

if ($html_output) {
fclose($html_file);
if ($outputHandler->isHtmlEnabled()) {
$outputHandler->closeHtmlHandle();;
}

if ($output_file != '' && $just_save_results) {
Expand Down Expand Up @@ -792,8 +793,8 @@ function main()
show_end($end_time);
show_summary();

if ($html_output) {
fclose($html_file);
if ($outputHandler->isHtmlEnabled()) {
$outputHandler->closeHtmlHandle();
}

save_or_mail_results();
Expand Down Expand Up @@ -1282,7 +1283,7 @@ function system_with_timeout($commandline, $env = null, $stdin = null, $captureS

function run_all_tests($test_files, $env, $redir_tested = null)
{
global $test_results, $failed_tests_file, $result_tests_file, $php, $test_idx, $file_cache;
global $test_results, $failed_tests_file, $result_tests_file, $php, $test_idx, $file_cache, $outputHandler;
// Parallel testing
global $PHP_FAILED_TESTS, $workers, $workerID, $workerSock;

Expand Down Expand Up @@ -1351,7 +1352,8 @@ function run_all_tests($test_files, $env, $redir_tested = null)
/** The heart of parallel testing. */
function run_all_tests_parallel($test_files, $env, $redir_tested)
{
global $workers, $test_idx, $test_cnt, $test_results, $failed_tests_file, $result_tests_file, $PHP_FAILED_TESTS, $shuffle, $SHOW_ONLY_GROUPS, $valgrind;
global $workers, $test_idx, $test_cnt, $test_results, $failed_tests_file, $result_tests_file, $PHP_FAILED_TESTS,
$shuffle, $SHOW_ONLY_GROUPS, $valgrind, $outputHandler;

// The PHP binary running run-tests.php, and run-tests.php itself
// This PHP executable is *not* necessarily the same as the tested version
Expand Down Expand Up @@ -1412,7 +1414,7 @@ function run_all_tests_parallel($test_files, $env, $redir_tested)
// Don't start more workers than test files.
$workers = max(1, min($workers, count($test_files)));

echo "Spawning workers… ";
echo "Spawning workers ... ";
Copy link
Member

Choose a reason for hiding this comment

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

This change is kinda unrelated and I'm not sure if it makes sense to change


// We use sockets rather than STDIN/STDOUT for comms because on Windows,
// those can't be non-blocking for some reason.
Expand Down Expand Up @@ -1455,10 +1457,18 @@ function run_all_tests_parallel($test_files, $env, $redir_tested)
error("Failed to accept connection from worker $i");
}

$htmlOutputHandlerSettings = [];
if($outputHandler->isHtmlEnabled()) {
$htmlOutputHandlerSettings['filename'] = $outputHandler->getHtmlOutputFilename();
}

$greeting = base64_encode(serialize([
"type" => "hello",
"workerID" => $i,
"GLOBALS" => $GLOBALS,
'output_handler' => [
'html' => $htmlOutputHandlerSettings
],
"constants" => [
"INIT_DIR" => INIT_DIR,
"TEST_PHP_SRCDIR" => TEST_PHP_SRCDIR,
Expand Down Expand Up @@ -1688,9 +1698,9 @@ function kill_children(array $children)
}
}

function run_worker()
function run_worker($workerID)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It seems inconsistent to have this as a global variable in some places but a regular parameter with the same value here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In an effort to move away from the global problem, this is the beginning of
many PRs.

Im trying to keep this PR tiny and focus on HTML bug fix, so i can come
back with another PR with many improvements.

I've started with this small win here, on this line and there will be more
to come.

{
global $workerID, $workerSock;
global $workerSock, $outputHandler;

$sockUri = getenv("TEST_PHP_URI");

Expand Down Expand Up @@ -1721,6 +1731,10 @@ function run_worker()
define($const, $value);
}

if(isset($greeting["output_handler"]['html']['filename'])) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: space between if and (isset

Copy link
Contributor Author

@dragoonis dragoonis May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I will format this code properly for styling. (phpStorm does this for me). I will hold off on this as it looks like we're likely to delete this code.

$outputHandler->setHtmlOutputFilename($greeting["output_handler"]['html']['filename']);
}

send_message($workerSock, [
"type" => "hello_reply",
"workerID" => $workerID
Expand Down Expand Up @@ -1775,14 +1789,15 @@ function show_file_block($file, $block, $section = null)
//
function run_test($php, $file, $env)
{
global $log_format, $ini_overwrites, $PHP_FAILED_TESTS;
global $outputHandler, $log_format, $ini_overwrites, $PHP_FAILED_TESTS;
global $pass_options, $DETAILED, $IN_REDIRECT, $test_cnt, $test_idx;
global $valgrind, $temp_source, $temp_target, $cfg, $environment;
global $no_clean;
global $SHOW_ONLY_GROUPS;
global $no_file_cache;
global $slow_min_ms;
global $preload, $file_cache;

// Parallel testing
global $workerID;
$temp_filenames = null;
Expand Down Expand Up @@ -3187,45 +3202,44 @@ function get_summary($show_ext_summary, $show_html)

function show_start($start_time)
{
global $html_output, $html_file;
global $outputHandler;

if ($html_output) {
fwrite($html_file, "<h2>Time Start: " . date('Y-m-d H:i:s', $start_time) . "</h2>\n");
fwrite($html_file, "<table>\n");
if ($outputHandler->isHtmlEnabled()) {
$outputHandler->writeHtml("<h2>Time Start: " . date('Y-m-d H:i:s', $start_time) . "</h2>\n<table>\n");
}

echo "TIME START " . date('Y-m-d H:i:s', $start_time) . "\n=====================================================================\n";
}

function show_end($end_time)
{
global $html_output, $html_file;
global $outputHandler;

if ($html_output) {
fwrite($html_file, "</table>\n");
fwrite($html_file, "<h2>Time End: " . date('Y-m-d H:i:s', $end_time) . "</h2>\n");
if ($outputHandler->isHtmlEnabled()) {
$outputHandler->writeHtml("</table>\n");
$outputHandler->writeHtml("<h2>Time End: " . date('Y-m-d H:i:s', $end_time) . "</h2>\n");
}

echo "=====================================================================\nTIME END " . date('Y-m-d H:i:s', $end_time) . "\n";
}

function show_summary()
{
global $html_output, $html_file;
global $outputHandler;

if ($html_output) {
fwrite($html_file, "<hr/>\n" . get_summary(true, true));
if ($outputHandler->isHtmlEnabled()) {
$outputHandler->writeHtml("<hr/>\n" . get_summary(true, true));
}

echo get_summary(true, false);
}

function show_redirect_start($tests, $tested, $tested_file)
{
global $html_output, $html_file, $line_length, $SHOW_ONLY_GROUPS;
global $outputHandler, $line_length, $SHOW_ONLY_GROUPS;

if ($html_output) {
fwrite($html_file, "<tr><td colspan='3'>---&gt; $tests ($tested [$tested_file]) begin</td></tr>\n");
if ($outputHandler->isHtmlEnabled()) {
$outputHandler->writeHtml("<tr><td colspan='3'>---&gt; $tests ($tested [$tested_file]) begin</td></tr>\n");
}

if (!$SHOW_ONLY_GROUPS || in_array('REDIRECT', $SHOW_ONLY_GROUPS)) {
Expand All @@ -3237,10 +3251,10 @@ function show_redirect_start($tests, $tested, $tested_file)

function show_redirect_ends($tests, $tested, $tested_file)
{
global $html_output, $html_file, $line_length, $SHOW_ONLY_GROUPS;
global $outputHandler, $line_length, $SHOW_ONLY_GROUPS;

if ($html_output) {
fwrite($html_file, "<tr><td colspan='3'>---&gt; $tests ($tested [$tested_file]) done</td></tr>\n");
if ($outputHandler->isHtmlEnabled()) {
$outputHandler->writeHtml("<tr><td colspan='3'>---&gt; $tests ($tested [$tested_file]) done</td></tr>\n");
}

if (!$SHOW_ONLY_GROUPS || in_array('REDIRECT', $SHOW_ONLY_GROUPS)) {
Expand Down Expand Up @@ -3282,15 +3296,14 @@ function parse_conflicts(string $text): array

function show_result($result, $tested, $tested_file, $extra = '', $temp_filenames = null)
{
global $html_output, $html_file, $temp_target, $temp_urlbase, $line_length, $SHOW_ONLY_GROUPS;

global $temp_target, $temp_urlbase, $line_length, $SHOW_ONLY_GROUPS, $outputHandler;
if (!$SHOW_ONLY_GROUPS || in_array($result, $SHOW_ONLY_GROUPS)) {
echo "$result $tested [$tested_file] $extra\n";
} elseif (!$SHOW_ONLY_GROUPS) {
clear_show_test();
}

if ($html_output) {
if ($outputHandler->isHtmlEnabled()) {
if (isset($temp_filenames['file']) && file_exists($temp_filenames['file'])) {
$url = str_replace($temp_target, $temp_urlbase, $temp_filenames['file']);
$tested = "<a href='$url'>$tested</a>";
Expand Down Expand Up @@ -3321,8 +3334,7 @@ function show_result($result, $tested, $tested_file, $extra = '', $temp_filename
$mem = "&nbsp;";
}

fwrite(
$html_file,
$outputHandler->writeHtml(
"<tr>" .
"<td>$result</td>" .
"<td>$tested</td>" .
Expand Down Expand Up @@ -3692,6 +3704,7 @@ public function wrapCommand($cmd, $memcheck_filename, $check_all)
}
}


function init_output_buffers()
{
// Delete as much output buffers as possible.
Expand All @@ -3703,6 +3716,11 @@ function init_output_buffers()
}
}

function is_worker_mode()
{
return getenv("TEST_PHP_WORKER");
}

function check_proc_open_function_exists()
{
if (!function_exists('proc_open')) {
Expand All @@ -3719,4 +3737,79 @@ function check_proc_open_function_exists()
}
}

class OutputHandler
{

/**
* @var null|string
*/
private $htmlOutputFilename = null;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private $htmlOutputFilename = null;
private ?string $htmlOutputFilename = null;

This one I'm less sure as I don't know how far back in compatibility we need to go, but if we are only targeting master this would make sense IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

Per the comment at the top of the file, this script is currently compatible with PHP 7.0. We could discuss raising the constraint, but unlikely to go to 7.4. (The test runner can be invoked with a different PHP version than it is testing. I believe @TysonAndre also expressed a desire to use newer test runners on PECL extensions that support old PHP versions, so it's possible to use new features.)

Copy link
Contributor Author

@dragoonis dragoonis May 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't add any types because I wanted to make it PHP 5.x compatible.

So what I'd like clarification on is: should I be adding any 7.0 features to this script? If so I'll go ahead and add types. Please clarify. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Per the comment at the top of the file, this script is currently compatible with PHP 7.0. We could discuss raising the constraint, but unlikely to go to 7.4. (The test runner can be invoked with a different PHP version than it is testing. I believe @TysonAndre also expressed a desire to use newer test runners on PECL extensions that support old PHP versions, so it's possible to use new features.)

The most significant new feature to me was the parallelism to take advantage of all cores on a test runner -j N.

I'm fine with raising the constraints - PECL projects which did need a run-tests.php that worked with 7.0-7.4 (and probably 8.x) could just copy the run-tests.php from the PHP-7.4 branch.

Copy link
Contributor

@TysonAndre TysonAndre May 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This file is already incompatible with php 5.6 - I don't think anyone is planning to support php 5.6 in the latest versions of run-tests.php.

It aimed for 7.0+ in one release, but that's already available, and can get raised.

I don't have a personal need for newer features right now, but I could imagine others wanting it

    $environment = $_ENV ?? array();  // line 190 uses php 7's null coalescing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case i will update this code to give us the type safety advantages
of php 7. Thanks


/**
* @var null|resource
*/
private $htmlHandle = null;

/**
* @param string $outputFilename
*/
public function setHtmlOutputFilename($outputFilename)
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 not using a type declaration?

Suggested change
public function setHtmlOutputFilename($outputFilename)
public function setHtmlOutputFilename(string $outputFilename)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the conversation below. Making this incompatible with php 5.x isn't something we've discussed. Once we've had that dsicussion then I'll update this file, respectively.

{
$this->htmlOutputFilename = $outputFilename;
}

public function clearHtmlFile()
{
ftruncate($this->getHtmlFileHandle(), 0);
}

/**
* @return string
*/
public function getHtmlOutputFilename()
{
if($this->htmlOutputFilename === null) {
throw new \Exception('No output filename has been set.');
}

return $this->htmlOutputFilename;
}

/**
* @return false|resource
*/
public function getHtmlFileHandle()
{
if($this->htmlHandle !== null) {
return $this->htmlHandle;
}

$this->fileHandle = fopen($this->getHtmlOutputFilename(), 'at+');

return $this->fileHandle;
}

/**
* @return bool
*/
public function isHtmlEnabled()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function isHtmlEnabled()
public function isHtmlEnabled(): bool

{
return !empty($this->htmlOutputFilename) && is_resource($this->getHtmlFileHandle());
}

public function closeHtmlHandle()
{
fclose($this->getHtmlFileHandle());
}

/**
* @param $content
*/
public function writeHtml($content)
{
fwrite($this->getHtmlFileHandle(), $content);
}

}

main();