Skip to content

THRIFT-2151: Add PHP persistent socket close regression coverage - #3796

Merged
sveneld merged 1 commit into
apache:masterfrom
sveneld:THRIFT-2151
Sep 11, 2026
Merged

THRIFT-2151: Add PHP persistent socket close regression coverage#3796
sveneld merged 1 commit into
apache:masterfrom
sveneld:THRIFT-2151

Conversation

@sveneld

@sveneld sveneld commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add regression coverage keeping TSocket::close() effective for persistent sockets (THRIFT-2151).
  • Fix existing socket tests to use valid constructor arguments and the deprecation capture record's errstr field.
  • Complete APCu mock responses in TSocketPoolTest, check that each expected fetch has a result, and expect the error log from the first failed connection attempt before a successful retry.
  • Align the BufferedReadTransport test fixture with the typed transport interface.
  • Continue running tests after warnings and return a failing exit code when warnings occur.

Why Include the Test Harness Fixes?

The upstream PHP 8.4 CI run after #3792 reported success after executing only 689 of 947 tests: stopOnWarning stopped the suite before the socket tests, while warnings did not cause a failing exit code. These test-only corrections let the persistent socket regression run as part of the full suite instead of hiding behind an earlier warning.

Testing

  • PHP 8.4 / PHPUnit 13.3.3 in Docker, with current Composer dependencies and freshly generated PHP fixtures.
  • Full suite: 948 tests, 2756 assertions, exit code 0; no errors, failures, or warnings. There are 5 skipped tests, 13 PHPUnit deprecations, and 6 PHPUnit notices.
  • TSocketPoolTest: 20 tests, 249 assertions, no warnings or failures.
  • An isolated warning probe verified both tests execute and PHPUnit returns exit code 1.
  • PHPCS passed for all changed PHP files; git diff --check passed.
  • Repository-wide make style is blocked by stale local Autotools configuration referencing the removed Swift directory.

Generated-by: OpenAI Codex GPT-5 noreply@openai.com

@mergeable mergeable Bot added the php label Sep 3, 2026
@sveneld
sveneld force-pushed the THRIFT-2151 branch 2 times, most recently from 7280d29 to a5bc934 Compare September 11, 2026 09:15
@sveneld
sveneld marked this pull request as ready for review September 11, 2026 09:42
Copilot AI lite review requested due to automatic review settings September 11, 2026 09:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The persistent-socket regression test does not yet verify closure of the underlying resource.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds PHP regression coverage for persistent socket closure and strengthens PHPUnit warning handling.

Changes:

  • Adds persistent TSocket::close() coverage and corrects socket test fixtures.
  • Completes socket-pool mocks and retry logging expectations.
  • Aligns transport fixture signatures.
  • Configures PHPUnit to continue after warnings while failing on them.
File summaries
File Description
lib/php/test/Unit/Lib/Transport/TSocketTest.php Updates socket tests and adds persistent close coverage.
lib/php/test/Unit/Lib/Transport/TSocketPoolTest.php Completes APCu responses and retry logging assertions.
lib/php/test/Unit/Lib/Transport/Fixture/BufferedReadTransport.php Adds typed transport method signatures.
lib/php/phpunit.xml Continues after warnings while failing the suite.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/php/test/Unit/Lib/Transport/TSocketTest.php
Client: php

Generated-by: OpenAI Codex GPT-5 <noreply@openai.com>
Copilot AI review requested due to automatic review settings September 11, 2026 09:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No unresolved blocking issues were identified, and the full test suite passes.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@sveneld
sveneld merged commit b04d876 into apache:master Sep 11, 2026
100 of 101 checks passed
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