THRIFT-5951: Improve PHP library unit test coverage

lib/php/lib/Protocol/TCompactProtocol.php

@@ -702,8 +702,16 @@ public function readI64(&$value)
 
     public function writeI64($value)
     {
-        // If we are in an I32 range, use the easy method below.
-        if (($value > 4294967296) || ($value < -4294967296)) {
+        if ($value === PHP_INT_MIN) {
+            // PHP_INT_MIN (-2^63) cannot be safely negated: -PHP_INT_MIN overflows
+            // the 64-bit signed integer range. Its zigzag encoding is the maximum
+            // unsigned 64-bit varint (0xFFFFFFFFFFFFFFFF), so we write it directly.
+
+            $out = "\xff\xff\xff\xff\xff\xff\xff\xff\xff\x01";
+            $this->trans_->write($out, 10);
+
+            return 10;
+        } elseif (($value > 4294967296) || ($value < -4294967296)) {
             // Convert $value to $hi and $lo
             $neg = $value < 0;
 

lib/php/test/Unit/Lib/Exception/TExceptionTest.php

@@ -22,7 +22,11 @@
 namespace Test\Thrift\Unit\Lib\Exception;
 
 use PHPUnit\Framework\TestCase;
+use Test\Thrift\Unit\Lib\Fixture\TestRichException;
 use Thrift\Exception\TException;
+use Thrift\Protocol\TBinaryProtocol;
+use Thrift\Transport\TMemoryBuffer;
+use Thrift\Type\TType;
 
 class TExceptionTest extends TestCase
 {
@@ -57,4 +61,163 @@ public function testExceptionWithSpecAndVals()
         $this->assertEquals(123456, $exception->int);
         $this->assertEquals(true, $exception->bool);
     }
+
+    public function testExceptionWithDefaultParams()
+    {
+        $exception = new TException();
+        $this->assertSame('', $exception->getMessage());
+        $this->assertSame(0, $exception->getCode());
+    }
+
+    public function testExceptionSpecIgnoresUnsetVals()
+    {
+        $spec = [
+            ['var' => 'field1'],
+            ['var' => 'field2'],
+        ];
+        $vals = ['field1' => 'set'];
+
+        $exception = new TException($spec, $vals);
+        $this->assertEquals('set', $exception->field1);
+    }
+
+    /**
+     * @dataProvider writeAndReadFieldDataProvider
+     */
+    public function testWriteAndReadField($field, $value)
+    {
+        $exception = new TestRichException();
+        $exception->$field = $value;
+
+        $result = $this->roundtrip($exception);
+
+        $this->assertEquals($value, $result->$field);
+    }
+
+    public function writeAndReadFieldDataProvider()
+    {
+        // scalars
+        yield 'string' => ['field' => 'stringField', 'value' => 'hello world'];
+        yield 'int' => ['field' => 'intField', 'value' => 42];
+        yield 'bool true' => ['field' => 'boolField', 'value' => true];
+        yield 'bool false' => ['field' => 'boolField', 'value' => false];
+        yield 'double' => ['field' => 'doubleField', 'value' => 3.14];
+
+        // containers
+        yield 'map' => ['field' => 'mapField', 'value' => ['key1' => 100, 'key2' => 200]];
+        yield 'list' => ['field' => 'listField', 'value' => ['alpha', 'beta', 'gamma']];
+        yield 'set' => ['field' => 'setField', 'value' => [10 => true, 20 => true, 30 => true]];
+
+        // empty containers
+        yield 'empty map' => ['field' => 'mapField', 'value' => []];
+        yield 'empty list' => ['field' => 'listField', 'value' => []];
+        yield 'empty set' => ['field' => 'setField', 'value' => []];
+    }
+
+    public function testWriteAndReadAllFields()
+    {
+        $exception = new TestRichException();
+        $exception->stringField = 'test';
+        $exception->intField = 99;
+        $exception->boolField = true;
+        $exception->doubleField = 2.718;
+        $exception->mapField = ['a' => 1];
+        $exception->listField = ['x', 'y'];
+        $exception->setField = [5 => true];
+
+        $result = $this->roundtrip($exception);
+
+        $this->assertEquals('test', $result->stringField);
+        $this->assertEquals(99, $result->intField);
+        $this->assertTrue($result->boolField);
+        $this->assertEquals(2.718, $result->doubleField);
+        $this->assertEquals(['a' => 1], $result->mapField);
+        $this->assertEquals(['x', 'y'], $result->listField);
+        $this->assertEquals([5 => true], $result->setField);
+    }
+
+    public function testWriteSkipsNullFields()
+    {
+        $exception = new TestRichException();
+        $exception->stringField = 'only this';
+
+        $transport = new TMemoryBuffer();
+        $protocol = new TBinaryProtocol($transport);
+        $exception->write($protocol);
+
+        $result = new TestRichException();
+        $readTransport = new TMemoryBuffer($transport->getBuffer());
+        $readProtocol = new TBinaryProtocol($readTransport);
+        $result->read($readProtocol);
+
+        $this->assertEquals('only this', $result->stringField);
+        $this->assertNull($result->intField);
+        $this->assertNull($result->boolField);
+        $this->assertNull($result->mapField);
+        $this->assertNull($result->listField);
+        $this->assertNull($result->setField);
+    }
+
+    public function testReadSkipsUnknownField()
+    {
+        // Write a struct with field id=99 (unknown to our spec)
+        $transport = new TMemoryBuffer();
+        $protocol = new TBinaryProtocol($transport);
+
+        $protocol->writeStructBegin('Test');
+        $protocol->writeFieldBegin('unknown', TType::STRING, 99);
+        $protocol->writeString('should be skipped');
+        $protocol->writeFieldEnd();
+        $protocol->writeFieldBegin('stringField', TType::STRING, 1);
+        $protocol->writeString('known');
+        $protocol->writeFieldEnd();
+        $protocol->writeFieldStop();
+        $protocol->writeStructEnd();
+
+        $result = new TestRichException();
+        $readTransport = new TMemoryBuffer($transport->getBuffer());
+        $readProtocol = new TBinaryProtocol($readTransport);
+        $result->read($readProtocol);
+
+        $this->assertEquals('known', $result->stringField);
+    }
+
+    public function testReadSkipsMismatchedFieldType()
+    {
+        // Write field id=1 as I32 instead of STRING
+        $transport = new TMemoryBuffer();
+        $protocol = new TBinaryProtocol($transport);
+
+        $protocol->writeStructBegin('Test');
+        $protocol->writeFieldBegin('stringField', TType::I32, 1);
+        $protocol->writeI32(999);
+        $protocol->writeFieldEnd();
+        $protocol->writeFieldBegin('intField', TType::I32, 2);
+        $protocol->writeI32(42);
+        $protocol->writeFieldEnd();
+        $protocol->writeFieldStop();
+        $protocol->writeStructEnd();
+
+        $result = new TestRichException();
+        $readTransport = new TMemoryBuffer($transport->getBuffer());
+        $readProtocol = new TBinaryProtocol($readTransport);
+        $result->read($readProtocol);
+
+        $this->assertNull($result->stringField);
+        $this->assertEquals(42, $result->intField);
+    }
+
+    private function roundtrip(TestRichException $exception): TestRichException
+    {
+        $transport = new TMemoryBuffer();
+        $protocol = new TBinaryProtocol($transport);
+        $exception->write($protocol);
+
+        $result = new TestRichException();
+        $readTransport = new TMemoryBuffer($transport->getBuffer());
+        $readProtocol = new TBinaryProtocol($readTransport);
+        $result->read($readProtocol);
+
+        return $result;
+    }
 }

lib/php/test/Unit/Lib/Factory/TBinaryProtocolFactoryTest.php

@@ -71,4 +71,30 @@ public function getProtocolDataProvider()
             'strictWrite' => true,
         ];
     }
