Skip to content

tests: add missing TEST_PHP_EXTRA_ARGS #11669

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

Jump to bottom
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

tests: add missing TEST_PHP_EXTRA_ARGS #11669

wants to merge 5 commits into from

Conversation

orlitzky
Copy link
Contributor

If I test a debug build while I have a non-debug PHP installed on the system, I get a bunch of test failures that can all be traced back to,

$ sapi/cli/php 
Failed loading /usr/lib64/php8.2/lib64/opcache.so:  /usr/lib64/php8.2/lib64/opcache.so: undefined symbol: _emalloc_32

The non-debug opcache.so is trying to load from the system's INI file.

Of course, I'm running the tests with sapi/cli/php -n ./run-tests.php -n, but the -n is not always passed to sub-invocations of PHP inside the test cases. This PR fixes most of them by passing TEST_PHP_EXTRA_ARGS to those invocations, but a few (described in the commit message) were much simpler to fix with only an -n.
@orlitzky orlitzky requested a review from bukka as a code owner July 11, 2023 02:31
@iluuu1994
Copy link
Member

Always passing -n is also not optimal. CI often enables opcache and other settings through global INI files, which means the sub-processes will be executed without those enabled.
@orlitzky
Copy link
Contributor Author

Always passing -n is also not optimal. CI often enables opcache and other settings through global INI files, which means the sub-processes will be executed without those enabled.

I could replace the array argument to proc_open() with a string?
@orlitzky
Copy link
Contributor Author

(In case that last comment is a bit mysterious, replacing the array with a string would make it easy to append $TEST_PHP_EXTRA_ARGS without having to parse them first.)
@orlitzky orlitzky force-pushed the more-test-php-extra-args branch from 887e262 to 59cc6c3 Compare July 14, 2023 01:07
@orlitzky
Copy link
Contributor Author

New version adds TEST_PHP_EXTRA_ARGS (as opposed to just -n) to those proc_open() tests, at the expense of turning their nice argument array into a string.
@orlitzky
Copy link
Contributor Author

I broke something on Windows. Two of them are throwing,

Parse error: syntax error, unexpected string content "echo", expecting end of file in Command line code on line 1

which probably has something to do with cmd.exe's quotation style. The other is,

Warning: PHP Startup: Unable to load dynamic library 'mysqli' (tried: C:\php\ext\mysqli (The specified module could not be found), C:\php\ext\php_mysqli.dll (The specified module could not be found)) in Unknown on line 0

which very likely has arisen from the fact that we're now actually passing args to this PHP from,

C:\obj\Release\php.exe" -d open_basedir= -d output_buffering=0 run-tests.php -d opcache.enable=1 -d opcache.enable_cli=1 -d opcache.protect_memory=1 -d opcache.jit_buffer_size=16M -d extension=mysqli -g FAIL,BORK,LEAK,XLEAK --no-progress -q --offline --show-diff --show-slow 1000 --set-timeout 120 --temp-source c:\tests_tmp --temp-target c:\tests_tmp --bless -j2 -p "C:\obj\Release\php.exe"

If anyone actually has a Windows box and wants to just tell me WTF to do in the first two, it would save me a lot of guessing.

The second I will have to think about. It's not immediately obvious why mysqli is failing for this one test but not others.
@orlitzky orlitzky force-pushed the more-test-php-extra-args branch from 884a92a to dde18ce Compare July 15, 2023 01:54
@orlitzky
Copy link
Contributor Author

That seems to have done the trick without being too overcomplicated.
bukka
bukka reviewed Jul 15, 2023
View reviewed changes
Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

