Skip to content

THRIFT-5951: Add PHP runtime coverage tests#3405

Open
sveneld wants to merge 1 commit intoapache:masterfrom
sveneld:THRIFT-5951
Open

THRIFT-5951: Add PHP runtime coverage tests#3405
sveneld wants to merge 1 commit intoapache:masterfrom
sveneld:THRIFT-5951

Conversation

@sveneld
Copy link
Copy Markdown
Contributor

@sveneld sveneld commented Apr 17, 2026

Summary

  • add 12 PHP runtime coverage tests for previously uncovered classes in lib/php/lib
  • cover TBase, TProtocol skip paths, TMultiplexedProcessor, StoredMessageProtocol, base server/transport abstractions, JSON/SimpleJSON helper contexts, TApplicationException, and constant/type classes
  • keep production PHP runtime code unchanged

Notes

  • JIRA: THRIFT-5951
  • this PR adds tests only
  • no third-party dependencies were added
  • LICENSE and NOTICE are unchanged

Validation

  • docker run --rm -v "$PWD:/src" -w /src php:8.3-cli php vendor/bin/phpunit -c lib/php/phpunit.xml --filter '/(TBaseTest|TProtocolTest|TMultiplexedProcessorTest|StoredMessageProtocolTest|TServerTest|TServerTransportTest|TTransportTest|JsonContextTest|SimpleJsonContextTest|TApplicationExceptionTest|ExceptionTypesTest|TypeConstantsTest)/' lib/php/test/Unit/Lib
  • docker run --rm -v "$PWD:/src" -w /src php:8.3-cli sh -lc 'for f in lib/php/test/Unit/Lib/Base/TBaseTest.php lib/php/test/Unit/Lib/Protocol/TProtocolTest.php lib/php/test/Unit/Lib/TMultiplexedProcessorTest.php lib/php/test/Unit/Lib/StoredMessageProtocolTest.php lib/php/test/Unit/Lib/Server/TServerTest.php lib/php/test/Unit/Lib/Server/TServerTransportTest.php lib/php/test/Unit/Lib/Transport/TTransportTest.php lib/php/test/Unit/Lib/Protocol/JsonContextTest.php lib/php/test/Unit/Lib/Protocol/SimpleJsonContextTest.php lib/php/test/Unit/Lib/Exception/TApplicationExceptionTest.php lib/php/test/Unit/Lib/Exception/ExceptionTypesTest.php lib/php/test/Unit/Lib/Type/TypeConstantsTest.php; do php -l "$f" || exit 1; done'

Generated-by: Codex GPT-5.4

@mergeable mergeable bot added c++ kotlin php python build and general CI cmake, automake and build system changes labels Apr 17, 2026
@sveneld sveneld force-pushed the THRIFT-5951 branch 2 times, most recently from 967d949 to 4357726 Compare April 17, 2026 12:58
Client: php

Generated-by: Codex GPT-5.4
@kpumuk kpumuk removed c++ python kotlin build and general CI cmake, automake and build system changes labels Apr 17, 2026
$transport->expects($this->exactly(2))
->method('readAll')
->withConsecutive([4], [2])
->willReturnOnConsecutiveCalls(pack('N', 2), '12');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is something weird going on here with mocks. readAll returns strings, but skipBinary() does math on it:

return 4 + $itrans->readAll($len);

In this case since we return '12' as a string, 4 + '12' will be 16. Curious how does it even work...

Copy link
Copy Markdown
Member

@kpumuk kpumuk Apr 17, 2026

Choose a reason for hiding this comment

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

What happens if you return 'aa' instead of '12'?

Edit:

There was 1 error:

1) Test\Thrift\Unit\Lib\Protocol\TProtocolTest::testSkipBinarySkipsPrimitiveAndContainerValues
TypeError: Unsupported operand types: int + string

/thrift/src/lib/php/lib/Protocol/TProtocol.php:289
/thrift/src/lib/php/test/Unit/Lib/Protocol/TProtocolTest.php:190

ERRORS!
Tests: 222, Assertions: 462, Errors: 1, Skipped: 5.

I think we have a bug in the protocol implementation, and that the test does not actually test the right thing due to mocks and inputs creatively crafted around the bug.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants