Add RSpec/NoExpectationExample

[r7kamura]

This cop checks that each example has any expectation.

fixes #1319

Example

# bad
it do
  a?
end

# good
it do
  expect(a?).to be_truthy
end

Background

Trying it out on my Rails project, I found the following bad cases by this Cop:

it do
end

it 'succeeds' do
  subject
end

xit 'returns 200' do
end

then I corrected the code like this:

- it do
- end

it 'succeeds' do
-   subject
+   expect { subject }.not_to raise_error
end

- xit 'returns 200' do
- end
+ xit 'returns 200'

---

Before submitting the PR make sure the following are checked:

• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Updated documentation.
• Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
• The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

• Added the new cop to config/default.yml.
• The cop is configured as Enabled: pending in config/default.yml.
• The cop is configured as Enabled: true in .rubocop.yml.
• The cop documents examples of good and bad code.
• The tests assert both that bad code is reported and that good code is not reported.
• Set VersionAdded in default/config.yml to the next minor version.

If you have modified an existing cop's configuration options:

• Set VersionChanged in config/default.yml to the next major version.

[r7kamura]

Sorry I haven't read the related issues at all. I see that we had the same discussion a few days ago and even 10 years ago. It's just a coincidence that I created this pull request today.

• Show warning for examples that don't include an expectation rspec/rspec-core#759
• Cop idea: add a rule for it block without any expectations #208
• Cop idea: NoExpectations #1319

[r7kamura]

Thanks for the review comments! I learned a lot of information I didn't know 👍
I have made some improvements and hope you will take another look.

[pirj]

I don't feel that this cop should implement autocorrection. WDYT?

[pirj]

What else can it be?

[r7kamura]

For example, if the parent node is s(:send, nil, :it), its children are [nil, :it], so it can be nil or a Symbol.

[r7kamura]

I don't feel that this cop should implement autocorrection. WDYT?

I'm on the fence about that.

RSpec/EmptyExampleGroup, which has a similar concept, implements autocorrection, so I thought it might be a good idea for this cop to do the same. Sometimes this autocorrection is useful, sometimes it is not. This cop is marked as Safe: false, so it will not be unintentionally autocorrected unless users force it. But thoughtlessly erasing the code is indeed dangerous...

I actually used this cop's autocorrection in my Rails app because I had to change a large amount of code there, but I am thinking that maybe this autocorrection should be removed.

[pirj]

RSpec/EmptyExampleGroup, which has a similar concept, implements autocorrection, so I thought it might be a good idea for this cop to do the same

I think an empty example group has more chances to be a deliberate choice than the no expectations one. The latter is most likely a mistake.
For me, removing the empty example group doesn't seem to be an ideal autocorrection. But for the no expectations, it isn't for sure. Taking example from the NoExpectations ticket:

it { subject } # Just checks if calling `subject` doesn't raise any exceptions

it { validates_presence_of(:attribute) } # Oops, missing `is_expected.to`

It would be quite odd to remove those examples.

This cop is marked as Safe: false

I have to agree with this, unfortunately, with the Safe definition as "indicates whether the cop can yield false positives (by design) or not". It is unfortunate, though, that this cop would in most cases won't be run, and won't detect legitimate offences. Sadly, I don't have an idea how to overcome this.

that maybe this autocorrection should be removed

I fully support that.

Let me quickly run the cop against several codebases in real-world-rspec to see how severe and frequent false positives are.

[pirj]

57497 files inspected, 24233 offenses detected

I briefly went through, and tried categorizing cases that look like false positive/suspicious:

1. Legacy RSpec expectation notation:

# /Users/pirj/source/real-world-rspec/loomio/spec/models/user_spec.rb
    it "returns the stored name if deactivated_at is nil" do
      user.name.should_not == 'Deleted user'
    end

# /Users/pirj/source/real-world-rspec/rspec-mocks/spec/rspec/mocks/configuration_spec.rb
          it "does not warn about the should syntax" do
            RSpec.should_not_receive(:deprecate)
            Object.new.should_not_receive(:bees)
          end

2. Might be legitimate offences

# /Users/pirj/source/real-world-rspec/mastodon/spec/controllers/auth/registrations_controller_spec.rb
    it 'does nothing if user already exists' do
      Fabricate(:account, username: 'test')
      subject # Frankly, I'm struggling to understand where `subject` is defined
    end

