Skip to content
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

Add unit test setup with php_network_connect_socket test #16987

Open
wants to merge 1 commit into
base: PHP-8.2
Choose a base branch
from

Conversation

bukka
Copy link
Member

@bukka bukka commented Nov 29, 2024

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)

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants