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. :)

[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

[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 ?

[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.

[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.