Handle RSpec description with japanese char in CP932 encoded files #2575

benoittgt · 2018-10-19T11:32:00Z

When the user create a test with CP932 encoding and japanese chars RSpec
fail to properly display characters or crash with the error:

Encoding::CompatibilityError: incompatible character encodings: Windows-31J and UTF-8

Fix:

JonRowe · 2018-10-19T11:51:08Z

lib/rspec/core/formatters/exception_presenter.rb

@@ -244,6 +244,10 @@ def formatted_message_and_backtrace(colorizer)
          end
        end

+        def encoded_description(description)
+          encoded_string(description) if description


I think I'd prefer return if description.nil? to make it clear what we're doing with this here

Yep. Much better. Thanks Jon

JonRowe · 2018-10-19T11:53:11Z

spec/rspec/core/formatters/exception_presenter_spec.rb

@@ -94,6 +94,21 @@ module RSpec::Core
        EOS
      end

+      it 'allows the caller to add encoded description' do
+        the_presenter = Formatters::ExceptionPresenter.new(exception, example,
+                                                           :description => "ジ".encode("CP932"))


This spec isn't going to work on 1.8.7, check what we do for encoded string in rspec-support and mimic how thats tested? (this is just a suggestion of where to look!)

Good catch!

I found this if String.method_defined?(:encoding) https://github.com/rspec/rspec-support/blob/master/spec/rspec/support/differ_spec.rb#L81

For the moment I'm not able to install ruby-1.8.7-p374 on my machine. It fails with rvm so I will use the CI. Sorry for the noise.

benoittgt · 2018-10-22T07:49:32Z

I don't understand why it breaks that much on AppVeyor. I squashed the commit.

JonRowe · 2018-10-22T14:23:11Z

lib/rspec/core/formatters/exception_presenter.rb

+        def encoded_description(description)
+          return if description.nil?
+
+          if String.method_defined?(:encoding)


You can if case this entire method rather than performing it at runtime.

But it's needed in lib/rspec/core/formatters/exception_presenter.rb:84:

def fully_formatted_lines(failure_number, colorizer) lines = [ encoded_description(description),

No?

Yes, what I mean is this:

if String.method_defined?(:encoding) def encoded_description(description) return if description.nil? encoded_string(description) end else def encoded_description(description) description end end

Theres no need to attempt to encode the string if we can't

JonRowe · 2018-10-22T14:23:35Z

spec/rspec/core/formatters/exception_presenter_spec.rb

+
+          expect(the_presenter.fully_formatted(1)).to eq(<<-EOS.gsub(/^ +\|/, ''))
+          |
+          |  1) ジ


It's not this character on windows, which is why appveyor is failing, it comes back as ? instead

@hitanaka156 are you on windows? It'd be good to know what this is supposed to do, I'd be half happy pending this test on windows with a note explaining. (Correct behaviour on posix only is better than no correct behaviour).

I'd be half happy pending this test on windows with a note explaining

I'm ok with this. Thanks for the explanation

Error is

1) RSpec::Core::Formatters::ExceptionPresenter#fully_formatted allows the caller to add encoded description Failure/Error: expect(the_presenter.fully_formatted(1)).to eq(<<-EOS.gsub(/^ +\|/, '')) | | 1) πé╕ | Failure/Error: # The failure happened here!#{ encoding_check } | | Boom | Bam | # ./spec/rspec/core/formatters/exception_presenter_spec.rb:#{line_num} EOS expected: "\n 1) \u30B8\n Failure/Error: # The failure happened here!\n\n Boom\n Bam\n # ./spec/rspec/core/formatters/exception_presenter_spec.rb:21\n" got: "\n 1) ?\n Failure/Error: # The failure happened here!\n\n Boom\n Bam\n # ./spec/rspec/core/formatters/exception_presenter_spec.rb:21\n" (compared using ==) Diff: @@ -1,5 +1,5 @@ - 1) ? + 1) ? Failure/Error: # The failure happened here!

ジ turns to πé╕

I'm just surprised its the replacement character on windows

