Skip to content
This repository was archived by the owner on Nov 30, 2024. It is now read-only.

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

Merged
merged 3 commits into from
Nov 27, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
05611c5
Handle RSpec description with japanese char in CP932 encoded files
benoittgt Oct 19, 2018
0c9f05f
Enforce UTF-8 locale when running tests in appveyor
benoittgt Nov 16, 2018
8372e5c
choose encoded_description method content code earlier
benoittgt Nov 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion appveyor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ before_test:
- bundle --version

test_script:
- bundle exec rspec --backtrace
- chcp 65001 && bundle exec rspec --backtrace
Copy link
Member

Choose a reason for hiding this comment

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

What does this do? Forgive my ignorance..

Copy link
Member Author

@benoittgt benoittgt Nov 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@benoittgt benoittgt Nov 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@benoittgt benoittgt Nov 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


environment:
matrix:
Expand Down
13 changes: 12 additions & 1 deletion lib/rspec/core/formatters/exception_presenter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ def fully_formatted(failure_number, colorizer=::RSpec::Core::Formatters::Console

def fully_formatted_lines(failure_number, colorizer)
lines = [
description,
encoded_description(description),
detail_formatter.call(example, colorizer),
formatted_message_and_backtrace(colorizer),
extra_detail_formatter.call(failure_number, colorizer),
Expand Down Expand Up @@ -244,6 +244,17 @@ def formatted_message_and_backtrace(colorizer)
end
end

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

def exception_backtrace
exception.backtrace || []
end
Expand Down
17 changes: 17 additions & 0 deletions spec/rspec/core/formatters/exception_presenter_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,23 @@ module RSpec::Core
EOS
end

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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).

Copy link
Member Author

@benoittgt benoittgt Oct 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

I'm ok with this. Thanks for the explanation

Copy link
Member Author

Choose a reason for hiding this comment

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

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 πé╕

Copy link
Member

Choose a reason for hiding this comment

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

I'm just surprised its the replacement character on windows

| Failure/Error: # The failure happened here!#{ encoding_check }
|
| Boom
| Bam
| # ./spec/rspec/core/formatters/exception_presenter_spec.rb:#{line_num}
EOS
end
end

it 'allows the caller to omit the description' do
the_presenter = Formatters::ExceptionPresenter.new(exception, example,
:detail_formatter => Proc.new { "Detail!" },
Expand Down