[run-tests.php] Multiple minor improvements for performance and readability #8963
Conversation
Ayesh
force-pushed
the
run-test-improvements
branch
from
a6868e7 to
5833053
Compare
| } | ||
|
|
||
| if (is_array($pattern_match)) { | ||
| $test_files = array_merge($test_files, $pattern_match); | ||
| } | ||
| } elseif (is_dir($testfile)) { | ||
| find_files($testfile); | ||
| } elseif (substr($testfile, -5) == '.phpt') { |
There was a problem hiding this comment.
Doing this if case before the is_dir() one would reduce file IO
There was a problem hiding this comment.
You are right the substr call (which I'm planning to replace with a str_ends_with) is better checked first, but I'm afraid this would slightly alter the test discovery flow? For example when there is a directory named foo.phpt (despite the very small odds), that I believe is still covered in the original flow of checks, but not with the suggested change.
Ayesh
force-pushed
the
run-test-improvements
branch
from
5833053 to
87425af
Compare
Ayesh
force-pushed
the
run-test-improvements
branch
from
87425af to
23ca93e
Compare
|
This PR is now rebased to |
| } | ||
|
|
||
| if (preg_match("/^$wanted_re\$/s", $output)) { | ||
| $wanted_re = strtr($wanted_re, [ |
There was a problem hiding this comment.
str_replace also can take arrays to do replacements in one go
There was a problem hiding this comment.
I thought of strtr because it takes an array of search => replace pairs as a single array, as opposed to the two separate arrays that str_replace accepts.
Ayesh
force-pushed
the
run-test-improvements
branch
from
23ca93e to
59d0b60
Compare
…s with happy path optimizations Simplifies and improves the readability of multiple `if`/`elseif`/`else` blocks by removing them when an earlier branch exists the execution flow by either returning or terminating the script.
…expressions first
…_exec()` calls The rationale is that `shell_exec()` is identical to the backtick operator (both of which are disabled when `shell_exec` function is disabled) makes it very clear that it is a shell execution, and eases security audits too.
…tr` call Makes the replacement easier to see, neatly aligned, and only takes one function call. This is safe because none of the combined replacement values contain tokens that would be recursively replaced. This also improves the readability on how the regular expressions in `EXPECTF` matcher is constructed. Co-authored-by: Michael Voříšek <mvorisek@mvorisek.cz>
Ayesh
force-pushed
the
run-test-improvements
branch
from
59d0b60 to
383d5ae
Compare
|
Thank you. |
if/elseif/elseblocks with happy path optimizationsSimplifies and improves the readability of multiple
if/elseif/elseblocks by removing them when an earlier branch exists the execution flow by either returning or terminating the script.ifblocks by placing simple expressions firstunset()calls to a single callshell_exec()callsThe rationale is that
shell_exec()is identical to the backtick operator (both of which are disabled whenshell_execfunction is disabled) makes it very clear that it is a shell execution, and eases security audits too.str_replacecalls to a singlestrtrcallMakes the replacement easier to see, neatly aligned, and only takes one function call.
This is safe because none of the combined replacement values contain tokens that would be recursively replaced.