Add new RSpec/EmptyLineAfterSharedInclusion cop

[sucicfilip]

Add a new RSpec/EmptyLineAfterSharedInclusion cop that checks for an empty line after shared example inclusion (include_context, include_examples, it_behaves_like, it_should_behave_like), or a group of them.

Modeled after Layout/EmptyLinesAfterModuleInclusion from rubocop - consecutive inclusions can be grouped together without empty lines between them, but an empty line is required after the last one before other code.

# bad
RSpec.describe User do
  include_context 'with_authentication'
  it { does_something }
end

# good
RSpec.describe User do
  include_context 'with_authentication'

  it { does_something }
end

# good - grouped inclusions
RSpec.describe User do
  include_context 'with_authentication'
  include_context 'with_authorization'

  it { does_something }
end

---

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: "<<next>>" in default/config.yml.

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

• Set VersionChanged: "<<next>>" in config/default.yml.

[bquorning]

Thank you for the contribution 👍

Never mind the failing CI – I’ve opened a bug report with RuboCop: rubocop/rubocop#15010

[bquorning]

cc @pirj

[pirj]

At a glance - would the cop’s code benefit from using the EmptyLineSeparation mixin?

[sucicfilip]

At a glance - would the cop’s code benefit from using the EmptyLineSeparation mixin?

Seems to me it would, nice! Will look into this later.

[pirj]

Tried on real-world-rspec:

# brew/Library/Homebrew/test/cask/cask_loader/from_api_loader_spec.rb
  describe "#load" do
    shared_examples "loads from API" do |cask_token, caskfile_only:|
      include_context "with API setup", cask_token
      let(:cask_from_api) { api_loader.load(config: nil) }

      it "loads from JSON API" do
        expect(cask_from_api).to be_a(Cask::Cask)

# canvas/migration/helpers/selective_content_formatter_spec.rb
  context "course copy" do
    include_context "lti2_course_spec_helper"
    let(:formatter) { Canvas::Migration::Helpers::SelectiveContentFormatter.new(@migration, "https://example.com", global_identifiers: true) }
    let(:top_level_items) do
      [{ type: "course_settings", property: "copy[all_course_settings]", title: "Course Settings" },
       { type: "syllabus_body", property: "copy[all_syllabus_body]", title: "Syllabus Body" },
       { type: "context_modules", property: "copy[all_context_modules]", title: "Modules", count: 1, sub_items_url: "https://example.com?type=context_modules" },

# canvas-lms/spec/lib/lti/message_handler_name_bookmarker_spec.rb
describe Lti::MessageHandlerNameBookmarker do
  include_context "name_bookmarker_base_shared_examples"
  include LtiSpecHelper

  it_behaves_like "a bookmarker for models with names" do

# canvas-lms/spec/models/quizzes/quiz_statistics/student_analysis_spec.rb
  end

  let(:report_type) { "student_analysis" }

  it_behaves_like "Quizzes::QuizStatistics::Report"
  before(:once) { course_factory }

  def csv(opts = {}, quiz = @quiz)

# cartodb/services/importer/spec/unit/connection_manager_spec.r
describe Carto::ConnectionManager do

  include_context 'with MessageBroker stubs'
  let(:user) { create(:carto_user_light) }
  let(:connection_manager) { Carto::ConnectionManager.new(user) }

# chatwoot/spec/controllers/shopify/callbacks_controller_spec.rb
    context 'when successful' do
      include_context 'with stubbed account'
      before do
        allow(auth_code_strategy).to receive(:get_token).and_return(token_response)

# chef/spec/functional/mixin/user_context_spec.rb
    context "when a non-nil user is specified" do
      include_context "a non-admin Windows user"
      context "when a username different than the process user is specified" do
        let(:username_to_impersonate) { test_user }
        let(:username_to_impersonate_password) { test_password }
        context "when an explicit domain is given with a valid password" do
          let(:domain_to_impersonate) { test_domain }
          it "uses different credentials for other network connections" do

# discourse/plugins/discourse-chat-integration/spec/system/create_channel_spec.rb
RSpec.describe "Create channel", type: :system do
  fab!(:admin)

  include_context "with dummy provider"
  let(:manager) { DiscourseChatIntegration::Manager }

module QA
  RSpec.describe 'Create', :skip_live_env, feature_category: :web_ide do
    describe 'Add first file in Web IDE' do
      include_context 'Web IDE test prep'
      let(:project) { create(:project, :with_readme, name: 'webide-create-file-project') }

# mastodon/spec/requests/cache_spec.rb
      describe endpoint do
        before { get endpoint }

        it_behaves_like 'cachable response'
        it_behaves_like 'language-dependent' if TestEndpoints::LANGUAGE_DEPENDENT.include?(endpoint)
      end

# rubocop/spec/rubocop/cop/team_spec.rb
    context 'when Cop#on_* raises an error' do
      include_context 'mock console output'
      before do
        allow_any_instance_of(RuboCop::Cop::Style::NumericLiterals)

etc

Total: 452 offences, I’ve only glanced over those ones above.

Some of those feel off.

[bquorning]

Disregard the Edge RuboCop failure. It’s another false positive, and I have opened an issue to get it fixed: rubocop/rubocop#15028

[sucicfilip]

Could you be more specific about which ones feel off?

Personally, I don't agree with this being flagged:

it_behaves_like 'cachable response'
it_behaves_like 'language-dependent' if TestEndpoints::LANGUAGE_DEPENDENT.include?(endpoint)

[pirj]

After having a fresh look all offences I’ve posted (except the las one that you’ve highlighted, too) are totally reasonable.

There’s one where include_context comes before include - but maybe there’s a reason for this, and ordering is out of question for this cop anyway.

Can you please check other offences to try to see if there are more edge cases?

[sucicfilip]

I've updated the cop's logic to take conditionals into account. Now the number of offenses on real-world-rspec is down to 448.

[bquorning]

Should we also show the corresponding "good" version of this example?

[bquorning]

This is a lot of extra work for almost the same check as lines 62–67, but after looking closer I cannot find a better way to implement it. At least not without changing EmptyLineSeparation more than I like.