add a new encoding test

[jvshahid]

add a new test to make sure that changing encoding of ASCII substrings that originated from binary strings doesn't fail. related pr and jira issues:

1. fix encoding of substrings when the original string contains binary (i.e. > 0x7f) bytes. jruby/jruby#223
2. https://github.com/rubyspec/rubyspec/pull/146
3. http://jira.codehaus.org/browse/JRUBY-6764

Note that i initially sent a pr to the rubyspec guys, but they aren't very responsive.

[nobu]

What's the intension?
The upper half seems like a test for Array#pack.
The rest doesn't exist already?

[jvshahid]

this test demonstrates a bug in jruby, in which certain ascii strings can
end up as a view in a larger binary buffer. changing the encoding will
traverse the entire buffer checking for binary characters (i.e. > 0x7f)
instead of traversing the view. please let me know if that wasn't clear.
the links may give you a better idea as well.
On Jul 21, 2012 8:41 PM, "Nobuyoshi Nakada" <
reply@reply.github.com>
wrote:

What's the intension?
The upper half seems like a test for Array#pack.
The rest doesn't exist already?

---

Reply to this email directly or view it on GitHub:
#148 (comment)

[nurse]

CRuby's test won't merge regression tests originated from other implementations.
And on CRuby, such test should use ext/-test-/string/coderange.c

[headius]

Why not include regression tests found via bugs in other implementations? If there's a gap in MRI tests that don't exercise the behavior, then MRI is at risk of regressing as well.

JRuby uses MRI's suite as one of our primary test suites, and we would like to contribute back to that suite in the form of additional tests. I do not understand the reluctance to add tests where there may be gaps.

The situation right now is that JRuby wants to use a test suite that is shared with other implementations, but RubySpec folks aren't merging pull requests, and MRI folks are rejecting them. Does nobody want to share test suites but us?

[tenderlove]

@nobu I can't seem to find any tests for this case. I've refactored the test to not look like a pack test. I think the intention is to test the behavior of String#slice + String#encode.

@nurse We should commit a test for this in order to define the behavior on MRI. Maybe you can find an existing test for this behavior?

Here's my updated patch:

diff --git a/test/ruby/test_string.rb b/test/ruby/test_string.rb
index 47f349c..0398aa4 100644
--- a/test/ruby/test_string.rb
+++ b/test/ruby/test_string.rb
@@ -1940,6 +1940,18 @@ class TestString < Test::Unit::TestCase
     assert_raise(Encoding::CompatibilityError) { "\u3042".encode("ISO-2022-JP").rstrip }
   end

+  def test_encoding_of_ascii_originating_from_binary
+    binary_string = [0x82, 0x66, 0x6f, 0x6f]
+
+    ascii_string  = binary_string.pack('C4').slice(1, 3)
+    assert_equal "foo", ascii_string
+    assert_equal "ASCII-8BIT", ascii_string.encoding.name
+
+    utf8_string   = ascii_string.encode("UTF-8")
+    assert_equal "foo", utf8_string
+    assert_equal "UTF-8", utf8_string.encoding.name
+  end
+
 =begin
   def test_symbol_table_overflow
     assert_in_out_err([], <<-INPUT, [], /symbol table overflow \(symbol [a-z]{8}\) \(RuntimeError\)/)

[nurse]

This test is not atomic; is join test.
As I said, on CRuby this test can be written with coderange.so and it can be atomic.
It should be assertions for ascii_string and checks the coderange and the encoding of it.

[tenderlove]

@nurse I don't understand what you mean by "atomic". Can you show me?

This test is supposed to be testing copy-on-write behavior of the string. Where calling slice doesn't create a new character array, and the bytes outside the new Ruby object range are not taken in to account when calling encode.

Here is a diagram illustrating what this code is trying to test:

[nobu]

Is "without raising an exception" the important part?
Then the test should be:

  def test_encoding_of_ascii_originating_from_binary
    binary_string = [0x82, 0x66, 0x6f, 0x6f]

    ascii_string  = binary_string.pack('C4').slice(1, 3)
    assert_equal("foo", ascii_string)
    assert_equal(Encoding::ASCII_8BIT, ascii_string.encoding)
    utf8_string = nil
    assert_nothing_raised("JRUBY-6764") do
      utf8_string = ascii_string.encode(Encoding::UTF_8)
    end
    assert_equal("foo", utf8_string)
    assert_equal(Encoding::UTF_8, utf8_string.encoding)
  end

[nurse]

@nurse I don't understand what you mean by "atomic". Can you show me?

This test is supposed to be testing copy-on-write behavior of the string. Where calling slice doesn't create a new character array, and the bytes outside the new Ruby object range are not taken in to account when calling encode.

In other words, I can't understand what is tested by the test.
Or it seems not a simplest reproducible program.
If it tests String#encode for shared string, it should describe it.
I couldn't find it by the test.

[tenderlove]

OK. @nurse where do we test the behavior of CoW strings?

@jvshahid can you rewrite the test like @nobu suggests and send another PR? This is a test that defines the behavior of encode when combined with CoW strings, so please don't include the "JRUBY" string in your assertion though.

Thanks! ❤️

[nurse]

OK. @nurse where do we test the behavior of CoW strings?

This seems a test for String#encode, so this should be in test/ruby/test_transcode.rb.
When creating a CoW string in this test, it should be clear that the code does it by C ext or a method named make_cow_string (it is OK the implementation uses pack or some hacky code).

[jvshahid]

I made the changes on my branch, it's a new commit that moves the test and make it look like what @tenderlove and @nobu suggested. can someone please reopen the pull request.

thanks,

[nobu]

Does JRuby make shorter strings also shared?
4bytes string wouldn't be shared in CRuby, I guess.

[headius]

We do not have a lower size limit for COW sharing (except for single-byte, for which we have a set of preallocated byte[1]).

[tenderlove]

Ah yes, @nobu is right. @jvshahid can you make the string longer?

Unfortunately, I'm not a collaborator, so I cannot reopen. @nobu or @nurse would you mind reopening this PR? Thank you!

[shyouhei]

@tenderlove welcome. You are now a ruby/ruby team member. Feel free to reopen this.

[tenderlove]

@shyouhei thank you! ❤️ I will reopen this.

[tenderlove]

Merged in r36808