When the user create a test with CP932 encoding and japanese chars RSpec fail to properly display characters or crash with the error: > Encoding::CompatibilityError: incompatible character encodings: Windows-31J and UTF-8 Fix: - rspec#2570 - rspec#2543 For Ruby 1.8.7 We are following the same behavior as for rspec-support https://github.com/rspec/rspec-support/blob/9940a8656071655b807f772f36101b4d27f1b67d/lib/rspec/support/spec/string_matcher.rb#L8

benoittgt · 2018-11-16T20:35:28Z

First I rebase my branch with rspec/rspec-core master.

I started a VM on my machine with Windows 10, and run rspec ./spec/rspec/core/formatters/exception_presenter_spec.rb I had no errors.

On the appveyor the error is still present (and we have other errors related to symlink also now). Maybe this is related to locale?

On the VM

Maybe related: https://help.appveyor.com/discussions/problems/6245-unicode-characters-show-as-in-build-output

I continue to look at it

benoittgt · 2018-11-16T20:58:00Z

Ok, it looks good with chcp 65001 in appveyor config file. Same behavior as local VM.

Except for this point #2575 (comment) that I am not sure I have properly handled, the PR looks good to me. Waiting for the final review @JonRowe. 🙏🏼

JonRowe · 2018-11-16T21:21:40Z

appveyor.yml

@@ -29,7 +29,7 @@ before_test:
  - bundle --version

 test_script:
-  - bundle exec rspec --backtrace
+  - chcp 65001 && bundle exec rspec --backtrace


What does this do? Forgive my ignorance..

I learned this yesterday. I changed the commit message to add more info.

