rspec / rspec-expectations

Provides a readable API to express expected outcomes of a code example
https://rspec.info
MIT License
1.26k stars 395 forks source link

raise_error with ArgumentError fails with incorrect message #1388

Closed denisdefreyne closed 1 year ago

denisdefreyne commented 1 year ago

Subject of the issue

A raise_error expectation with ArgumentError will not match the correct message.

Your environment

Steps to reproduce

Write the following to spec/foo_spec.rb:

# frozen_string_literal: true

describe 'Foo' do
  example do
    expect { raise ArgumentError, 'foo' }
      .to raise_error(ArgumentError, 'foo')
  end
end

Run bundle exec rspec spec/foo_spec.rb.

Expected behavior

Spec passes.

Actual behavior

  1) Foo is expected to raise ArgumentError with "foo"
     Failure/Error:
       expect { raise ArgumentError, 'foo' }
         .to raise_error(ArgumentError, 'foo')

       expected ArgumentError with "foo", got #<ArgumentError: foo

           expect { raise ArgumentError, 'foo' }
                          ^^^^^^^^^^^^^^^^^^^^> with backtrace:
         # ./spec/foo_spec.rb:5:in `block (3 levels) in <top (required)>'
         # /Users/denis/.rubies/ruby-3.1.2/lib/ruby/gems/3.1.0/gems/rspec-expectations-3.12.0/lib/rspec/matchers/built_in/raise_error.rb:59:in `matches?'

The message of the raised ArgumentError is a String with the following contents:

foo

    expect { raise ArgumentError, 'foo' }
                   ^^^^^^^^^^^^^^^^^^^^

More details

This only appears to be an issue for ArgumentError.

This started failing in the week or so. I’ve not been able to narrow down the cause just yet.

This fails with stock Ruby, no gems loaded.

To reproduce without RSpec:

begin
  raise ArgumentError, 'zing'
rescue => e
  p e.message
end

This prints "zing\n\n raise ArgumentError, 'zing'\n ^^^^^^^^^^^^^^^^^^^^^".

It should (I think) print "zing" instead.

denisdefreyne commented 1 year ago

I’m 99% sure this is caused by error_highlight.

I’m not sure whether this really is a RSpec issue, but… if it is not, would it be good for rspec-expectation to handle it anyway?

denisdefreyne commented 1 year ago

This is likely a duplicate of #1362.

denisdefreyne commented 1 year ago

It looks like error_highlight version 0.5 (released on November 08, 2022) made this break. Most likely candidate for the cause is this commit: https://github.com/ruby/error_highlight/commit/defcaf1beb95ea9d233cced21863e88ccbe64203

denisdefreyne commented 1 year ago

Relevant issue in error_highlight repo: https://github.com/ruby/error_highlight/issues/28

pirj commented 1 year ago

Thanks for reporting. I'm a bit confused by examples in the description of a relevant error_highlight issue:

works without failures with error_highlight 0.4.0 gem install error_highlight -v 0.5.0

Is this right? Do I understand it correctly that there's a regression in error_highlight 0.5.0?

Would version pinning in Rails

gem "error_highlight", ">= 0.4.0", "< 0.5.0"

fix the issue? Can we just recommend using the next Rails version to fix this for our users?

I would like to point out that rspec-expectations do not depend on error_highlight, and pinning it here would make no sense.

If you run your example on a blank slate project without error_highlight, it would pass.

A related fix mentioning error_highlight.

Do you think there is anything we can fix by making changes in rspec-expectations, @denisdefreyne ?

denisdefreyne commented 1 year ago

That is correct:

Do I understand it correctly that there's a regression in error_highlight 0.5.0?

Yes

Would version pinning in Rails […] fix the issue?

I can definitely pin the error_highlight gem to 0.4.x in my own (non-Rails) projects, though that would only solve the problem for these personal projects.

If you run your example on a blank slate project without error_highlight, it would pass.

Yes and no -- error_highlight is part of Ruby (at least version 0.3) and required by default. Luckily, versions 0.3 and 0.4 do not exhibit this problem, but if you would have error_highlight 0.5 installed, the example would fail even without invoking any requires.

Do you think there is anything we can fix by making changes in rspec-expectations

It’s a good question! It’s also a question that might not need to be answered right now, depending on how/if https://github.com/ruby/error_highlight/issues/28 gets resolved.

As a hack, I imagine rspec-expectations could strip off a trailing \n\n.*\n\s*\^+ regex -- but oh, I do not like that hack. 😅

denisdefreyne commented 1 year ago

I think the better way forward might be to encourage people to not write

.to raise_error(ArgumentError, 'foo')

but rather use an anchored regex:

.to raise_error(ArgumentError, /\Afoo$/)
JonRowe commented 1 year ago

Is it just new line characters added?

yahonda commented 1 year ago

I believe error_highlight 0.5.1 including https://github.com/ruby/error_highlight/pull/29 address this issue.

pirj commented 1 year ago

Thank you, @yahonda . I also see a Rails PR. So what most users would have to do is to:

bundle update --conservative error_highlight

Is this correct?

Is there anything left to be done in rspec-expectations?

yahonda commented 1 year ago

Just updating error_highlight version to 0.5.1 is fine.

--conservative option is not mandatory. I added it because I just want to update error_highlight gem. Refer to https://bundler.io/v1.14/whats_new.html#conservative-updates

pirj commented 1 year ago

@denisdefreyne @JonRowe Any objections on closing this issue?

JonRowe commented 1 year ago

I think as error_highlight has released a change fixing this (unintentional breaking of #message) this is closable by us.

denisdefreyne commented 1 year ago

Perfect, thanks :)