You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "kpumuk (via GitHub)" <gi...@apache.org> on 2025/12/17 01:37:49 UTC
[PR] Fixed make check for Ruby library [thrift]
kpumuk opened a new pull request, #3270:
URL: https://github.com/apache/thrift/pull/3270
<!-- Explain the changes in the pull request below: -->
I am not yet quite sure what value does https://github.com/apache/thrift/pull/2223 bring, but since it was merged it is breaking `make -C lib/rb check`:
* It uses `test-unit` library, while `lib/rb` only has RSpec — missing gem requirement
* It tries to load `thrift` from global, while it is not installed
This MR makes the tests pass for now.
Other issues which I did not try to address are:
* `test-unit` vs RSpec - why two testing libraries in the same project? I know `test-unit` is used for `test/rb`, but that is a separate project within the monorepo
* code formatting is extremely off. Time to introduce automated linters?
* Does it bring any value compared to existing specs? Not quite sure, the test suite is large, will need to compare
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [ ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? ([Request account here](https://selfserve.apache.org/jira-account.html), not required for trivial changes)
- [ ] 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.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
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: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5909: Fixed make check for Ruby library [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #3270:
URL: https://github.com/apache/thrift/pull/3270#discussion_r2637374915
##########
lib/rb/lib/thrift/protocol/compact_protocol.rb:
##########
@@ -27,7 +27,7 @@ class CompactProtocol < BaseProtocol
TYPE_BITS = 0x07
TYPE_SHIFT_AMOUNT = 5
- TSTOP = ["", Types::STOP, 0]
+ TSTOP = [nil, Types::STOP, 0]
Review Comment:
See https://github.com/apache/thrift/blob/master/lib/rb/ext/compact_protocol.c#L464
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5909: Fixed make check for Ruby library [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on PR #3270:
URL: https://github.com/apache/thrift/pull/3270#issuecomment-3678117696
Test results after the change:
```
$ make -C lib/rb check
<skipped>
Finished in 5.96 seconds (files took 0.25745 seconds to load)
448 examples, 0 failures, 1 pending
$ make -C test/rb check
<skipped>
Finished in 0.0091145 seconds.
--------------------------------------------------------------------------------------------------------------------------------------------
16 tests, 78 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
--------------------------------------------------------------------------------------------------------------------------------------------
1755.44 tests/s, 8557.79 assertions/s
```
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5909: Fixed make check for Ruby library [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #3270:
URL: https://github.com/apache/thrift/pull/3270#discussion_r2637374915
##########
lib/rb/lib/thrift/protocol/compact_protocol.rb:
##########
@@ -27,7 +27,7 @@ class CompactProtocol < BaseProtocol
TYPE_BITS = 0x07
TYPE_SHIFT_AMOUNT = 5
- TSTOP = ["", Types::STOP, 0]
+ TSTOP = [nil, Types::STOP, 0]
Review Comment:
See https://github.com/apache/thrift/blob/master/lib/rb/ext/compact_protocol.c#L464
```c
return rb_ary_new3(3, Qnil, INT2FIX(0), INT2FIX(0));
```
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5909: Fixed make check for Ruby library [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G merged PR #3270:
URL: https://github.com/apache/thrift/pull/3270
--
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: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5909: Fixed make check for Ruby library [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on PR #3270:
URL: https://github.com/apache/thrift/pull/3270#issuecomment-3678115067
After a careful consideration, I decided to rewrite the tests merged in https://github.com/apache/thrift/pull/2223, and move them from `lib/rb` to `test/rb`. Here is why:
* These are more of a protocol integration tests, and act as a pre-check for the cross-tests
* They depend on test-unit, which is the framework used in `test/rb`
Original architecture depended on `fork` call, which does not cooperate with test-unit. For example, compact protocol test was failing in the client on non-accelerated builds (Ruby version returned `""` as the first element, while binary versions returns `nil`). The tests never picked that because exit code was coming from the client, which only writes and has no assertions, so never fails. Additionally, artificial delay of 2 seconds was injected into every test to coordinate between server and client.
Instead, the new approach uses threads and a queue to signal to the client when server is ready. It correctly asserts in the main threads and so the exit code is always correct.
```
============================================================================================================================================
Failure: test_different_data_types(TestCompactProtocol)
core/protocol/test_compact_protocol.rb:177:in 'TestCompactProtocol#test_different_data_types'
174: assert_equal("[6, 8, 12]", "#{server_results[:acc_map1]}")
175: assert_equal("[0, 0, 0]", "#{server_results[:acc_map2]}")
176: assert_equal("[8, 5]", "#{server_results[:acc_set]}")
=> 177: assert_equal("[nil, 6, 5]", "#{server_results[:acc_field1]}")
178: assert_equal("[nil, 0, 0]", "#{server_results[:acc_field2]}")
179: end
180: end
<"[nil, 6, 5]"> expected but was
<"[\"\", 6, 5]">
diff:
? [nil, 6, 5]
? ""
? ???
============================================================================================================================================
Finished in 0.01072975 seconds.
--------------------------------------------------------------------------------------------------------------------------------------------
1 tests, 16 assertions, 1 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
0% passed
```
The change in the `Bytes` was made to address a warning in Ruby 3.4+:
```
/code/lib/rb/lib/thrift/bytes.rb:35: warning: literal string will be frozen in the future (run with --debug-frozen-string-literal for more information)
```
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-5909: Fixed make check for Ruby library [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #3270:
URL: https://github.com/apache/thrift/pull/3270#discussion_r2637375138
##########
lib/rb/lib/thrift/protocol/compact_protocol.rb:
##########
@@ -276,7 +276,7 @@ def read_field_begin
# push the new field onto the field stack so we can keep the deltas going.
@last_field.push(field_id)
- ["", CompactTypes.get_ttype(type & 0x0f), field_id]
+ [nil, CompactTypes.get_ttype(type & 0x0f), field_id]
Review Comment:
See https://github.com/apache/thrift/blob/master/lib/rb/ext/compact_protocol.c#L488:
```c
return rb_ary_new3(3, Qnil, INT2FIX(get_ttype(type & 0x0f)), INT2FIX(field_id));
```
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org