# /Users/pirj/source/real-world-rspec/refinerycms/pages/spec/models/refinery/page_meta_data_spec.rb
      it 'meta_description' do
        page.respond_to?(:meta_description) # should be `expect(page).to respond_to(:meta_description)` ?
      end

3. Expectation helper method

    it 'tracks when sign in is nil' do
      user.update(current_sign_in_at: nil)
      sign_in user, scope: :user
      get :show

      expect_updated_sign_in_at(user)
    end

4. Skipped examples

      it "updates the requested expense" do
        expense = Expense.create! valid_attributes
        put :update, {:id => expense.to_param, :expense => new_attributes}, valid_session
        expense.reload
        skip("Add assertions for updated state")
      end

5. Rails/Minitest-style assertions

# /Users/pirj/source/real-world-rspec/publify/publify_core/spec/controllers/admin/articles_controller_spec.rb
      it "sends notifications on create" do
        u = create(:user, notify_via_email: true, notify_on_new_articles: true)
        u.save!
        ActionMailer::Base.deliveries.clear
        emails = ActionMailer::Base.deliveries

        post :create, params: { "article" => base_article }

        assert_equal(1, emails.size)
        assert_equal(u.email, emails.first.to[0])
      end

6. Alternative mocking library (e.g. Mocha)

# /Users/pirj/source/real-world-rspec/puppet/acceptance/lib/puppet/acceptance/classifier_utils_spec.rb
  it "provides a handle to the classifier service" do
    handle.expects(:perform_request).with(Net::HTTP::Get, '/hi', {})
    handle.get('/hi')
  end

7. No explicit expect { ... }.not_to raise_error

# /Users/pirj/source/real-world-rspec/rspec-core/spec/rspec/core/backtrace_formatter_spec.rb
      it "deals gracefully with a security error" do
        Metadata.instance_eval { @relative_path_regex = nil }
        with_safe_set_to_level_that_triggers_security_errors do
          self.formatter.__send__(:backtrace_line, __FILE__)
          # on some rubies, this doesn't raise a SecurityError; this test just
          # assures that if it *does* raise an error, the error is caught inside
        end
      end

8. Esoteric - definition of examples inside examples

# /Users/pirj/source/real-world-rspec/rspec-core/spec/rspec/core/configuration_spec.rb
      it 'successfully invokes the block' do
        RSpec.describe("group") { it "example 1" do; end}
        example = RSpec.world.example_groups.first.examples.first
        expect(example.metadata[:new_key]).to eq(:new_value)
      end

9. Missing configuration for well-known expectations

    it 'does not register any offenses' do
      expect_no_offenses(<<~RUBY, 'foo.rb')
        # cop will not read these contents
        gem('rubocop')
        gem('rubocop')
      RUBY
    end

Can we put this:

RSpec:
  Language:
    Expectations:
      - expect_correction
      - expect_no_offenses
      - expect_offense

into some additional YAML file so rubocop and other extensions could just include this, and this would be used in both this cop and MultipleExpectations?

It looks like an immense effort to fix this all. With Safe: false, I believe it's ok to keep the cop as it is.
I wouldn't even care about the long-deprecated monkey-patching object.should RSpec syntax and Minitest-style assertions.

[pirj]

Looks good, thanks!

[Darhazer]

The cop itself looks fine. I'm worried about extending the Language though

[Darhazer]

This change in Language potentially affects multiple cops (ExpectInHook, MultipleExpectations. StubbedMock). Perhaps it's worth mentioning it in the changelog. Also I'm not sure we should support the should expectations at all

[Darhazer]

@bquorning what do you think about extending the default language?

[bquorning]

I haven’t personally used the ‘should’ syntax since RSpec 3 was released, but from the RSpec documentation it sounds like it’s not being removed, even in v4. So I guess we should support it?

@pirj I imagine you might know about official RSpec plans?

[pirj]

All forms of should will be removed from RSpec 4 with no replacement.

[pirj]

Personally, I would not keep the burden of supporting should, as it was soft-deprecated nine years ago and was disabled in the generated config since then:

For RSpec 3, we considered the idea of disabling the old syntax by default, forcing users to opt-in to use it.

But I still see people use it, even those who joined software development less than nine years ago 😄

[Darhazer]

perhaps we should skip checking of pending or skipped examples in the first place.

[pirj]

Thank you, @r7kamura !