sj26 / rspec_junit_formatter

RSpec results that your CI can read
http://rubygems.org/gems/rspec_junit_formatter
MIT License
302 stars 122 forks source link

Force convert all the diff content to UTF-8 #78

Closed agate closed 4 years ago

agate commented 4 years ago

Force convert all the diff content to UTF-8

What

We are facing exception:

/bundle/ruby/2.6.0/bundler/gems/rspec_junit_formatter-1129bf0d5fb4/lib/rspec_junit_formatter.rb:186:in `sub': invalid byte sequence in UTF-8 (ArgumentError)

Why

One of the diff string contains invalid UTF-8 characters. E.g. string = "784: unexpected token at '\u001F\x8B\b'" Then if you run string.sub('xxx', 'xxx') you will see the same exception be raised.

Solution

Force encode string to UTF-8 encoding at the beginning.

agate commented 4 years ago

Hi @sj26 Could you take a look this patch and let me know if it looks good to you. If not, please let me know what should I do to make it able to be merged. Thanks!

sj26 commented 4 years ago

Hi @agate, sorry for the delay. This should already be handled by escape(...). Can you provide a failing example?

agate commented 4 years ago

Sorry that I didn't scan all the methods in RSpecJUnitFormatter. Seems escape will also have this problem. Let me show you some examples.

> RSpecJUnitFormatter.new("").send(:escape, "test")
 => "test"
> RSpecJUnitFormatter.new("").send(:escape, "\u001F\x8B\b")
Traceback (most recent call last):
        7: from /Users/foobar/.rvm/rubies/ruby-2.6.3/bin/irb:23:in `<main>'
        6: from /Users/foobar/.rvm/rubies/ruby-2.6.3/bin/irb:23:in `load'
        5: from /Users/foobar/.rvm/rubies/ruby-2.6.3/lib/ruby/gems/2.6.0/gems/irb-1.0.0/exe/irb:11:in `<top (required)>'
        4: from (irb):16
        3: from (irb):16:in `rescue in irb_binding'
        2: from /Users/foobar/.rvm/gems/ruby-2.6.3/gems/rspec_junit_formatter-0.4.1/lib/rspec_junit_formatter.rb:165:in `escape'
        1: from /Users/foobar/.rvm/gems/ruby-2.6.3/gems/rspec_junit_formatter-0.4.1/lib/rspec_junit_formatter.rb:165:in `gsub'

Same idea to strip_diff_colors:

> RSpecJUnitFormatter.new("").send(:strip_diff_colors, "test")
 => "test"
> RSpecJUnitFormatter.new("").send(:strip_diff_colors, "\u001F\x8B\b")
Traceback (most recent call last):
        6: from /Users/foobar/.rvm/rubies/ruby-2.6.3/bin/irb:23:in `<main>'
        5: from /Users/foobar/.rvm/rubies/ruby-2.6.3/bin/irb:23:in `load'
        4: from /Users/foobar/.rvm/rubies/ruby-2.6.3/lib/ruby/gems/2.6.0/gems/irb-1.0.0/exe/irb:11:in `<top (required)>'
        3: from (irb):19
        2: from /Users/foobar/.rvm/gems/ruby-2.6.3/gems/rspec_junit_formatter-0.4.1/lib/rspec_junit_formatter.rb:181:in `strip_diff_colors'
        1: from /Users/foobar/.rvm/gems/ruby-2.6.3/gems/rspec_junit_formatter-0.4.1/lib/rspec_junit_formatter.rb:181:in `sub'
ArgumentError (invalid byte sequence in UTF-8)

So for sure I need to update this PR. But I noticed that strip_diff_colors didn't call escape method. So even if I fix this problem in escape the strip_diff_colors method will still raise exception. Then the question will be do you want me to add encode('UTF-8', 'UTF-8', invalid: :replace) inside of escape method and then call escape inside of strip_diff_colors? Or do the encoding for both methods?

sj26 commented 4 years ago

strip_diff_colours has been calibrated to strip expected rspec escape codes in an expected place, and where there is no other way to remove the colours. It's output is consumed by a thing which should then call escape on the output. The idea is that we don't want to strip legit codes from example output, like for example if you were working on a library which ansi colourises things.

I can fix it if you can tell me the version of rspec, rspec_junit_formatter you're using, and provide a spec file which causes bad output?

sj26 commented 4 years ago

@agate are you able to supply a failing case, here?

agate commented 4 years ago

Sorry. I wasn't able to find out the output that we use to see. I was hoping to reproduce this in our CI. But after weeks wait. We still not able to reproduce the problem. I suggest we can just close this PR. And when I see the error shows up again I will reopen one.

BTW, we are using: rspec_junit_formatter: 0.4.1 rspec: 3.8.0

sj26 commented 4 years ago

Sorry @agate, thanks for understanding — please keep an eye out and if you find a reproducible case we can fix it. 🙌