Add RSpecRails/Timecop cop

[pirj]

Add Timecop cop

This PR is a revival of rubocop/rubocop-rails#38
All due credit for the implementation goes to @sambostock

Fixes rubocop/rubocop-rails#20
Style guide rubocop/rspec-style-guide#71

Changes from the original PR:

• changed unfreeze_time to travel_back (since it's an alias anyway)
• taught the cop about travel_back with a block that appeared in Rails 6.1

What

    Timecop.freeze should be replaced with freeze_time (autocorrected)
    Timecop.freeze(...) should be replaced with travel or travel_to
    Timecop.return should be replaced with travel_back (autocorrected)
    Timecop.travel should be replaced with travel or travel_to.
        Explicitly travelling again should be used instead of relying on time continuing to flow
    Timecop should not appear anywhere

Why?

Rails projects already depend on ActiveSupport, which gives them most of the functionality Timecop provides.

While Timecop does provide some functionality TimeHelpers doesn't, the desired behaviour is better expressed by being explicit.
For instance, Timecop.scale should probably be replaced by explicitly calling travel or travel_to.

Disallowing Timecop in favour of TimeHelpers allows projects to remove Timecop as a dependency.

While Rails is an omakase stack, it still allows you to replace certain frameworks or libraries with alternatives. It just doesn’t require you to.

I might be misinterpreting this statement to my liking, please correct me if so. But from my perspective, it means "use what is built-in in Rails, and only replace its parts with what third-party provide when absolutely necessary".

Also, from my personal experience (from 7 years back) Timecop was contributing to some peculiar thread-related issues, suggesting that it's not thread-safe. I may be mistaken after all those years, so please take this note lightly.

Benchmark

A benchmark to compare the two, across 100,000 trials.

                                       user     system      total        real
Timecop.freeze { }                 0.763503   0.002264   0.765767 (  0.767017)
freeze_time    { }                 3.617230   0.031886   3.649116 (  3.706262)
Timecop.freeze and Timecop.return  0.939708   0.005576   0.945284 (  0.958756)
freeze_time    and travel_back     2.707030   0.018956   2.725986 (  2.741195)

While TimeHelpers is certainly much slower, a difference of a few seconds across a 100,000 tests is not a meaningful penalty.
NOTE: this benchmark is 5 years old.

---

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 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]

cc @sambostock @andyw8

[bquorning]

I do not understand this part of the comments. I think what is required is that #travel_back is called in an after block in the spec configuration (be it rails_helper or spec_helper or other).

[pirj]

You're right. It's not that TimeHelpers need to be required from rails_helper, or rails_helper should be required.
Minitest teardown hooks will only run for Rails example groups, those where specific metadata is set (:model, :controller etc), manually in each spec, or with define_derived_metadata.

This note should be changed accordingly.

[pirj]

On a slightly different note, the RSpec maintainers plan to sunset setting this metadata automatically with define_derived_metadata sometimes in the future, I think our users will either:

1. define derived metadata on their own
2. start setting metadata explicitly

We don't know for sure, so relying on explicitly set metadata to skip or proceed with this inspection is not a viable option.

Maybe rspec-rails should wrap TimeHelpers module with their own that would inject after { reset_time_helpers! }? We could then write a cop that suggests using this module instead.

[bquorning]

I really like that Timecop has a safe mode. The equivalent would be that you can only call #travel and #freeze_time with a block, and the teardown / after block is not necessary.

That said, I can just disable this cop in projects where I prefer to use Timecop.

[pirj]

@bquorning We may add a separate cop that would enforce using only safe AS TimeHelpers methods variants.

Honestly, I can’t say I fully understand the point of the safe mode really is. Is it to reset stubs after the example was run?
Because RSpec has a special hook which (similar to native Rails testing) resets them. You don’t have to manually travel_back in an after hook.

To my best memory, the cleanup only happens to Rails example groups (those for models, controllers etc). If you include and use TestHelpers outside of a Rails example group, the time won’t travel back. Some discussion in rspec-rails.

[sambostock]

Timecop has a safe mode

Timecop's safe mode is there to ensure changes to time don't leak out of tests (or whatever code is invoking Timecop).

TimeHelpers does not have a safe mode because it is only meant for use in tests, and unconditionally calls travel_back in after_teardown, so it is always safe¹. The block mode only serves as a convenience to undo a time change within the body of the same test.

test "something" do
  freeze_time do
    # do something with frozen time
  end
  
  # do something with normal time
end

Footnotes

1. As long as the test framework reliably calls the after_teardown hook. ↩

[bquorning]

As long as the test framework reliably calls the after_teardown hook.

I guess my question is: Does RSpec unconditionally call the after_teardown hook? Or do I need to configure a global after block?

[pirj]

Does RSpec unconditionally call the after_teardown hook

Only in Rails example groups where the MinitestLifecycleAdapter is included.

[sambostock]

Is TimeHelpers only included in Rails example groups as well?

[pirj]

TimeHelpers are not included by default.