+
+    /**
+     * @return void
+     */
+    public function testGetTransport()
+    {
+        $transport = $this->createMock(TTransport::class);
+        $factory = new TBinaryProtocolFactory();
+        $protocol = $factory->getProtocol($transport);
+
+        $this->assertSame($transport, $protocol->getTransport());
+    }
+
+    /**
+     * @return void
+     */
+    public function testGetProtocolCreatesNewInstancePerCall()
+    {
+        $transport = $this->createMock(TTransport::class);
+        $factory = new TBinaryProtocolFactory();
+
+        $protocol1 = $factory->getProtocol($transport);
+        $protocol2 = $factory->getProtocol($transport);
+
+        $this->assertNotSame($protocol1, $protocol2);
+    }
 }

lib/php/test/Unit/Lib/Factory/TCompactProtocolFactoryTest.php

@@ -44,4 +44,30 @@ public function testGetProtocol()
 
         $this->assertSame($transport, $this->getPropertyValue($protocol, 'trans_'));
     }
+
+    /**
+     * @return void
+     */
+    public function testGetTransport()
+    {
+        $transport = $this->createMock(TTransport::class);
+        $factory = new TCompactProtocolFactory();
+        $protocol = $factory->getProtocol($transport);
+
+        $this->assertSame($transport, $protocol->getTransport());
+    }
+
+    /**
+     * @return void
+     */
+    public function testGetProtocolCreatesNewInstancePerCall()
+    {
+        $transport = $this->createMock(TTransport::class);
+        $factory = new TCompactProtocolFactory();
+
+        $protocol1 = $factory->getProtocol($transport);
+        $protocol2 = $factory->getProtocol($transport);
+
+        $this->assertNotSame($protocol1, $protocol2);
+    }
 }

