Skip to content

When running FPM tests, pass -n option to php-fpm #11373

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

Closed
wants to merge 1 commit into from

Conversation

tstarling
Copy link
Contributor

"make test" was failing for me, because I was running it prior to
install, and there was an old installed version which was incompatible.
The FPM tests were using the installed php.ini which referenced
installed dynamically linked extensions.

So, pass -n to php-fpm.

FPM\Tester::start() has $extensions, $iniEntries and
TEST_FPM_EXTENSION_DIR but they are not actually set by anything. Rather
than rely on the installed php.ini to be correct, it seems safer to pass
-n, and any tests that need specific config can pass $iniEntries.

"make test" was failing for me, because I was running it prior to
install, and there was an old installed version which was incompatible.
The FPM tests were using the installed php.ini which referenced
installed dynamically linked extensions.

So, pass -n to php-fpm.

FPM\Tester::start() has $extensions, $iniEntries and
TEST_FPM_EXTENSION_DIR but they are not actually set by anything. Rather
than rely on the installed php.ini to be correct, it seems safer to pass
-n, and any tests that need specific config can pass $iniEntries.
@tstarling tstarling requested a review from bukka as a code owner June 5, 2023 00:55
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.

It's been useful in past to change ini and extension but both are now supported using test params so I agree there is no longer need for this and it also made some failures for other users in past. I just tested it locally and it also passes for all other tests that are not running in the pipeline so all good.

@bukka
Copy link
Member

bukka commented Jun 15, 2023

Just a note that I will merge it to master only in case some configurations depended for some reason on their ini even though it is probably unlikely but don't want to be bothered about stable version complications.

@bukka bukka closed this in ea2ee60 Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants