Skip to content

Add unit test setup with php_network_connect_socket test #16987

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 3 commits into
base: master
Choose a base branch
from
Open

Add unit test setup with php_network_connect_socket test #16987

wants to merge 3 commits into from

Conversation

bukka
Copy link
Member

@bukka bukka commented Nov 29, 2024 •
edited
Loading

This PR introduces new unit tests using cmocka. The idea is to use those tests for things that are very hard to test using the integration tests (especially the networking related stuff).

Currently it just contains a test for php_network_connect_socket that was used for testing #16606 (this was initially introduced as part of that PR but separated here to give more time for review).

The build is a bit rusty (single Makefile) but it depends on php-src build to be done first so libphp.a is available. It means it requires a minimal php-src build (./configure --disable-all --enable-embed=static). Not sure how to best integrate it to the php-src build so I just chose to have it separate. Maybe @petk can have some suggestions how to improve the build here. It might be also an option to create a separate configure.ac as it should probably check that --wrap is available (e.g. it's not available on OSX) and cmocka installed.

Also it might be worth to add it to the pipeline. This could actually be limited to just specific paths (path filter) so it runs only if the files that are unit tested are changed (in this case it would be just main/network.c)

@bukka bukka mentioned this pull request Nov 29, 2024
Closed
@cmb69
Copy link
Member

cmb69 commented Dec 2, 2024

This PR introduces new unit tests using cmocka.

It might be a good idea to pursue the RFC process, if we want to introduce a new unit testing framework. Besides that these tests need maintainance, there might be other options than cmocka.

Not sure how to best integrate it to the php-src build so I just chose to have it separate.

Maybe we could have a dedicated SAPI (or possible use embed)?

@bukka
Copy link
Member Author

bukka commented Dec 16, 2024

It might be a good idea to pursue the RFC process, if we want to introduce a new unit testing framework. Besides that these tests need maintainance, there might be other options than cmocka.

So I did some comparison and decided to just sum it up on internals and open the discussion there. Unless there are some objections I don't feel we necessarily need RFC as it is just an internal change (most votes don't care about this). In terms of maintenance, if it's limited to only critical parts, it should be fine but of course there is a bit extra thing to check but I think it's for good.

Maybe we could have a dedicated SAPI (or possible use embed)?

I think embed SAPI is fine for now as it has all that is currently needed.

arnaud-lb added a commit to arnaud-lb/php-src that referenced this pull request Jul 26, 2025
@arnaud-lb
Add unique entry point for extra tests
4ddb279 
We are adding extra (non-phpt) test suites in [1] and [2]. In order to
avoid touching CI files too often (which are maintained in 8.1 and merged in
upper branches), we add a single entry point to call the extra tests.

[1] php#16987
[2] php#18939
arnaud-lb added a commit to arnaud-lb/php-src that referenced this pull request Jul 26, 2025
@arnaud-lb
Add unique entry point for extra tests
da720d9 
We are adding extra (non-phpt) test suites in [1] and [2]. In order to
avoid touching CI files too often (which are maintained in 8.1 and merged in
upper branches), we add a single entry point to call the extra tests.

[1] php#16987
[2] php#18939
This was referenced Jul 26, 2025
Closed
Closed
arnaud-lb added a commit that referenced this pull request Jul 26, 2025
@arnaud-lb
Add unique entry point for extra tests
b633720 
We are adding extra (non-phpt) test suites in [1] and [2]. In order to
avoid touching CI files too often (which are maintained in 8.1 and merged in
upper branches), we add a single entry point to call the extra tests. The entry
point can be updated in branches without synchronizing all the way from 8.1.

CI files still need to be touched to install dependencies of these tests,
but this should be manageable as these do not change often and are the same
in every branch.

Closes GH-19242.

[1] #16987
[2] #18939
@bukka bukka changed the base branch from PHP-8.2 to master August 13, 2025 11:22
@bukka bukka force-pushed the tests_unit_network_socket branch from 11190fa to ef8c917 Compare August 13, 2025 11:22
@bukka bukka requested a review from TimWolla as a code owner August 13, 2025 11:22
@bukka bukka force-pushed the tests_unit_network_socket branch from ef8c917 to 44316e4 Compare August 13, 2025 11:41
@bukka bukka requested review from a team and removed request for a team August 15, 2025 11:30
DanielEScherzer
DanielEScherzer reviewed Aug 16, 2025
View reviewed changes
.github/workflows/unit-tests.yml
run: |
set -x
cd tests/unit
make test
Copy link
Member

@DanielEScherzer DanielEScherzer Aug 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

suggest adding a trailing newline

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielEScherzer DanielEScherzer DanielEScherzer left review comments

@TimWolla TimWolla Awaiting requested review from TimWolla TimWolla is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bukka @cmb69 @DanielEScherzer