Those proc_open might need some changes.
ext/standard/tests/general_functions/proc_open_redirect.phpt
try {
proc_open([$php], [['redirect']], $pipes);
Copy link
Member

Choose a reason for hiding this comment

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

isn't this changing test from array syntax to shell syntax? If so, those are different things to test so it should not be changed. You should rather create array from $args and pass it 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.

I agree, but how do we create an array from $args? Am I overlooking an easy solution?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this seems to have stalled, the stated purpose of this file is not to test the syntax of proc_open(), but instead to test that redirection works. While it does test the array syntax as a side effect, that's not the crucial part; so in my opinion it's reasonable to change the syntax to fix this unrelated issue.
iluuu1994
iluuu1994 approved these changes Oct 3, 2023
View reviewed changes
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

This looks good from my side. @bukka Were your concerns clarified? ext/standard/tests/general_functions/proc_open_array.phpt is there to explicitly test the [] variant.
@TimWolla TimWolla removed their request for review October 3, 2023 12:18
orlitzky added 5 commits April 10, 2024 18:42
@orlitzky
sapi/cli/tests/023.phpt: support TEST_PHP_EXTRA_ARGS
bc59267 
This test runs the PHP test executable from the command-line, but was
not passing TEST_PHP_EXTRA_ARGS to it from run-tests.php. This commit
adds the args, but also updates the test case.

This test relies on memory_limit being updated by an INI file
(generated on-the-fly), but by default, run-tests.php overrides PHP's
memory_limit using TEST_PHP_EXTRA_ARGS. So if we pass the extra args
to PHP, then the memory limit will be constant. To work around that,
we switch to using max_input_vars (as opposed to memory_limit) inside
the test.
@orlitzky
ext/standard/tests: pass TEST_PHP_EXTRA_ARGS to some proc_open() tests
48a3896 
Several of our proc_open() tests fail to pass TEST_PHP_EXTRA_ARGS to
the PHP binary they execute. This can lead to spurious failures when,
for example, the system's INI file is incompatible with the just-built
CLI php:

  ---- ACTUAL OUTPUT
  array(1) {
    [2]=>
    resource(4) of type (stream)
  }
  string(123) "Failed loading /usr/lib64/php8.2/lib64/opcache.so:
  /usr/lib64/php8.2/lib64/opcache.so: undefined symbol: _emalloc_32
  Error"

Naturally we would like to pass TEST_PHP_EXTRA_ARGS to these
invocations, after which "run-tests.php -n" would avoid loading the
incompatible INI file. However, in this case, TEST_PHP_EXTRA_ARGS is a
free-form environment variable, and proc_open() is being fed an array
of arguments. It would be difficult to parse TEST_PHP_EXTRA_ARGS into
an array, so we first convert the command-line array to a string,
and then afterwards simply append the extra args.
@orlitzky
{ext/standard,sapi/cli}/tests: pass TEST_PHP_EXTRA_ARGS to php
a53828e 
Several of these tests fail to pass TEST_PHP_EXTRA_ARGS to
the PHP binary they execute. This can lead to spurious failures when,
for example, the system's INI file is incompatible with the just-built
CLI php:

  ---- EXPECTED OUTPUT
  int(111)
  ---- ACTUAL OUTPUT
  Failed loading /usr/lib64/php8.2/lib64/opcache.so:
  /usr/lib64/php8.2/lib64/opcache.so: undefined symbol: _emalloc_32
  int(111)
  ---- FAILED

This commit adds TEST_PHP_EXTRA_ARGS to the "php" invocations that are
missing them, allowing the user to run "run-tests.php -n" to work
around the problem.
@orlitzky
.github/scripts/windows: pass an extension_dir to the test runner.
624cd5b 
In some cases, on Windows, we pass -dextension=mysqli to the test
runner, but omit the extension_dir. This works fine so long as that
extension_dir can be obtained from php.ini. However, we have at least
one test that loads some other (or no) php.ini file; when that
happens, the mysqli extension cannot be found. To solve that problem,
we now also specify an extension_dir whenever we specify an extension
to load.
@orlitzky
ext/standard/tests: work around Windows quoting with tempfiles.
3cd30ff 
Two of our proc_open() tests pass code to PHP via the shell, leading
to three levels of quoting, one of which makes cmd.exe unhappy on
Windows. To avoid the issue, we use a temporary file instead, and
pass only the (at most, once-quoted) name of the file to PHP.
@orlitzky orlitzky force-pushed the more-test-php-extra-args branch from dde18ce to 3cd30ff Compare April 10, 2024 22:44
@orlitzky
Copy link
Contributor Author

I didn't realize this branch had a conflict. A line adjacent to something that I changed was changed. Force-pushed a fix to .github/scripts/windows/test_task.bat.
@orlitzky
Copy link
Contributor Author

Any chance to get this merged? Gentoo users run the test suite when PHP is installed, and they can easily switch between debug and non-debug builds by passing a flag to the package manager. I'm having to delete these tests each time to avoid the failures in that case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Extension: standard SAPI: cli
3 participants
@orlitzky @iluuu1994 @bukka