error_highlight fix #1339 does not correctly report the issue

[voxik]

Trying to execute Thor test suite with rspec-expectations 3.11.0 + Ruby 3.1, I observe the following error:

  1) Thor::Options#parse raises an error for unknown switches
     Failure/Error: expect { check_unknown! }.to raise_error(Thor::UnknownArgumentError, expected)

       expected Thor::UnknownArgumentError with "Unknown switches \"--baz\"\nDid you mean?  \"--bar\"", got #<Thor::UnknownArgumentError: Unknown switches "--baz"
       Did you mean?  "--bar"> with backtrace:
         # ./lib/thor/parser/options.rb:146:in `check_unknown!'
         # ./spec/parser/options_spec.rb:17:in `check_unknown!'
         # ./spec/parser/options_spec.rb:119:in `block (4 levels) in <top (required)>'
         # ./spec/parser/options_spec.rb:119:in `block (3 levels) in <top (required)>'
         # /usr/share/gems/gems/webmock-3.14.0/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'
     # ./spec/parser/options_spec.rb:119:in `block (3 levels) in <top (required)>'
     # /usr/share/gems/gems/webmock-3.14.0/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'

This brought me to the #1339, which is the reason for the error message, but I don't think the reported error is correct. It should actually be:

       expected Thor::UnknownArgumentError with "Unknown switches \"--baz\"\nDid you mean?  \"--bar\"", got #<Thor::UnknownArgumentError: Unknown switches "--baz"> with backtrace:

IOW it should not mention the Did you mean? "--bar", because the original_message was used for the comparison. I think that this line should be adjusted.

This is the test case in question:

https://github.com/rails/thor/blob/ab3b5be455791f4efb79f0efb4f88cc6b59c8ccf/spec/parser/options_spec.rb#L112-L120

[voxik]

This IMO falls into same bucket as https://github.com/rspec/rspec-mocks/issues/1460 and the error message should give even more details what is going on, because this is not transparent, especially at the time a lot of places are already adjusted to account for DidYouMean

[pirj]

It seems that expected is missing indentation:

# actual
Unknown switches "--baz"
       Did you mean?  "--bar"

vs

# expected
Unknown switches "--baz"
Did you mean?  "--bar"

[JonRowe]

I'm open to improving the error output of this matcher, I don't think its automatically diffed, so it could improve its output when the class matches but the message differs, even leveraging the differ for the error string. Want to have a go at this @voxik?

[pirj]

Interesting.

    151: def verify_message
 => 152:   require 'pry'; binding.pry
    153:   return true if @expected_message.nil?
    154:   values_match?(@expected_message, actual_error_message.to_s)
    155: end

[1] pry(#<RSpec::Matchers::BuiltIn::RaiseError>)> @expected_message
=> "Unknown switches \"--baz\"\nDid you mean?  \"--bar\""
[2] pry(#<RSpec::Matchers::BuiltIn::RaiseError>)> actual_error_message
=> "Unknown switches \"--baz\""
[3] pry(#<RSpec::Matchers::BuiltIn::RaiseError>)> $ actual_error_message

From: /Users/pirj/.rvm/gems/ruby-3.1.0@thor/gems/rspec-expectations-3.11.0/lib/rspec/matchers/built_in/raise_error.rb:119:
Owner: RSpec::Matchers::BuiltIn::RaiseError
Visibility: private
Signature: actual_error_message()
Number of lines: 5

def actual_error_message
  return nil unless @actual_error

  @actual_error.respond_to?(:original_message) ? @actual_error.original_message : @actual_error.message
end
[4] pry(#<RSpec::Matchers::BuiltIn::RaiseError>)> @actual_error
=> #<Thor::UnknownArgumentError: Unknown switches "--baz"
Did you mean?  "--bar">
[5] pry(#<RSpec::Matchers::BuiltIn::RaiseError>)> @actual_error.message
=> "Unknown switches \"--baz\"\nDid you mean?  \"--bar\""
[6] pry(#<RSpec::Matchers::BuiltIn::RaiseError>)> @actual_error.original_message
=> "Unknown switches \"--baz\""
[7] pry(#<RSpec::Matchers::BuiltIn::RaiseError>)> $ @actual_error.original_message

From: /Users/pirj/.rvm/rubies/ruby-3.1.0/lib/ruby/3.1.0/did_you_mean/core_ext/name_error.rb:6:
Owner: DidYouMean::Correctable
Visibility: public
Signature: original_message()
Number of lines: 7

def original_message
  meth = method(:to_s)
  while meth.owner.const_defined?(:SKIP_TO_S_FOR_SUPER_LOOKUP)
    meth = meth.super_method
  end
  meth.call
end
[8] pry(#<RSpec::Matchers::BuiltIn::RaiseError>)>

It indeed was introduced in #1339 .

WDYT of changing this to something like?

values_match?(@expected_message, actual_error_message.to_s) || values_match?(@expected_message, @actual_error.message)

[voxik]

WDYT of changing this to something like?

values_match?(@expected_message, actual_error_message.to_s) || values_match?(@expected_message, @actual_error.message)

I think that this would be good if you preferred backward compatibility and keep the test suite working.

However, as I said, this IMO belongs to the same bucket as https://github.com/rspec/rspec-mocks/issues/1460 (it covers new Ruby functionality, but it is not very clear what is going on) and therefore the functionality should stay as it is, the error message should provide more information and all the test suites will need to be adjusted.

On top of that RSpec could be extended to be able to test the original output (which is the current situation, but was not the case previously) as well as the output mangled by did_you_mean/error_highlight (if this is deemed interesting, testing the original is probably more valuable).

IOW I'd say that the proposed solution could be enough to fix the behavior but the "right breaking" behavior should be introduced at some right milestone.

[voxik]

IOW it is unfortunate situation that the #1339 have been triggered by error_highlight, where the same issues had already happened when did_you_mean was introduced.

With did_you_mean, upstreams were forced to update their test suites, while with error_highlight (where the issue with mangled output was probably more prominent), RSpec decided to check the original output, which in turn breaks the test suites which were already adjusted for did_you_mean.