THRIFT-5757 Unit tests for php lib #2942

sveneld · 2024-03-02T22:49:44Z

After running PHP unit tests for each pull request THRIFT-5756

Next step is writing test for php lib.

It will be several pull request for this task.

Did you create an Apache Jira ticket? [ (Request account here, not required for trivial changes)](https://issues.apache.org/jira/browse/THRIFT-5757)
If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
Did you squash your changes to a single commit? (not required, but preferred)
Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

sveneld · 2024-03-02T22:50:04Z

@pkvach take a look please

sveneld · 2024-03-02T22:52:45Z

lib/php/lib/Transport/TSSLSocket.php

+        if ($this->context_ === null) {
+            $this->context_ = stream_context_create();
+        } else {
+            $this->context_ = $context;


giving null to stream_socket_client as a context generate a warning, so it's better to create an empty context if it does not provided

sveneld · 2024-03-02T22:53:52Z

lib/php/lib/Transport/TSSLSocket.php

@@ -87,7 +92,8 @@ public function open()
            throw new TTransportException('Socket already connected', TTransportException::ALREADY_OPEN);
        }

-        if (empty($this->host_)) {


it does not work because ssl:// always added to hostname

sveneld · 2024-03-02T22:55:41Z

lib/php/lib/Transport/TSocket.php

@@ -253,7 +253,9 @@ public function open()
        if (function_exists('socket_import_stream') && function_exists('socket_set_option')) {
            // warnings silenced due to bug https://bugs.php.net/bug.php?id=70939
            $socket = @socket_import_stream($this->handle_);
-            @socket_set_option($socket, SOL_TCP, TCP_NODELAY, 1);
+            if ($socket !== false) {


previous function can return false, and in php 8.0 it trigger a type error, so i added a check.

In future it will be great to remove all @ and work correctly with all warning and error generated by stream functions

lib/php/lib/Transport/TSSLSocket.php

lib/php/test/Unit/Lib/Transport/Fixtures/TestStream.php

sveneld · 2024-03-03T20:24:55Z

I found a library https://github.com/php-mock/php-mock-phpunit so I will try to rewrite tests with better coverage and best readability

lib/php/test/Unit/Lib/Transport/TFramedTransportTest.php

pkvach · 2024-03-03T22:28:30Z

lib/php/test/Unit/Lib/Transport/TFramedTransportTest.php

+        $transport
+            ->expects($this->exactly(count($lowLevelTransportReadAllParams)))
+            ->method('readAll')
+            ->withConsecutive(...$lowLevelTransportReadAllParams)


withConsecutive() is deprecated and has been removed in PHPUnit 10.

I will read about it. Thanks

sebastianbergmann/phpunit#4026 i read this long discussion. As I understand it's possible to use something like taht

->with(self::callback(function (string $message) { static $i = 0; return match (++$i) { 1 => $message === 'Removing audio files started.', 2 => $message === "Removing audio files that older than '2018-01-01 00:00:00'.", }; }));

But maybe someone will offer solution which will be accepted by phpunit maintainers.
I prefer to leave using of this method till migration to php8, where we can use match, maybe to this moment we will have bettre replacement for withConsecutive

Okay, I agree to leave it as is for now. Thanks.

lib/php/test/Unit/Lib/Transport/TBufferedTransportTest.php

sveneld · 2024-03-03T23:01:22Z

build/docker/ubuntu-focal/Dockerfile

      re2c \
      composer

+RUN pecl install xdebug-3.1.1 && \
+      echo "zend_extension=xdebug.so" > /etc/php/7.4/cli/conf.d/20-xdebug.ini


for some docker builds was missed extensions and xdebug

Looks like there may be an issue here.
The Dockerfile installs PHP without specifying a version, so it will install the latest version available in the package repository. However, the Xdebug configuration is written to a specific PHP 7.4 directory. If the installed PHP version is not 7.4, the Xdebug configuration will not be applied because PHP won't be able to find it.

Same for other Dockerfiles as well.

It was easier than I thought :)

RUN apt-get install -y --no-install-recommends php-xdebug

@pkvach build will be updated here a8fb726

sveneld · 2024-03-03T23:02:39Z

lib/php/lib/Transport/TCurlClient.php

@@ -227,7 +228,6 @@ public function flush()
            register_shutdown_function(array('Thrift\\Transport\\TCurlClient', 'closeCurlHandle'));
            self::$curlHandle = curl_init();
            curl_setopt(self::$curlHandle, CURLOPT_RETURNTRANSFER, true);
-            curl_setopt(self::$curlHandle, CURLOPT_BINARYTRANSFER, true);


CURLOPT_BINARYTRANSFER This constant had no effect since PHP 5.1.2

build/docker/ubuntu-bionic/Dockerfile

lib/php/test/Unit/Lib/Transport/TSocketTest.php

Jens-G · 2024-03-05T23:48:01Z

Can we squash that bunch of commits please? If it is work in progress consider marking it as "draft". If it is otherwise unrelated PRs that you want to stay separated for some reason or another, do that.

sveneld · 2024-03-06T07:11:18Z

@pkvach currently I have a trouble when running all tests together

Thrift\Exception\TException : TPhpStream: Could not open php://input
 /opt/project/lib/php/lib/Transport/TPhpStream.php:62
 /opt/project/lib/php/test/Unit/Lib/Transport/TPhpStreamTest.php:67

i added var_dump(error_get_last()); and it showed me such error.

array(4) {
  'type' =>
  int(2)
  'message' =>
  string(89) "Zend OPcache can't be temporary enabled (it may be only disabled till the end of request)"
  'file' =>
  string(19) "Standard input code"
  'line' =>
  int(105)
}

Maybe you have any idea how to fix of how to debug it?

sveneld · 2024-03-06T07:12:49Z

Can we squash that bunch of commits please? If it is work in progress consider marking it as "draft". If it is otherwise unrelated PRs that you want to stay separated for some reason or another, do that.

Marked as a draft, when i will finish coverage of folder Transport i will squash all commits and mark pull request as ready for review.
Thanks.

pkvach · 2024-03-06T15:01:35Z

@sveneld Regarding the falling TPhpStreamTest

Looks like the issue is due to the fact that the fopen file resource created in the data provider is being closed before it can be used in the test.

To fix this, I think we can move the creation of the file resource into the test method itself.

I'm attaching an example solution with changes for testOpen()
TPhpStreamTest.php.zip

Maybe you'll have another option.

sveneld · 2024-03-07T07:07:46Z

To fix this, I think we can move the creation of the file resource into the test method itself.

You was right, moving creation of file resource into the test method helped to solve the trouble.

One more class left to cover with test in folder Transport and this pull request will be ready :)
But it is not totally finished test coverage, only 38% percent of code is covered with tests..

pkvach · 2024-03-07T18:41:49Z

Great progress! Thank you for all the good work you do.

sveneld · 2024-03-08T18:24:24Z

@pkvach @Jens-G ready for review

Folder Transport is covered by unit tests

sveneld · 2024-03-08T18:26:13Z

lib/php/lib/Transport/TSocketPool.php

- * installed, then these null functions will step in and act like cache
- * misses.
- */
-if (!function_exists('apcu_fetch')) {


move it inside class as private methods. Global redeclare is not very good.

sveneld · 2024-03-08T18:29:20Z

Update of docker images will be in #2937

pkvach · 2024-03-09T09:13:15Z

lib/php/test/Unit/Lib/Transport/TSocketPoolTest.php

+    protected function setUp(): void
+    {
+        #need to be defined before the TSocketPool class definition
+        $this->defineFunctionMock('Thrift\Transport', 'function_exists');


Suggested change

$this->defineFunctionMock('Thrift\Transport', 'function_exists');

self::defineFunctionMock('Thrift\Transport', 'function_exists');

pkvach · 2024-03-09T12:07:37Z

LGTM

sveneld · 2024-03-09T12:19:37Z

@Jens-G let's merge this pull request

sveneld commented Mar 2, 2024

View reviewed changes