Skip to content

Conversation

@headius
Copy link
Contributor

@headius headius commented Feb 19, 2025

This PR fixes all fixable test failures in the stringio test suite when running on JRuby with the Java-based extension.

@headius
Copy link
Contributor Author

headius commented Feb 19, 2025

@nobu Hello! Maybe you can help me figure out this last failure, because it has me stumped.

Failure: test_write_encoding_conversion(TestStringIO):
  
  1. [1/4] Assertion for "\"🍣\""
     | Encoding::CompatibilityError expected but nothing was raised.
  
  2. [2/4] Assertion for "\"あ🍣\""
     | Encoding::CompatibilityError expected but nothing was raised.
  
  3. [4/4] Assertion for "[\"a\", \"🍣\"]"
     | Encoding::CompatibilityError expected but nothing was raised.
  <false> is not true.
/Users/headius/work/jruby/lib/ruby/gems/shared/gems/test-unit-ruby-core-1.0.5/lib/core_assertions.rb:532:in 'assert'
/Users/headius/work/jruby/lib/ruby/gems/shared/gems/test-unit-ruby-core-1.0.5/lib/core_assertions.rb:766:in 'assert_all_assertions'
/Users/headius/work/stringio/test/stringio/test_stringio.rb:229:in 'test_write_encoding_conversion'

I've traced through the code in JRuby and CRuby and the first example here eventually gets to these lines in enc_compatible_latter:

https://github.com/ruby/ruby/blob/4ac75f6f6453bbf3c89f5b9ae02a03085b506ed5/encoding.c#L1086-L1087

At this point, str1 is the blank Windows-31J string inside the StringIO, and str2 is the UTF-8 "🍣" character. The test above returns enc2 because str1 is empty, Windows-31J is not ASCII-compatible, and str2 is not all 7-bit ASCII.

But test_write_encoding_conversion wants this to fail! I can't figure out why or how it fails on CRuby because it seems to follow the path I describe, which will always return UTF-8 and not error.

@headius
Copy link
Contributor Author

headius commented Feb 19, 2025

@nobu After teaching myself how to step-debug CRuby again I figured out the problem: my version always passed str1 as the first argument to rb_enc_check. This allowed enc2 to be chosen if str1 was blank during enc_compatible_latter. This is the cause of all three failures.

I have a hack coming to duplicate this behavior in StringIO for JRuby until JRuby can add a version of rb_enc_check that can accept an encoding for the first argument.

The version of this method in JRuby does not support passing an
encoding for the first argument, which means downstream code in
enc_compatible_latter will not reject cases StringIO is expected
to reject. This patch hacks a version of rb_enc_check that works
with current JRuby versions and exhibits the behavior required.

A future JRuby update will improve this API and this hack should
become a fallback at that point (for older JRuby versions).
@headius
Copy link
Contributor Author

headius commented Feb 19, 2025

Oddly enough, the JRuby on macos job failed so I only added JRuby on windows to CI. I have filed jruby/jruby#8642 for the issue.

@nobu @hsbt This can be merged and released any time, let me know if you have any comments.

@headius
Copy link
Contributor Author

headius commented Feb 19, 2025

I just discovered some regressions and additional failures I can fix. Please wait to merge.

headius added a commit to headius/jruby that referenced this pull request Feb 19, 2025
The stringio library depends on being able to pass an encoding
only as the first argument to rb_enc_check, which causes the
downstream enc_compatible_latter to reject incompatible encodings
regardless of the first string being blank.

Part of fixes to pass stringio tests in ruby/stringio#116.
headius added a commit to headius/stringio that referenced this pull request Feb 20, 2025
Specs trigger off the Ruby version currently running, assuming
that the stringio version will be that which is typically shipped
at that Ruby version level. However the gem can be upgraded, and
will be upgraded in JRuby master (3.1 compat) once
ruby#116 is merged and released, so support both versions
of behavior here.
String data might be shared, so modify to ensure we have a writable
version.
This method is added to JRuby in 9.4.13.0 and avoids the overhead
of creating a fake CodeRangeable.

See jruby/jruby#8643
@headius
Copy link
Contributor Author

headius commented Feb 20, 2025

I have pushed the remaining fixes needed to have all tests and all specs green. Everything here and in ruby/spec will be green once jruby/jruby#8643 has been merged to JRuby master and deployed as jruby-head. I tried to move that process along, but it may take another day to settle.

Once everything runs green this can be merged and released.

@headius headius merged commit 10e94d6 into ruby:master Feb 20, 2025
48 checks passed
@headius headius deleted the stringio_green branch February 20, 2025 01:06
@headius
Copy link
Contributor Author

headius commented Feb 20, 2025

@hsbt @nobu I have merged the fixes in and we can release any time. Let me know if I should do that.

@kou
Copy link
Member

kou commented Feb 20, 2025

I'll release a new version in a few days.

@kou
Copy link
Member

kou commented Feb 20, 2025

Done.

@headius
Copy link
Contributor Author

headius commented Feb 20, 2025

@kou Thank you!

headius added a commit to headius/jruby that referenced this pull request Feb 20, 2025
This updates stringio to 3.1.4, which includes all the fixes from
ruby/stringio#116. All CRuby tests and ruby/spec specs for stringio
are now green.

Tests are from v3.1.4 of the ruby/stringio repo.
headius added a commit to headius/jruby that referenced this pull request Feb 21, 2025
This updates stringio to 3.1.5, which includes all the fixes from
ruby/stringio#116 and the regression ruby/stringio#119 fixed by
ruby/stringio#121. All CRuby tests and ruby/spec specs for stringio
are now green.

Tests are from v3.1.5 of the ruby/stringio repo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants