Skip to content

Add some missing tests to the test setting of Bazel for Ruby#19870

Closed
y-yagi wants to merge 3 commits intoprotocolbuffers:mainfrom
y-yagi:add_some_missing_tests
Closed

Add some missing tests to the test setting of Bazel for Ruby#19870
y-yagi wants to merge 3 commits intoprotocolbuffers:mainfrom
y-yagi:add_some_missing_tests

Conversation

@y-yagi
Copy link
Copy Markdown
Contributor

@y-yagi y-yagi commented Jan 5, 2025 •
edited
Loading

I'm not sure why, but it seems that some tests are not running via Bazel. CI uses Bazel. So I think we should add those to test correctly.

Also, I fixed some tests to pass.

  • Run memory tests against CRuby native implementation
  • Run service options extension test against CRuby native implementation
    • It seems that JRuby doesn't support this so far.
    • And, service options are frozen objects. But, in CRuby FFI, can't get a Message instance if the message is frozen. So this doesn't work.
    • So the service options extension test only passes in CRuby native.

@y-yagi y-yagi requested a review from a team as a code owner January 5, 2025 07:34
@y-yagi y-yagi requested review from ericsalo and removed request for a team January 5, 2025 07:34
@JasonLunn JasonLunn added ruby 🅰️ safe for tests Mark a commit as safe to run presubmits over labels Jan 6, 2025
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 6, 2025
@JasonLunn JasonLunn added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 6, 2025
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 6, 2025
@JasonLunn
Copy link
Copy Markdown
Contributor

JasonLunn commented Jan 6, 2025

Seeing new failures that differ across configurations:

On Ruby 3.3 FFI (but not when running without FFI):

Error: test_service_options_extensions(ServiceTest): NoMethodError: undefined method `int_option_value' for nil
/workspace/_build/out/sandbox/processwrapper-sandbox/995/execroot/com_google_protobuf/bazel-out/k8-fastbuild/bin/ruby/tests/service_test.runfiles/com_google_protobuf/ruby/tests/service_test.rb:37:in `test_service_options_extensions'
     34: 
     35:   def test_service_options_extensions
     36:     extension_field = Google::Protobuf::DescriptorPool.generated_pool.lookup('service_test_protos.test_options')
  => 37:     assert_equal 8325, extension_field.get(@test_service.options).int_option_value
     38:   end
     39: 
     40:   def test_method_options

On JRuby (with and without FFI):

FAIL: //ruby/tests:memory_test (see /workspace/_build/out/execroot/com_google_protobuf/bazel-out/k8-fastbuild/testlogs/ruby/tests/memory_test/test.log)
INFO: From Testing //ruby/tests:memory_test:
==================== Test output for //ruby/tests:memory_test:
/workspace/_build/out/external/protobuf_bundle/lib/jruby/3.1.0/gems/power_assert-2.0.5/lib/power_assert.rb:9: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
/workspace/_build/out/external/protobuf_bundle/lib/jruby/3.1.0/gems/power_assert-2.0.5/lib/power_assert.rb:9: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
NameError: uninitialized constant Google::Protobuf::Internal::SIZEOF_LONG
  const_missing at org/jruby/RubyModule.java:4331
         <main> at /workspace/_build/out/sandbox/processwrapper-sandbox/776/execroot/com_google_protobuf/bazel-out/k8-fastbuild/bin/ruby/tests/memory_test.runfiles/com_google_protobuf/ruby/tests/memory_test.rb:10

and

AIL: //ruby/tests:service_test (see /workspace/_build/out/execroot/com_google_protobuf/bazel-out/k8-fastbuild/testlogs/ruby/tests/service_test/test.log)
INFO: From Testing //ruby/tests:service_test:
==================== Test output for //ruby/tests:service_test:
/workspace/_build/out/external/protobuf_bundle/lib/jruby/3.1.0/gems/power_assert-2.0.5/lib/power_assert.rb:9: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
/workspace/_build/out/external/protobuf_bundle/lib/jruby/3.1.0/gems/power_assert-2.0.5/lib/power_assert.rb:9: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
Loaded suite /workspace/_build/out/sandbox/processwrapper-sandbox/889/execroot/com_google_protobuf/bazel-out/k8-fastbuild/bin/ruby/tests/service_test.runfiles/com_google_protobuf/ruby/tests/service_test
Started
E
===============================================================================
Error: test_service_options_extensions(ServiceTest): Java::JavaLang::ClassCastException: class org.jruby.RubyNil cannot be cast to class com.google.protobuf.jruby.RubyFieldDescriptor (org.jruby.RubyNil is in module org.jruby.dist of loader 'app'; com.google.protobuf.jruby.RubyFieldDescriptor is in unnamed module of loader org.jruby.util.JRubyClassLoader @3d99d22e)