lib/php/test/Unit/Lib/Factory/TFramedTransportFactoryTest.php

@@ -46,4 +46,31 @@ public function testGetTransport()
         $this->assertTrue($this->getPropertyValue($framedTransport, 'write_'));
         $this->assertSame($transport, $this->getPropertyValue($framedTransport, 'transport_'));
     }
+
+    /**
+     * @return void
+     */
+    public function testGetTransportWrapsInnerTransport()
+    {
+        $transport = $this->createMock(TTransport::class);
+        $factory = new TFramedTransportFactory();
+        $framedTransport = $factory->getTransport($transport);
+
+        $this->assertNotSame($transport, $framedTransport);
+        $this->assertInstanceOf(TFramedTransport::class, $framedTransport);
+    }
+
+    /**
+     * @return void
+     */
+    public function testGetTransportCreatesNewInstancePerCall()
+    {
+        $transport = $this->createMock(TTransport::class);
+        $factory = new TFramedTransportFactory();
+
+        $result1 = $factory->getTransport($transport);
+        $result2 = $factory->getTransport($transport);
+
+        $this->assertNotSame($result1, $result2);
+    }
 }

lib/php/test/Unit/Lib/Factory/TTransportFactoryTest.php

@@ -38,4 +38,22 @@ public function testGetTransport()
 
         $this->assertSame($transport, $result);
     }
+
+    /**
+     * @return void
+     */
+    public function testGetTransportCreatesNewInstancePerCall()
+    {
+        $factory = new TTransportFactory();
+
+        $transport1 = $this->createMock(TTransport::class);
+        $transport2 = $this->createMock(TTransport::class);
+
+        $this->assertSame($transport1, $factory->getTransport($transport1));
+        $this->assertSame($transport2, $factory->getTransport($transport2));
+        $this->assertNotSame(
+            $factory->getTransport($transport1),
+            $factory->getTransport($transport2)
+        );
+    }
 }

lib/php/test/Unit/Lib/Fixture/ProcessorSpy.php → lib/php/test/Unit/Lib/Fixture/TestProcessor.php

@@ -23,7 +23,7 @@
 
 use Thrift\Protocol\TProtocol;
 
-class ProcessorSpy
+class TestProcessor
 {
     public function process(TProtocol $input, TProtocol $output)
     {