`RSpec/NoExpectationExample` false positives

[boris-petrov]

scenario 'test' do
  method
end

private

def method
  expect(1).to eq(1)
end

This gives RSpec/NoExpectationExample but it shouldn't. :)

[ShockwaveNN]

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

[annaswims]

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

[ShockwaveNN]

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

[Darhazer]

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

[annaswims]

Thanks! I missed the docs

[Darhazer]

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?

[bquorning]

I think this would be much easier to check with dynamic analysis that with static analysis. @pirj Is this something that might be built into RSpec itself one day?

[pirj]

No, it's not possible (to do this reliably) in RSpec, too, since rspec-mocks/rspec-expectations are not the only way of making expectations, e.g. assert_nil being one I frequently see. There was a discussion about this somewhere in RSpec issue tracker, but I can't quickly find it.

[Darhazer]

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

[AlexWayfer]

My case with sequel-enum_values:

I've got RSpec/NoExpectationExample: No expectation found in this example for such code:

shared_examples(
	'for all enum predicate methods'
) do |predicate_method_names, *expectations|
	predicate_method_names.each do |predicate_method_name|
		describe predicate_method_name do
			subject { item_class.new.public_send(predicate_method_name) }

			## rubocop:disable RSpec/NoExpectationExample
			specify do
				Array(expectations).each do |expectation|
					instance_exec(&expectation)
				end
			end
			## rubocop:enable RSpec/NoExpectationExample
		end
	end
end

I understand, meta-programming is difficult for linters like RuboCop.

But I've also got:

Lint/RedundantCopDisableDirective: Unnecessary disabling of RSpec/NoExpectationExample.

That's non-sense!

> bundle exec rubocop spec/sequel/plugins/enum_values_spec.rb

Inspecting 1 file
W

Offenses:

spec/sequel/plugins/enum_values_spec.rb:167:9: W: [Correctable] Lint/RedundantCopDisableDirective: Unnecessary disabling of RSpec/NoExpectationExample.
							## rubocop:disable RSpec/NoExpectationExample
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/sequel/plugins/enum_values_spec.rb:168:8: C: RSpec/NoExpectationExample: No expectation found in this example.
							specify do ...
       ^^^^^^^^^^

1 file inspected, 2 offenses detected, 1 offense autocorrectable

[pirj]

Unnecessary disabling of RSpec/NoExpectationExample

This would be better reported to the parent project https://github.com/rubocop/rubocop/issues/

meta-programming is difficult for linters like RuboCop.

Not only. Also for people if its sole purpose is to make things dry rather than more readable.

Looking at the spec:

status_enum_values = %w[created selected canceled]

describe 'predicate_methods option' do
  convert_enum_values_to_predicate_methods = lambda do |enum_values|
    enum_values.map { |enum_value| :"#{enum_value}?" }
  end

  status_enum_predicate_methods =
    convert_enum_values_to_predicate_methods.call status_enum_values

  include_examples 'for all enum predicate methods',
    status_enum_predicate_methods,
    -> { expect(subject).to be false }

leads me to a thought that this can be written as

status_enum_values = %w[created selected canceled]

describe 'predicate_methods option' do
  status_enum_values.each do |status_enum_value|
    subject { item_class.new.public_send("#{status_enum_value}?") }

    it 'returns false' do
      expect(subject).to be(false)
    end
  end
end

Or:

status_enum_values = %w[created selected canceled]

shared_examples 'returns false' do |status_enum_value|
  subject { item_class.new.public_send("#{status_enum_value}?") }

  it 'returns false' do
    expect(subject).to be(false)
  end
end

status_enum_values.each do |status_enum_value|
  it_behaves_like 'returns false', status_enum_value
end

Am I missing something? Do those options look worse from your perspective, @AlexWayfer ?

[texpert]

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

Adding has_text? to language config doesn't help (I've also try just has_text) in cases like page.has_text?.

[pirj]

Can you provide an example of a spec with has_text?, @texpert ?

[texpert]

Can you provide an example of a spec with has_text?, @texpert ?

Well, here's one with has_link?, @pirj : - https://github.com/texpert/rails_6_rss_reader/commit/4923144a590cffce414cf0b84d4182dc52cc8c2d

When changing the has_link? to page.has_link?, we get a Rubocop failure:

Offenses:

spec/feauters/home_page_spec.rb:38:9: C: RSpec/NoExpectationExample: No expectation found in this example. (https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/NoExpectationExample)
        it 'has a `New Feed` link' do ...
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[pirj]

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

Are you confident that this code is correct, @texpert ? Isn't it a no-op in terms of expectations? Shouldn't it be expect(page).to have_link?('New Feed', exact: true)?

I have no awareness if page is the root page or how within affects that scope, unfortunately, but I'd say that within('body') doesn't do much scoping anyway, does it?

[texpert]

I am pretty confident, @pirj - see https://github.com/teamcapybara/capybara#querying and https://github.com/teamcapybara/capybara#scoping

Capybara's matchers are more concise and readable - https://rubydoc.info/github/teamcapybara/capybara/master/Capybara/Node/Matchers

Also, as a main reason for using Capybara's matchers, they are adhering to a default waiting behaviour - https://github.com/teamcapybara/capybara#asynchronous-javascript-ajax-and-friends.

[pirj]

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.

[pirj]

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.

[texpert]

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 :(

[pirj]

Mildly related discussion in the style guide

[pirj]

expect in after block

RSpec/ExpectInHook disapproves such style, @ShockwaveNN

[pirj]

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

[ydah]

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

[pirj]

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