From 36a5f4ffaffa8592709916d17526bee68e2b515c Mon Sep 17 00:00:00 2001 From: Kamil Tekiela Date: Thu, 7 Dec 2023 02:24:31 +0100 Subject: [PATCH] Improve code quality in run-tests.php (#12889) --- run-tests.php | 153 ++++++++++++++++---------------------------------- 1 file changed, 48 insertions(+), 105 deletions(-) diff --git a/run-tests.php b/run-tests.php index 116987071ad..f545ec0e254 100755 --- a/run-tests.php +++ b/run-tests.php @@ -23,12 +23,6 @@ +----------------------------------------------------------------------+ */ -/* Temporary variables while this file is being refactored. */ -/** @var ?JUnit $junit */ -$junit = null; - -/* End temporary variables. */ - /* Let there be no top-level code beyond this point: * Only functions and classes, thanks! * @@ -150,17 +144,16 @@ function main(): void * looks like it doesn't belong, it probably doesn't; cull at will. */ global $DETAILED, $PHP_FAILED_TESTS, $SHOW_ONLY_GROUPS, $argc, $argv, $cfg, - $cfgfiles, $cfgtypes, $conf_passed, $end_time, $environment, + $end_time, $environment, $exts_skipped, $exts_tested, $exts_to_test, $failed_tests_file, - $ignored_by_ext, $ini_overwrites, $is_switch, $colorize, - $log_format, $matches, $no_clean, $no_file_cache, - $optionals, $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, $test_cnt, $test_dirs, - $test_files, $test_idx, $test_list, $test_results, $testfile, - $user_tests, $valgrind, $sum_results, $shuffle, $file_cache, $num_repeats, - $bless, $show_progress; + $ignored_by_ext, $ini_overwrites, $colorize, + $log_format, $no_clean, $no_file_cache, + $pass_options, $php, $php_cgi, $preload, + $result_tests_file, $slow_min_ms, $start_time, + $temp_source, $temp_target, $test_cnt, + $test_files, $test_idx, $test_results, $testfile, + $valgrind, $sum_results, $shuffle, $file_cache, $num_repeats, + $show_progress; // Parallel testing global $workers, $workerID; global $context_line_count; @@ -243,10 +236,6 @@ function main(): void $environment["SystemRoot"] = getenv("SystemRoot"); } - $php = null; - $php_cgi = null; - $phpdbg = null; - if (getenv('TEST_PHP_LOG_FORMAT')) { $log_format = strtoupper(getenv('TEST_PHP_LOG_FORMAT')); } else { @@ -269,10 +258,9 @@ function main(): void } // Check whether user test dirs are requested. + $user_tests = []; if (getenv('TEST_PHP_USER')) { $user_tests = explode(',', getenv('TEST_PHP_USER')); - } else { - $user_tests = []; } $exts_to_test = []; @@ -655,25 +643,11 @@ function main(): void } if (!$php) { - $php = getenv('TEST_PHP_EXECUTABLE'); - } - if (!$php) { - $php = PHP_BINARY; + $php = getenv('TEST_PHP_EXECUTABLE') ?: PHP_BINARY; } - if (!$php_cgi) { - $php_cgi = getenv('TEST_PHP_CGI_EXECUTABLE'); - } - if (!$php_cgi) { - $php_cgi = get_binary($php, 'php-cgi', 'sapi/cgi/php-cgi'); - } - - if (!$phpdbg) { - $phpdbg = getenv('TEST_PHPDBG_EXECUTABLE'); - } - if (!$phpdbg) { - $phpdbg = get_binary($php, 'phpdbg', 'sapi/phpdbg/phpdbg'); - } + $php_cgi = getenv('TEST_PHP_CGI_EXECUTABLE') ?: get_binary($php, 'php-cgi', 'sapi/cgi/php-cgi'); + $phpdbg = getenv('TEST_PHPDBG_EXECUTABLE') ?: get_binary($php, 'phpdbg', 'sapi/phpdbg/phpdbg'); putenv("TEST_PHP_EXECUTABLE=$php"); $environment['TEST_PHP_EXECUTABLE'] = $php; @@ -702,8 +676,8 @@ function main(): void // Run selected tests. $test_cnt = count($test_files); - verify_config(); - write_information(); + verify_config($php); + write_information($user_tests, $phpdbg); if ($test_cnt) { putenv('NO_INTERACTION=1'); @@ -742,19 +716,13 @@ function main(): void $exts_tested = $exts_to_test; $exts_skipped = []; sort($exts_to_test); - $test_dirs = []; - $optionals = ['Zend', 'tests', 'ext', 'sapi']; - foreach ($optionals as $dir) { + foreach (['Zend', 'tests', 'ext', 'sapi'] as $dir) { if (is_dir($dir)) { - $test_dirs[] = $dir; + find_files(TEST_PHP_SRCDIR . "/{$dir}", $dir == 'ext'); } } - foreach ($test_dirs as $dir) { - find_files(TEST_PHP_SRCDIR . "/{$dir}", $dir == 'ext'); - } - foreach ($user_tests as $dir) { find_files($dir, $dir == 'ext'); } @@ -820,10 +788,8 @@ if (!function_exists("hrtime")) { } } -function verify_config(): void +function verify_config(string $php): void { - global $php; - if (empty($php) || !file_exists($php)) { error('environment variable TEST_PHP_EXECUTABLE must be set to specify PHP executable!'); } @@ -833,9 +799,12 @@ function verify_config(): void } } -function write_information(): void +/** + * @param string[] $user_tests + */ +function write_information(array $user_tests, $phpdbg): void { - global $php, $php_cgi, $phpdbg, $php_info, $user_tests, $ini_overwrites, $pass_options, $exts_to_test, $valgrind, $no_file_cache; + global $php, $php_cgi, $php_info, $ini_overwrites, $pass_options, $exts_to_test, $valgrind, $no_file_cache; $php_escaped = escapeshellarg($php); // Get info from php @@ -930,8 +899,7 @@ VALGRIND : " . ($valgrind ? $valgrind->getHeader() : 'Not used') . " function save_results(string $output_file, bool $prompt_to_save_results): void { - global $sum_results, $failed_test_summary, - $PHP_FAILED_TESTS, $php; + global $sum_results, $failed_test_summary, $PHP_FAILED_TESTS, $php; if (getenv('NO_INTERACTION') || TRAVIS_CI) { return; @@ -1266,10 +1234,7 @@ function system_with_timeout( return $data; } -/** - * @param string|array|null $redir_tested - */ -function run_all_tests(array $test_files, array $env, $redir_tested = null): void +function run_all_tests(array $test_files, array $env, ?string $redir_tested = null): void { global $test_results, $failed_tests_file, $result_tests_file, $php, $test_idx, $file_cache; global $preload; @@ -1347,12 +1312,9 @@ function run_all_tests(array $test_files, array $env, $redir_tested = null): voi } } -/** The heart of parallel testing. - * @param string|array|null $redir_tested - */ -function run_all_tests_parallel(array $test_files, array $env, $redir_tested): void +function run_all_tests_parallel(array $test_files, array $env, ?string $redir_tested): void { - global $workers, $test_idx, $test_cnt, $test_results, $failed_tests_file, $result_tests_file, $PHP_FAILED_TESTS, $shuffle, $SHOW_ONLY_GROUPS, $valgrind, $show_progress; + global $workers, $test_idx, $test_results, $failed_tests_file, $result_tests_file, $PHP_FAILED_TESTS, $shuffle, $valgrind, $show_progress; global $junit; @@ -1860,7 +1822,6 @@ function run_test(string $php, $file, array $env): string $retried = false; retry: - $temp_filenames = null; $org_file = $file; $php_cgi = $env['TEST_PHP_CGI_EXECUTABLE'] ?? null; @@ -1921,6 +1882,7 @@ TEST $file } /* For GET/POST/PUT tests, check if cgi sapi is available and if it is, use it. */ + $uses_cgi = false; if ($test->isCGI()) { if (!$php_cgi) { return skip_test($tested, $tested_file, $shortname, 'CGI not available'); @@ -1993,7 +1955,7 @@ TEST $file $temp_skipif .= 's'; $temp_file .= 's'; $temp_clean .= 's'; - $copy_file = $temp_dir . DIRECTORY_SEPARATOR . basename(is_array($file) ? $file[1] : $file) . '.phps'; + $copy_file = $temp_dir . DIRECTORY_SEPARATOR . basename($file) . '.phps'; if (!is_dir(dirname($copy_file))) { mkdir(dirname($copy_file), 0777, true) or error("Cannot create output directory - " . dirname($copy_file)); @@ -2002,19 +1964,6 @@ TEST $file if ($test->hasSection('FILE')) { save_text($copy_file, $test->getSection('FILE')); } - - $temp_filenames = [ - 'file' => $copy_file, - 'diff' => $diff_filename, - 'log' => $log_filename, - 'exp' => $exp_filename, - 'out' => $output_filename, - 'mem' => $memcheck_filename, - 'sh' => $sh_filename, - 'php' => $temp_file, - 'skip' => $temp_skipif, - 'clean' => $temp_clean - ]; } if (is_array($IN_REDIRECT)) { @@ -2195,9 +2144,9 @@ TEST $file if (!strncasecmp('skip', $output, 4)) { if (preg_match('/^skip\s*(.+)/i', $output, $m)) { - show_result('SKIP', $tested, $tested_file, "reason: $m[1]", $temp_filenames); + show_result('SKIP', $tested, $tested_file, "reason: $m[1]"); } else { - show_result('SKIP', $tested, $tested_file, '', $temp_filenames); + show_result('SKIP', $tested, $tested_file, ''); } $message = !empty($m[1]) ? $m[1] : ''; @@ -2217,7 +2166,7 @@ TEST $file // Pretend we have an XLEAK section $test->setSection('XLEAK', ltrim(substr($output, 5))); } elseif ($output !== '') { - show_result("BORK", $output, $tested_file, 'reason: invalid output from SKIPIF', $temp_filenames); + show_result("BORK", $output, $tested_file, 'reason: invalid output from SKIPIF'); $PHP_FAILED_TESTS['BORKED'][] = [ 'name' => $file, 'test_name' => '', @@ -2233,7 +2182,7 @@ TEST $file if (!extension_loaded("zlib") && $test->hasAnySections("GZIP_POST", "DEFLATE_POST")) { $message = "ext/zlib required"; - show_result('SKIP', $tested, $tested_file, "reason: $message", $temp_filenames); + show_result('SKIP', $tested, $tested_file, "reason: $message"); $junit->markTestAs('SKIP', $shortname, $tested, null, $message); return 'SKIPPED'; } @@ -2280,7 +2229,7 @@ TEST $file } $bork_info = "Redirect info must contain exactly one TEST string to be used as redirect directory."; - show_result("BORK", $bork_info, '', '', $temp_filenames); + show_result("BORK", $bork_info, '', ''); $PHP_FAILED_TESTS['BORKED'][] = [ 'name' => $file, 'test_name' => '', @@ -2296,7 +2245,7 @@ TEST $file } $bork_info = "Redirected test did not contain redirection info"; - show_result("BORK", $bork_info, '', '', $temp_filenames); + show_result("BORK", $bork_info, '', ''); $PHP_FAILED_TESTS['BORKED'][] = [ 'name' => $file, 'test_name' => '', @@ -2315,7 +2264,7 @@ TEST $file show_file_block('php', $test->getSection('FILE'), 'TEST'); save_text($test_file, $test->getSection('FILE'), $temp_file); } else { - $test_file = $temp_file = ""; + $test_file = ""; } if ($test->hasSection('GET')) { @@ -2567,7 +2516,7 @@ COMMAND $cmd /* when using CGI, strip the headers from the output */ $headers = []; - if (!empty($uses_cgi) && preg_match("/^(.*?)\r?\n\r?\n(.*)/s", $out, $match)) { + if ($uses_cgi && preg_match("/^(.*?)\r?\n\r?\n(.*)/s", $out, $match)) { $output = trim($match[2]); $rh = preg_split("/[\n\r]+/", $match[1]); @@ -2608,9 +2557,7 @@ COMMAND $cmd } } - ksort($wanted_headers); $wanted_headers = implode("\n", $wanted_headers); - ksort($output_headers); $output_headers = implode("\n", $output_headers); } @@ -2664,8 +2611,8 @@ COMMAND $cmd if (!$leaked && !$failed_headers) { // If the test passed and CLEAN produced output, report test as borked. if ($clean_output) { - show_result("BORK", $output, $tested_file, 'reason: invalid output from CLEAN', $temp_filenames); - $PHP_FAILED_TESTS['BORKED'][] = [ + show_result("BORK", $output, $tested_file, 'reason: invalid output from CLEAN'); + $PHP_FAILED_TESTS['BORKED'][] = [ 'name' => $file, 'test_name' => '', 'output' => '', @@ -2688,7 +2635,7 @@ COMMAND $cmd $warn = true; $info = " (warn: Test passed on retry attempt)"; } else { - show_result("PASS", $tested, $tested_file, '', $temp_filenames); + show_result("PASS", $tested, $tested_file, ''); $junit->markTestAs('PASS', $shortname, $tested); return 'PASSED'; } @@ -2779,7 +2726,7 @@ $output foreach ($env as $env_var => $env_val) { $env_lines[] = "export $env_var=" . escapeshellarg($env_val ?? ""); } - $exported_environment = $env_lines ? "\n" . implode("\n", $env_lines) . "\n" : ""; + $exported_environment = "\n" . implode("\n", $env_lines) . "\n"; $sh_script = <<tool}, cannot proceed."); } - $this->version = $version; - $this->header = sprintf( - "%s (%s)", trim($header), $this->tool); + $this->header = sprintf("%s (%s)", trim($header), $this->tool); $this->version_3_8_0 = version_compare($version, '3.8.0', '>='); } @@ -3849,7 +3792,7 @@ class TestFile // Match the beginning of a section. if (preg_match('/^--([_A-Z]+)--/', $line, $r)) { - $section = (string) $r[1]; + $section = $r[1]; if (isset($this->sections[$section]) && $this->sections[$section]) { throw new BorkageException("duplicated $section section");