Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Rails 5.2.3 test cases #558

Merged
merged 5 commits into from
Oct 8, 2019
Merged

Conversation

aried3r
Copy link
Contributor

@aried3r aried3r commented Oct 3, 2019

This is part of #535. Locally I had a test failure but I hope that this won't be the case on Travis.

Edit: Fixes one test case, but another is failing, which is a new test case that wasn't in 5.0.2 (the previous version of tests).
Here's the bug description in Rails:
rails/rails#26132
And the PR that fixed it:
rails/rails#30953

How would you like to proceed in this case? Is it something that Oj should handle?

@aried3r
Copy link
Contributor Author

aried3r commented Oct 3, 2019

So the same tests fail on Travis, sadly I have no idea why.

https://travis-ci.org/ohler55/oj/jobs/592965333#L397-L410

Failure:
TestJSONEncoding#test_utf8_string_encoded_properly [/home/travis/build/ohler55/oj/test/activesupport5/encoding_test.rb:77]:
Expected: "\"€2.99\""
  Actual: "\"\\u20ac2.99\""
bin/rails test home/travis/build/ohler55/oj/test/activesupport5/encoding_test.rb:75

Failure:
TestJSONEncoding#test_to_json_works_on_io_objects [/home/travis/build/ohler55/oj/test/activesupport5/encoding_test.rb:466]:
Expected: "\"#<IO:0x00000000018d41d0>\""
  Actual: "null"
bin/rails test home/travis/build/ohler55/oj/test/activesupport5/encoding_test.rb:465

It's this line

assert_equal '"€2.99"', result

and this one
assert_equal STDOUT.to_s.to_json, STDOUT.to_json

@aried3r
Copy link
Contributor Author

aried3r commented Oct 3, 2019

According to hexdump, it's using the correct UTF-8 encoding. But the test case suggests it's UTF-16. I did accidentally remove the # encoding: UTF-8 header, but I don't think it should matter. At least locally, it doesn't make a difference (the test case still fails with the header).

UTF-8 is 0xE2 0x82 0xAC (e282ac)
UTF-16 is 0x20AC (20ac) according to:
https://www.fileformat.info/info/unicode/char/20ac/index.htm

$ hexdump -C test/activesupport5/encoding_test.rb | rg "e2 82[[:space:]]+ac" -A2
00000a90  2e 65 6e 63 6f 64 65 28  22 e2 82 ac 32 2e 39 39  |.encode("...2.99|
00000aa0  22 29 0a 20 20 20 20 61  73 73 65 72 74 5f 65 71  |").    assert_eq|
00000ab0  75 61 6c 20 27 22 e2 82  ac 32 2e 39 39 22 27 2c  |ual '"...2.99"',|
00000ac0  20 72 65 73 75 6c 74 0a  20 20 20 20 61 73 73 65  | result.    asse|
00000ad0  72 74 5f 65 71 75 61 6c  28 45 6e 63 6f 64 69 6e  |rt_equal(Encodin|

However, Ruby thinks this of the string. Encoding is UTF-8 but it prints the UTF-16 representation in hexadecimal. (20AC)

result = ActiveSupport::JSON.encode("€2.99")
pp result.encoding  #<Encoding:UTF-8>
pp "€2.99".encoding #<Encoding:UTF-8>
pp "€2.99".chars.map { |c| puts "%s %3d %02X" % [ c, c.ord, c.ord ] }
# € 8364 20AC
# 2  50 32
# .  46 2E
# 9  57 39
# 9  57 39

The same happens if I force the encoding using:

"€2.99".dup.force_encoding('UTF-8')

@aried3r
Copy link
Contributor Author

aried3r commented Oct 3, 2019

Of course the answer is much easier. I was just not careful enough copying the test cases.

I was missing:

    # The original test seems to expect that
    # ActiveSupport.escape_html_entities_in_json reverts to true even after
    # being set to false. I haven't been able to figure that out so the value is
    # set to true, the default, before running the test. This might be wrong but
    # for now it will have to do.
    ActiveSupport.escape_html_entities_in_json = true

Comment on lines +7 to +10
require_relative "abstract_unit"
require "active_support/json"
require "active_support/time"
require_relative "time_zone_test_helpers"
Copy link
Owner

Choose a reason for hiding this comment

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

I think the Oj code needs to be after this or else the active support decoder is used. Have you checked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that's a good point! I do remember a way to check if Oj is being used, but can't find it in the docs atm. Could you maybe point me into the right direction? Then I can also add a test case for it :)

Copy link
Owner

Choose a reason for hiding this comment

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

You can turn on tracing but that doesn't help for testing. How about checking the ActiveSupport::json_encoder or is it ActiveSupport.json_encoder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I can test this in the encoding test, but there is no json_decoder, so the test fails (I added it anyway, to see what I tried).

@ohler55 ohler55 merged commit 9ae66cd into ohler55:develop Oct 8, 2019
@ohler55
Copy link
Owner

ohler55 commented Oct 8, 2019

Thanks for the help getting Oj to pass the more current tests.

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