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

NameError on Ruby 3.1 breaks error matching #1338

Closed petergoldstein closed 2 years ago

petergoldstein commented 2 years ago

Subject of the issue

The introduction of error highlight behavior in Ruby 3.1 for NameError causes the raise_error(message)and raise_error.with_message(message) matchers to fail on NameErrors.

This has a wide impact on the ecosystem, causing CI to fail for a number of repos when using Ruby 3.1.0. That includes rspec-expectations own CI run. Getting some sort of solution out for this quickly will make it much easier to add Ruby 3.1 as an option in the ecosystem.

It should be noted that the original message is preserved by syntax highlighting and can be accessed via the original_message method. A somewhat hacky solution may be to introduce a method like

def actual_error_message
  return nil unless @actual_error

  @actual_error.respond_to?(:original_message) ? @actual_error.original_message : @actual_error.message
end

and replace the use of @actual_error.message in RaiseError with this method call.

Your environment

Steps to reproduce

Failing cases can be found in the rspec-expectations test suite. The simplest to reproduce is:

  1. Create a single spec file with the following content:
RSpec.describe "expect { ... }.to raise_error(message)" do
  it "passes if a NameError is raised with the expected message" do
    expect { raise NameError.new('blah') }.to raise_error('blah')
  end
end
  1. Run this spec file under rspec with Ruby 3.0. It passes.
  2. Switch to Ruby 3.1.0
  3. Run the spec again, and it fails

Expected behavior

NameErrors initialized with the expected message should pass the matcher (raise_error(message), raise_error.with_message(message)).

Actual behavior

NameErrors initialized with the expected message fail the matcher (raise_error(message), raise_error.with_message(message)).

pirj commented 2 years ago

That sounds like a major compatibility issue. I regret we didn't build against ruby-head before.

Ruby 3.1 CI to fail ... rspec-expectations own CI run

But how, we don't have 3.1 in our build matrix, do we?

Is this the raise_error to blame? Can you please send a PR with a failing example? If it's failing our build, a PR to add Ruby 3.1 to the build matrix is sufficient.

We'll surely do what we can to make RSpec compatible with Ruby 3.1. However, please consider that we are volunteers, and presently we have quite a lot on our plate. If you could invest time into the fix, this is very much appreciated.

petergoldstein commented 2 years ago

@pirj Of course. Your efforts are appreciated. And I'm happy to help.

I've got a failing build against my fork of the repo and am working on a fix. I'm running into a few other build failures, mostly against older Rubies (e.g. 2.1) running on Windows because of a failure to load something called ansicon.

I should have a PR up today, although it may not be green against these older Rubies.

(And the referenced failure occurs when I add Ruby 3.1 to the rspec-expectations CI matrix)

pirj commented 2 years ago

First of all, my bad. We do, indeed, test against ruby-head. It's just that we allow failures of this build job. We should have made the build mandatory and prepared for Ruby 3.1 release in advance. This is our miss.

Please disregard CI failures on older builds, it's an evergoing struggle - things break here and there without any changes on our side. Do not bother fixing anything that you have not broken, or even running older versions locally - it's a pain.

What I suggest doing:

We'll provide guidance and feedback as needed. And thanks for volunteering to fixing this issue. I hope it's the only one on the road to compatibility with Ruby 3.1.

petergoldstein commented 2 years ago

So this turned out to be more complicated that I thought, so I'm going to break it into two separate issues:

  1. NameError on Ruby 3.1 breaks error matching.
  2. Rails 3.1 in CI

This issue will track #1. I'll open #2 in a separate issue.

I got confused, and while I was seeing the NameError issue locally I DID NOT see it in rspec-expectations CI. I'm virtually certain I saw it in a CI run for at least one other repo, but I need to track that down.

I can reliably reproduce the issue locally in this environment - MacBook Pro w/Monterey (12.1) running Rubies installed with rbenv. To reproduce:

  1. Check out the rspec-expectations repo
  2. Install Ruby 3.1.0 using rbenv and select it as the active Ruby
  3. Run bundle
  4. Run bundle exec rspec spec/rspec/matchers/built_in/raise_error_spec.rb
  5. Specs fail because of the additional text in the error message

By setting a RUBYOPT value I can make the specs green

  1. Now run RUBYOPT="--disable-error_highlight" bundle exec rspec spec/rspec/matchers/built_in/raise_error_spec.rb
  2. Specs will pass

I'm not sure why error highlighting isn't being enabled in the CI environment for rspec-expectations, but this is definitely an observable failure locally.

I have a fix for it that doesn't require the user to set the RUBYOPT value. That PR is in draft and I'm doing some rebasing and tweaking. Will link here when it's ready.

petergoldstein commented 2 years ago

Here's the PR with the changes that fix the raise_error_spec.rb run referenced in my previous comment - https://github.com/rspec/rspec-expectations/pull/1339

petergoldstein commented 2 years ago

As a note, I just ran into this when I tried to add Ruby 3.1 to the factory_bot CI matrix. In CI we're seeing a failure that's due to this issue. Adding the RUBYOPT value allowed me to suppress the behavior - https://github.com/thoughtbot/factory_bot/pull/1526

petergoldstein commented 2 years ago

Ran into this when adding Ruby 3.1 to the hashie CI matrix - https://github.com/hashie/hashie/pull/558

JonRowe commented 2 years ago

Closed in #1339