rubocop-rspec: `RSpec/NoExpectationExample` false positives

I have reviewed a codebase with the cop applied, and now I think that we may add an AllowedPatterns configuration, so people can just whitelist expect_ and assert_ methods without needing to add them to Language. Though having not them in language also forces people to think how to better express them, e.g. creating custom matchers, instead of merely methods that run expectations

@annaswims you can add assert_text (and other Capybara assertions) in the language config, so it counts it as an expectation

Personally, I follow the recommendation to use helper methods that is supported by the Effective Testing with RSpec book.

However, since for static checking it creates complications, do you think we can all agree that if something is an expectation, we can name it as such?

def expect_low_value
  expect(product.value).to be_between(0, 50)
end

context 'during summer' do
  it 'has low value' do
    expect_low_value
  end
end

In such a way, we can introduce an option with a default to the cop, say, ConsiderHelperMethodsPrefixes: [expect] that would consider such helper methods as expectations.

Eventually, this can become part of the community RSpec style guide if it gets some traction.

How does that sound?

PS My apologies for an attempt to steal the idea, this is what @Darhazer suggests above.

A lot of edge cases for single cop, maybe it should be disabled by default

It also does not accept Capybara::Node::Matchers#assert_text as an assertion in an example like:

    it "is not authorized" do
      assert_text("You are not authorized to perform this action.")
    end

If no one plans to send a PR, I will send. How do you like it?

As I can see from the source of e.g. has_link?,

      def has_link?(locator = nil, **options, &optional_filter_block)
        has_selector?(:link, locator, **options, &optional_filter_block)
      end

      def has_selector?(*args, **options, &optional_filter_block)
        make_predicate(options) { assert_selector(*args, options, &optional_filter_block) }
      end

      def make_predicate(options)
        options[:wait] = 0 unless options.key?(:wait) || session_options.predicates_wait
        yield
      rescue Capybara::ExpectationNotMet
        false
      end

just returns false that is not sufficient for RSpec to consider the example failed.

Won’t

        it 'has a `New Feed` link' do
          within('body') do
            has_link?('New Feed', exact: true)
          end
        end

be equivalent to:

        it 'has a `New Feed` link' do
          within('body') do
            false
          end
        end

?

As I recall how RSpec predicate matchers work, expect(page).to have_link(...) is a just another way of writing expect(page.has_link?(...)).to be(true). But omitting expect(page) effectively means no expectation is made.

Similar problem then expect in after block

Something like this:

RSpec.describe do
  after do
    expect('File exists').not_to be_nil
  end

  it 'foo' do
    p 'crete_file'
  end

  it 'bar' do
    p 'crete_file another way'
  end
end

There’s no better candidate than you, @ydah with so many recent contributions, please go for it.

Does anyone want to send a PR to introduce AllowedPatterns configuration option, so with default expect_ and assert_ as @Darhazer suggests above?

expect in after block

RSpec/ExpectInHook disapproves such style, @ShockwaveNN

As I can see from the source of e.g. has_link?,

      def has_link?(locator = nil, **options, &optional_filter_block)
        has_selector?(:link, locator, **options, &optional_filter_block)
      end

      def has_selector?(*args, **options, &optional_filter_block)
        make_predicate(options) { assert_selector(*args, options, &optional_filter_block) }
      end

      def make_predicate(options)
        options[:wait] = 0 unless options.key?(:wait) || session_options.predicates_wait
        yield
      rescue Capybara::ExpectationNotMet
        false
      end

just returns false that is not sufficient for RSpec to consider the example failed.

Won’t

        it 'has a `New Feed` link' do
          within('body') do
            has_link?('New Feed', exact: true)
          end
        end

be equivalent to:

        it 'has a `New Feed` link' do
          within('body') do
            false
          end
        end

?

As I recall how RSpec predicate matchers work, expect(page).to have_link(...) is a just another way of writing expect(page.has_link?(...)).to be(true). But omitting expect(page) effectively means no expectation is made.

Yep, @pirj ,you’re right - I don’t know why I was thinking these matchers are working also like expectations 😦

I think we have to account for the expectations in hooks that are in scope. We have the ExpectInHook that would disallow such usage, but it would be frustrating for users to disable that cop just to have NoExpectationExample complain again.

Maybe we also should add the capybara expectations by default to the language (perhaps behind a config option). I’m on a fense for this one, as it would affect things like MultipleExpectations - perhaps the style for capybara scenarios is a bit different.

@rubocop/rubocop-rspec WDYT?