RSpec/UnspecifiedException appears to trigger if the subject function is named `raise_exception`

[aarestad]

It seems like the RSpec/UnspecifiedException cop should not be triggered on the following code, but it is:

require "spec_helper"

class Foo

  def raise_exception
    raise StandardError, "huh"
  end

end

describe Foo do
  let(:subject) { described_class.new }

  describe "#raise_exception" do
    it "throws a StandardError" do
      expect { subject.raise_exception }.to raise_error(StandardError)
    end
  end
end

In particular, the cop highlights subject.raise_exception as violating the cop, which doesn't make sense - the cop should be looking it how we handle the exception, yeah?

[pirj]

raise_exception sounds like it’s an internal class api, a private method. Is it?

To fix the cop, I see two options:

1. Break on non-send nodes here.
2. Add a check that the receiver is implicit (nil) here

Would you like to submit a pr?

[aarestad]

In our code, it is a public method (obviously not public public, but y'know). I will take a look at this tomorrow (US Central) and see if I can submit a PR myself. :)

[aarestad]

I am having trouble understanding how to implement this fix, sorry. I don't fully understand the parsing abstractions being done, and I have no experience here. :( I will try to stare at it some more, but I feel like I am out of my depth here.

[pirj]

This is really easy, as in either case you won’t have a o dig i to the abstract syntax tree or node patterns.
Just write a spec for two cases, and set breakpoints to those places i’ve referenced above.

1. This iterates from the send node (the statement that is a method call) up through all the method calls. Since RSpec is all DSL, a lot will be “send” nodes. But some will be send nodes wrapped in block nodes, which is eg expect { raise_exception }. As soon as we encounter a “block” node, we need to break from the cycle, as this is not the raise_exception we’re after, since it’s not called on the expect { … }.to. Basically, remove the :send argument from each_ancestor to avoid filtering only on those “send” nodes, and break as soon as you encounter a “block” node. You can check it with ancestor.block_type? and ancestor.send_type?.
2. Check if the node.receiver.nil? and return otherwise (the case you’ve described when there’s an explicit receiver)

You can implement either fix or both. Please let me know if you need more details, and please don’t hesitate to ask if you get stuck.

[aarestad]

Thank you for this explanation! I apologize for not responding sooner - I have had a lot going on. I would be happy to take a look at doing a quick PR. I did get as far as creating a failing test case, at least. :)

[aarestad]

Hey! I tried implementing both of these like this in Rspec::UnspecifiedException:

        def find_expect_to(node)
          return if node.receiver.nil?

          node.each_ancestor.find do |ancestor|
            break if ancestor.block_type?

            next unless ancestor.send_type?

            expect_to?(ancestor)
          end
        end

and added the following tests to the spec:

    it 'allows a subject function to be named raise_exception' do
      expect_no_offenses(<<~RUBY)
        def raise_exception
          raise StandardError
        end

        expect {
          raise_exception
        }.to raise_error(StandardError)
      RUBY
    end

    it 'allows a subject function to be named raise_error' do
      expect_no_offenses(<<~RUBY)
        def raise_error
          raise StandardError
        end

        expect {
          raise_error
        }.to raise_exception(StandardError)
      RUBY
    end

This caused my new test cases to pass, but it caused several regressions in the UnspecifiedException suite. Among others, this test failed:

    it 'detects the `unspecified_exception` offense' do
      expect_offense(<<~RUBY)
        expect {
          raise StandardError
        }.to raise_error
             ^^^^^^^^^^^ Specify the exception being captured
      RUBY
    end

with this message:

0) RuboCop::Cop::RSpec::UnspecifiedException with raise_error matcher detects the `unspecified_exception` offense
     Failure/Error: super

       Diff:
       @@ -1,5 +1,4 @@
        expect {
          raise StandardError
        }.to raise_error
       -     ^^^^^^^^^^^ Specify the exception being captured
       
     # ./spec/support/expect_offense.rb:17:in `expect_offense'
     # ./spec/rubocop/cop/rspec/unspecified_exception_spec.rb:6:in `block (3 levels) in <top (required)>'

Apologies for the dump - I am still fairly new with Ruby, and in particular, the internals of RuboCop.

[G-Rath]

@aarestad I believe that @pirj was meaning you should implement one of those fixes, not both of them, and it seems that option 1 is the better way to go: if you remove the return if node.receiver.nil? (aka option 2) then for me locally all the tests pass, indicating you've fixed the issue 🎉

Also two tips I've learnt from my contributions:

1. there's a sort-of-bug in how the assertions work which is why you end up with Failure/Error: super - you can see the real error by commenting out expect_offense and expect_no_offenses in spec/support/expect_offence.rb
   • (I'm not a core maintainer of RuboCop nor have I extensively tested the whole plugin with this change, so there is probably a very legit reason for this code, and generally anyway most of the time the error is "offense was/wasn't raised when it shouldn't/should be")
2. you can inspect the AST using puts RuboCop::AST::ProcessedSource.new(File.read('script'), 2.7).ast.inspect (make sure to require 'rubocop/ast':

rubocop-rspec on  master [$!?] via 💎 v3.1.4
❯ cat script
def raise_exception
  raise StandardError
end

expect { raise_exception }.to raise_error(StandardError)
expect { raise StandardError }.to raise_exception

rubocop-rspec on  master [$!?] via 💎 v3.1.4
❯ ruby print-ast.rb
s(:begin,
  s(:def, :raise_exception,
    s(:args),
    s(:send, nil, :raise,
      s(:const, nil, :StandardError))),
  s(:send,
    s(:block,
      s(:send, nil, :expect),
      s(:args),
      s(:send, nil, :raise_exception)), :to,
    s(:send, nil, :raise_error,
      s(:const, nil, :StandardError))),
  s(:send,
    s(:block,
      s(:send, nil, :expect),
      s(:args),
      s(:send, nil, :raise,
        s(:const, nil, :StandardError))), :to,
    s(:send, nil, :raise_exception)))

You might have already figured these out but since you mentioned this was the first time you'd worked with the Rubocop code, I figure no harm in sharing as those two things were probably the most impactful to figure out when I was getting started 🙂

[G-Rath]

I've confirmed locally as well that this fixes #1954 - @aarestad feel free to include a third spec for that if you want, otherwise I'm happy to do a new PR after yours adding a spec to showcase its fixed and to prevent regression in the future

[aarestad]

Huh - I could have sworn I tried to do them each on their own. Looks ok - I am going to create a PR now. Thank you all for checking my work in advance :)