Add RSpec/Rails/TravelAround and RSpec/RedundantAround cops

[r7kamura]

Since Rails 5.2, travel_back is called automatically, so you can easily do that by just calling it in before instead of using around with nested blocks.

• Remove time stubs after each test rails/rails#29860

In response to this change, I thought it would be a good idea to add a cop recommending the easier way when possible.

# bad
around do |example|
  freeze_time do
    example.run
  end
end

# good
before { freeze_time }

Unlike a typical Pull Request, the exception here is the addition of two cops.

RSpec/Rails/TravelAround applies the following autocorrection:

# before
around do |example|
  freeze_time do
    example.run
  end
end

# after
before { freeze_time }

around do |example|
  example.run
end

This may result in a meaningless around. As a measure to remove this, I have also added RSpec/RedundantAround cop to remove this kind of redundant around. I thought it would be more orthogonal to leave it to an independentcCop rather than have each cop take care of such a redundant around. This is why the both two cops are included in this one Pull Request.

For further reference, another possible idea related to this Rails change would be to remove the redundant travel_back as shown below:

# bad
after { travel_back }

# good

# bad
around do |example|
  foo
  example.run
  bar
  travel_back
end

# good
around do |example|
  foo
  example.run
  bar
end

In some cases, there may be travel-dependent hooks after this, so this would be an unsafe autocorrection.

---

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.

[pirj]

If I recall it correctly, travel_back is called from teardown, which is only called when RSpec's MinitestLifecycleAdapter is included. This is the case for Rails example groups, but not in general. Can you please confirm or refute this?
References rspec/rspec-rails#2263 rubocop/rubocop-rails#38

[r7kamura]

Oh, it's true. There are cases where #after_teardown is not called. Specifically, if the ExampleGroup does not have one of the following metadata[:type], then this Cop is not applicable because of this reason.

• :controller
• :feature
• :generator
• :helper
• :job
• :mailbox
• :mailer
• :model
• :request
• :request
• :request
• :routing
• :system
• :view

So, would it be better to add the above explanation and set Safe: false?
Or, instead, maybe we could limit Include to spec/{controllers,features,...}/**/*.rb, expecting config.infer_spec_type_from_file_location! to be enabled.

[pirj]

If the cop is focused to specs with Rails metadata it might be fine.
Just one scenario that is risky:

1. a spec has Rails metadata, but doesn't have really, like a type: :model for MyService spec
2. we suggest removing travel_back
3. they clean up metadata as redundant when it turns out the spec works without it
4. travel_back is now not called automatically on teardown 💥

[r7kamura]

I see. Based on the background above, I agree to set Safe: false, as false-positives can happen, even if they are a bit rare.

[pirj]

Looks great, thank you!

No errors, and offences look legit:

/Users/pirj/source/real-world-rspec/gitlabhq/spec/spec_helper.rb:258:3: C: [Correctable] RSpec/RedundantAround: Remove redundant around hook.
  config.around do |example| ...
  ^^^^^^^^^^^^^^^^^^^^^^^^^^

/Users/pirj/source/real-world-rspec/solidus/core/spec/models/spree/return_item/eligibility_validator/time_since_purchase_spec.rb:23:7: C: [Correctable] RSpec/Rails/TravelAround: Prefer to travel in before rather than around.
      travel_to(Time.current) { e.run }
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

57497 files inspected, 90 offenses detected, 90 offenses autocorrectable

[Darhazer]

What happens if the example is using freeze_time, is travel_back still called automatically? In other words, can we now avoid:

it 'does something' do
  freeze_time do
    # whole body of the test
  end
end

and just call freeze_time in the beginning? Note that this is valid only if the block is the last thing in the example, e.g. we don't rely on time being restored for next statements.
Also, in the above case, perhaps someone would prefer to still use the block, instead of just freeze_time statement

[Darhazer]

Overall looks good.

[Darhazer]

@r7kamura I'm sorry, there were few releases around the capybara extraction.
Would you mind bringing the Changelog up to date and I'll merge the PR

[pirj]

Thank you, @r7kamura !