Copilot commented on code in PR #3634:
URL: https://github.com/apache/thrift/pull/3634#discussion_r3566550582
##########
test/tests.json:
##########
@@ -444,6 +444,9 @@
"ruby",
"../integration/TestClient.rb",
"--"
+ ],
+ "transports": [
+ "http"
]
Review Comment:
The Ruby client is now restricted to HTTP (client.transports = ["http"]) but
it still inherits the top-level sockets list including "domain". This generates
unsupported http+domain combinations (the Ruby client explicitly errors in
TestClient.rb) and forces a large number of entries into known_failures. Define
client.sockets for rb to only include IP sockets (similar to how php/dart
clients are configured) so these invalid test cases are not generated.
##########
test/known_failures_Linux.json:
##########
@@ -125,6 +125,20 @@
"cpp-py_multih_http-domain",
"cpp-py_multij-json_http-domain",
"cpp-py_multij_http-domain",
+ "cpp-rb_binary-accel_http-domain",
+ "cpp-rb_binary_http-domain",
+ "cpp-rb_compact_http-domain",
+ "cpp-rb_header_http-domain",
+ "cpp-rb_json_http-domain",
+ "cpp-rb_multi-accel_http-domain",
+ "cpp-rb_multi-binary_http-domain",
+ "cpp-rb_multi_http-domain",
+ "cpp-rb_multic-compact_http-domain",
+ "cpp-rb_multic_http-domain",
+ "cpp-rb_multih-header_http-domain",
+ "cpp-rb_multih_http-domain",
+ "cpp-rb_multij-json_http-domain",
+ "cpp-rb_multij_http-domain",
Review Comment:
This file adds many known-failure entries for *_http-domain combinations
involving the Ruby client. Since the Ruby HTTP client rejects --domain-socket
(and tests.json supports per-client sockets), these should be prevented by
configuration rather than permanently expanding known_failures. After
restricting the rb client sockets to ip/ip-ssl in test/tests.json, these
http-domain known-failure entries should be removed to keep the known-failure
list meaningful.
##########
test/known_failures_Linux.json:
##########
@@ -934,6 +967,66 @@
"py-rs_multic_buffered-ip",
"py-rs_multic_framed-domain",
"py-rs_multic_framed-ip",
+ "rb.falcon-java_accel-binary_http-ip",
+ "rb.falcon-java_accel-binary_http-ip-ssl",
+ "rb.falcon-java_binary_http-ip",
+ "rb.falcon-java_binary_http-ip-ssl",
+ "rb.falcon-java_compact_http-ip",
+ "rb.falcon-java_compact_http-ip-ssl",
+ "rb.falcon-java_json_http-ip",
+ "rb.falcon-java_json_http-ip-ssl",
+ "rb.falcon-java_multi-binary_http-ip",
+ "rb.falcon-java_multi-binary_http-ip-ssl",
+ "rb.falcon-java_multi_http-ip",
+ "rb.falcon-java_multi_http-ip-ssl",
+ "rb.falcon-java_multic-compact_http-ip",
+ "rb.falcon-java_multic-compact_http-ip-ssl",
+ "rb.falcon-java_multic_http-ip",
+ "rb.falcon-java_multic_http-ip-ssl",
+ "rb.falcon-java_multij-json_http-ip",
+ "rb.falcon-java_multij-json_http-ip-ssl",
+ "rb.falcon-java_multij_http-ip",
+ "rb.falcon-java_multij_http-ip-ssl",
Review Comment:
This adds a large block of known-failure entries for Ruby HTTP servers
(rb.thin/rb.puma/rb.falcon) when paired with the Java client over HTTP/HTTPS.
Since the PR description says the integration harness now exercises these
servers, masking essentially all Java↔Ruby HTTP server coverage in CI
undermines the stated goal. It would be better to identify and fix the
interoperability issue(s) (or narrow the known-failures to the specific failing
cases) so at least one maintained Rack server path is actually validated
cross-language.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