chp is for "change code page" (https://ss64.com/nt/chcp.html) and 65001 is the code for UTF-8. By doing this command you enforce UTF-8 locale for the next command.

Do we want to do that? Seems that we want to test we work in non UTF-8 environments...

But for me, if you don't set the correct local it will fail for every representation of strings with Japanese characters. I will run the command with Japanese locale https://docs.microsoft.com/en-us/windows/desktop/intl/code-page-identifiers to validate the correct behavior and also try to see what is the default local for appveyor.

My concern here is that we want to work with the console mode being japanese, not force it to utf-8

Sorry for the delay @JonRowe. I am thinking every day about this open PR and other things I have to do on RSpec repos. 😥

I ran a build with chcp to display the default encoding of app veyor machine.

chcp Active code page: 437

437 is for "United States". - source

So I think by doing this we are going in the right direction. 437 encoding is probably not meant to display Unicode char, we have to change it.
Related StackOverFlow answer https://stackoverflow.com/a/3781095/2747638
or https://superuser.com/questions/625866/cmd-how-to-use-non-english-characters

I can drop the last commit when you are ok with the existing work.

Alright, I honestly don't know any better so lets drop that commit and merge this, get a release out and see what affect this makes for our friends with problems

JonRowe · 2018-11-16T21:23:02Z

lib/rspec/core/formatters/exception_presenter.rb

+        def encoded_description(description)
+          return if description.nil?
+
+          if String.method_defined?(:encoding)


Yes, what I mean is this:

if String.method_defined?(:encoding) def encoded_description(description) return if description.nil? encoded_string(description) end else def encoded_description(description) description end end

Theres no need to attempt to encode the string if we can't

chcp 65001 mean Code Page Number and 65001 is UTF-8. So by doing this change we modify the code page to UTF-8 for the commande that follows. See documentation: https://ss64.com/nt/chcp.html See related issue: appveyor/ci#2107

Instead of runtime see https://github.com/rspec/rspec-core/pull/2575/files#r227002134

JonRowe · 2018-11-26T19:35:25Z

appveyor.yml

@@ -29,7 +29,7 @@ before_test:
  - bundle --version

 test_script:
-  - bundle exec rspec --backtrace
+  - chcp 65001 && bundle exec rspec --backtrace


Alright, I honestly don't know any better so lets drop that commit and merge this, get a release out and see what affect this makes for our friends with problems

Handle RSpec description with japanese char in CP932 encoded files

This commit was imported from rspec/rspec-core@4987e46.

…ith_encoding_error Handle RSpec description with japanese char in CP932 encoded files --- This commit was imported from rspec/rspec-core@6c5628f.

This commit was imported from rspec/rspec-core@f506d69.

JonRowe reviewed Oct 19, 2018

View reviewed changes

benoittgt force-pushed the deal_with_encoding_error branch 2 times, most recently from c7e2625 to d0b6ee4 Compare October 22, 2018 07:49

JonRowe requested changes Oct 22, 2018

View reviewed changes

benoittgt force-pushed the deal_with_encoding_error branch from d0b6ee4 to 05611c5 Compare November 16, 2018 20:21

JonRowe requested changes Nov 16, 2018

View reviewed changes

benoittgt added 2 commits November 17, 2018 09:16

Enforce UTF-8 locale when running tests in appveyor

0c9f05f

chcp 65001 mean Code Page Number and 65001 is UTF-8. So by doing this change we modify the code page to UTF-8 for the commande that follows. See documentation: https://ss64.com/nt/chcp.html See related issue: appveyor/ci#2107

choose encoded_description method content code earlier

8372e5c

Instead of runtime see https://github.com/rspec/rspec-core/pull/2575/files#r227002134

benoittgt force-pushed the deal_with_encoding_error branch from 076d4aa to 8372e5c Compare November 17, 2018 08:25

benoittgt force-pushed the deal_with_encoding_error branch from c4f0966 to df99dcd Compare November 26, 2018 05:56

JonRowe approved these changes Nov 26, 2018

View reviewed changes

benoittgt force-pushed the deal_with_encoding_error branch from df99dcd to 8372e5c Compare November 27, 2018 12:05

benoittgt merged commit 6c5628f into rspec:master Nov 27, 2018

benoittgt deleted the deal_with_encoding_error branch November 27, 2018 13:12

benoittgt added a commit that referenced this pull request Nov 27, 2018

Change log for #2575

f506d69

This was referenced Nov 27, 2018

Japanese letters get garbled in error messages. #2570

Closed

If the test code is CP 932 and Japanese is included in failure / error, Encoding::CompatibilityError #2543

Open

JonRowe pushed a commit that referenced this pull request Mar 4, 2019

Change log for #2575

4987e46

MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020

Merge pull request rspec#2575 from benoittgt/deal_with_encoding_error

7c3558f

Handle RSpec description with japanese char in CP932 encoded files

MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020

Change log for rspec#2575

85d60b9

yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 6, 2021

[core] Change log for rspec/rspec-core#2575

31cd62b

This commit was imported from rspec/rspec-core@4987e46.

yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 6, 2021

[core] Change log for rspec/rspec-core#2575

176d1d4

This commit was imported from rspec/rspec-core@f506d69.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle RSpec description with japanese char in CP932 encoded files #2575

Handle RSpec description with japanese char in CP932 encoded files #2575

benoittgt commented Oct 19, 2018

JonRowe Oct 19, 2018

benoittgt Oct 19, 2018

JonRowe Oct 19, 2018

benoittgt Oct 19, 2018

benoittgt commented Oct 22, 2018

JonRowe Oct 22, 2018

benoittgt Oct 22, 2018

JonRowe Nov 16, 2018

benoittgt Nov 17, 2018

JonRowe Oct 22, 2018

JonRowe Oct 22, 2018

benoittgt Oct 22, 2018 •

edited

benoittgt Oct 22, 2018

JonRowe Oct 23, 2018

benoittgt commented Nov 16, 2018 •

edited

benoittgt commented Nov 16, 2018

JonRowe Nov 16, 2018

benoittgt Nov 17, 2018 •

edited

JonRowe Nov 17, 2018

benoittgt Nov 17, 2018 •

edited

JonRowe Nov 20, 2018

benoittgt Nov 26, 2018 •

edited

benoittgt Nov 26, 2018

JonRowe Nov 26, 2018

JonRowe Nov 16, 2018

JonRowe Nov 26, 2018

Handle RSpec description with japanese char in CP932 encoded files #2575

Handle RSpec description with japanese char in CP932 encoded files #2575

Conversation

benoittgt commented Oct 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoittgt commented Oct 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoittgt Oct 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoittgt commented Nov 16, 2018 • edited

benoittgt commented Nov 16, 2018

Choose a reason for hiding this comment

benoittgt Nov 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoittgt Nov 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoittgt Nov 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoittgt Oct 22, 2018 •

edited

benoittgt commented Nov 16, 2018 •

edited

benoittgt Nov 17, 2018 •

edited

benoittgt Nov 17, 2018 •

edited

benoittgt Nov 26, 2018 •

edited