@y-yagi
Copy link
Copy Markdown
Contributor Author

y-yagi commented Jan 7, 2025

@JasonLunn
Thanks for your feedback!

I pushed some commits to fix the tests c451812 、bbfd9a0 、7d8d9d5

Could you review?

@JasonLunn JasonLunn added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 7, 2025
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 7, 2025
@y-yagi
Copy link
Copy Markdown
Contributor Author

y-yagi commented Jan 7, 2025

Hmm. Sorry, tests are sill failing. I will check.

@y-yagi y-yagi force-pushed the add_some_missing_tests branch from 7d8d9d5 to 586cfbf Compare January 10, 2025 10:02
@JasonLunn JasonLunn added 🅰️ safe for tests Mark a commit as safe to run presubmits over and removed wait for user action labels Jan 11, 2025
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 11, 2025
JasonLunn
JasonLunn reviewed Jan 11, 2025
View reviewed changes
Comment thread ruby/tests/service_test.rb Outdated
end

def test_service_options_extensions
omit "JRuby does not support service options extensions" if defined? JRUBY_VERSION && :NATIVE == Google::Protobuf::IMPLEMENTATIO
Copy link
Copy Markdown
Contributor

@JasonLunn JasonLunn Jan 11, 2025

Choose a reason for hiding this comment

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

IMPLEMENTATION?

Copy link
Copy Markdown
Contributor Author

@y-yagi y-yagi Jan 12, 2025

Choose a reason for hiding this comment

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

Thanks! I fixed 🙏

@y-yagi y-yagi force-pushed the add_some_missing_tests branch from 586cfbf to be27833 Compare January 12, 2025 02:28
@y-yagi
Copy link
Copy Markdown
Contributor Author

y-yagi commented Jan 12, 2025

@JasonLunn
Sorry for many times. I fixed the PR again and updated the summary of the PR. Could you review this when you have time? Thanks.

@JasonLunn JasonLunn added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 12, 2025
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 12, 2025
@y-yagi
Copy link
Copy Markdown
Contributor Author

y-yagi commented Jan 14, 2025

Hmm, it seems that unrelated tests(Python) have failed. I will rebase withe the latest code.

y-yagi added 3 commits January 14, 2025 16:43
@y-yagi
Add some missing tests to the test setting of Bazel
ce11f27 
I'm not sure why, but it seems that some tests are not running via Bazel.
CI uses Bazel. So I think we should add those to test correctly.
@y-yagi
Run memory tests against CRuby native implementation
8e6e60a 
These tests are for CRuby native implementation. So this doesn't work
with FFI and JRuby.

Ref: protocolbuffers#10291
@y-yagi
Run service options extension test against CRuby native implementation
d51158d 
It seems that JRuby doesn't support this so far.

And, service options are frozen objects. But, in CRuby FFI, can't
get a Message instance if the message is frozen. So this doesn't work.
https://github.com/protocolbuffers/protobuf/blob/d406cae0138f4f1f283a22be9474bef1f48bb0dc/ruby/lib/google/protobuf/ffi/message.rb#L419

So the service options extension test only passes in CRuby native.
@y-yagi y-yagi force-pushed the add_some_missing_tests branch from be27833 to d51158d Compare January 14, 2025 07:43
@JasonLunn JasonLunn added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 14, 2025
@github-actions github-actions Bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 14, 2025
@y-yagi y-yagi deleted the add_some_missing_tests branch January 31, 2025 04:28
@y-yagi y-yagi mentioned this pull request Jan 31, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JasonLunn JasonLunn JasonLunn left review comments

@ericsalo ericsalo Awaiting requested review from ericsalo ericsalo is a code owner automatically assigned from protocolbuffers/protobuf-ruby

+1 more reviewer

@acozzette acozzette acozzette approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ruby

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@y-yagi @JasonLunn @acozzette