Add Timecop cop

[sambostock]

This cop makes Timecop illegal, in favour of ActiveSupport::Testing::TimeHelpers.

Specifically,

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

Benchmark

I ran a quick 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.

---

This is my first custom Cop, so if I've made a mistake, or simply not chosen the best way of doing something, let me know!

---

Before Merging ⚠️

• [ ] Address FIXME comments
  • [ ] Is there a better way to get the range for the receiver and message?

    def receiver_and_message_range(node)
      # FIXME: There is probably a better way to do this
      # Just trying to get the range of `Timecop.method_name`, without args, or block, or anything
      node.location.expression.with(end_pos: node.location.selector.end_pos)
    end
    

  • [ ] Should cases that are not autocorrected be tested? If so, is there an explicit way?

    # FIXME: Is this how NOT autocorrecting something should be tested?
    it 'does not autocorrect' do
      source = 'Timecop.freeze(123) { }'
    
      expect(autocorrect_source(source)).to(eq(source))
    end
    

• [ ] Are there any other special cases that should receive specific offences or corrections? e.g. Should Timecop.scale be mentioned?

---

Closes #20 Closes rubocop-hq/rspec-style-guide#71

[andyw8]

Great work! A first thought: ActiveSupport::Testing::TimeHelpers isn't available be default, (at least not in rspec-rails anyway), so the autocorrect would could break some tests. It's probably not reasonable for the cop to adjust the test runner config, but it might useful to give some direction, for example:

If using RSpec, add the following to your `spec_helper` (and `rails_helper` too, if it exists):

  RSpec.configure do |config|
    config.include ActiveSupport::Testing::TimeHelpers
  end

[sambostock]

Ooh, interesting 🤔

Where would you put that message? Would you attach it to every offence? I feel like that might be a little noisy, especially for projects which aren't using RSpec, where the correction should be safe.

Perhaps it makes sense to have the autocorrect as an option that is off by default? Or maybe an option to suppress the RSpec message?

[andyw8]

Perhaps it could include a link to a page with more details? (I'm not sure if any other cops do anything like that).

[sambostock]

Perhaps. I can't think of any cops that do that off the top of my head.

It occurs to me that maybe this concern is solved by the cop not being enabled by default? We could include a note about RSpec in the docs, so when someone enables it, they know how to fix any issues that come up from improper config.

[sambostock]

I've made some updates

• Add documentation & examples
• Fix formatting and other offences
• Remove erroneous Timecop.return with block correction
• Add commented out Rails 6 specs for unfreeze_time correction

[sambostock]

Looks like RSpec doesn't run the after_teardown hook automatically, so TimeHelpers isn't safe without using blocks outside of Minitest.

I'm trying to figure out how to get it to run. It looks like there is RSpec::Rails::MinitestLifecycleAdapter, but I can't seem to figure out how to properly include it. I expected the following to work, but it fails to complains about uninitialized constant RSpec::Rails...

RSpec.configure do |config|
  config.include RSpec::Rails::MinitestLifecycleAdapter # 💥
  config.include ActiveSupport::Testing::TimeHelpers
end

[Drenmi]

Is there a better way to get the **range for the receiver and message?

Considering the output from ruby-parse:

Timecop.method_name(:foo)
       ~ dot                
        ~~~~~~~~~~~ selector 
                        ~ end
                   ~ begin          
~~~~~~~~~~~~~~~~~~~~~~~~~ expression

I think your approach seems really good. 👍

Should cases that are not autocorrected be tested? If so, is there an explicit way?

We just merged this feature. 🙂

it 'adds an offense but does not autocorrect' do
  expect_offense(<<-RUBY.strip_indent)
    Timecop.freeze(123)
    ^^^^^^^^^^^^^^^^^^^ Use `travel` or `travel_to` instead of `Timecop.freeze`
  RUBY

  expect_no_correction
end

[pirj]

Looks like RSpec doesn't run the after_teardown hook automatically, so TimeHelpers isn't safe without using blocks outside of Minitest.

Looks like a shortcoming in rspec-rails, I believe it should do that out of the box.

[bquorning]

Do Rails` time helpers have an equivalent of Timecop’s safe mode? https://github.com/travisjeffery/timecop/blob/master/README.markdown#timecopsafe_mode

I have seen people resetting various time helpers and other global state changes in e.g. a global after(:each) block. And I much prefer forcing each test that changes global state to reset it themselves.

[sambostock]

@bquorning I actually started looking into implementing a Safe Mode in TimeHelpers to force you to use blocks, but realized it's unneeded, due to TimeHelpers#after_teardown, which makes sure travel_back is always called.

As discussed above though, RSpec does not run the after_teardown hook automatically. I'll open an issue in rspec-rails to see if that can be rectified, or if at least there is a workaround to ensure it does run. I would like to make sure we list such a workaround before merging this.

---

@pirj You may want to edit your response above, as there is a bunch of email noise. Might also want to check on that api_key it lists 😅

[dkniffin]

@sambostock Any update on this or the related rspec-rails issue? I didn't even know this feature existed, but after running into an issue with Timecop.freeze, googling around and ending up here, I would love to start using freeze_time and have it automatically cleaned up so I don't have this issue again.

[dogweather]

@dkniffin freeze_time presents the same issues as Timecop.freeze, but is more dangerous. It lacks Timecop's safe mode to enforce the block style and its auto-cleanup.

[pirj]

Would you be interested in completing this @sambostock ?

[sambostock]

@dogweather, see my comment above addressing why freeze_time has no safe mode.

@pirj Things got in the way and this slipped through the cracks, but yes, I'd like to. Looks like the main blocker is ensuring rspec-rails respects TimeHelper#after_teardown. I'll throw an issue together there and link to this PR.

That said, there is always the option of merging this with the caveat of

don't enable this rule if you're using RSpec~

[pirj]

It's been a while and I'm lost the context, but I recall there's nothing to fix on rspec-rails side.

[sambostock]

@pirj sorry for the... multi-year delay... 😅

I've updated the branch. The tests should be passing, docs should be complete (and include the RSpec caveats). Unless I've missed anything, this should be ready to go.

[dogweather]

Nice to see this getting worked on. I had no idea that this alternative to Timecop existed. And I just tried to use Timecop in my Rails 6 app and it failed to work for some reason - no time to debug it.

[sambostock]

After running some testing against some of our code, I've discovered there are edge cases where the correction is arguably unsafe. For example:

-Timecop.freeze do
+freeze_time do
  # ...
- Timecop.travel(time_or_duration)
+ Timecop.travel(time_or_duration) # rubocop:todo Rails/Timecop
  # ...
end

Timecop.freeze is autocorrected to freeze_time, but Timecop.travel(time_or_duration) can't be safely corrected because we can't statically know if we should correct to travel_to(time) or travel(duration).

Unfortunately, if Timecop's safe mode is enabled, that breaks the test, because Timecop.travel is no longer in a block where it knows it will be cleaned up.

Another thing I noticed is that in the examples of Timecop.travel I came across, most of them (at a glance) seemed to be passed a time, not a duration. It would be very useful for our large codebases if we could simply unsafely correct and simply rely on the tests to tell us if the correction is right.

-Timecop.freeze do
+freeze_time do
  # ...
- Timecop.travel(probably_time_but_maybe_duration)
+ travel_to(probably_time_but_maybe_duration) # might blow up
  # ...
end

Therefore, I wonder if we should just mark the cop as an unsafe autocorrection and make assumptions in the correction code.

[koic]

Great job. On the other hand, Timecop is a gem different from Rails, and its API and specifications are those of Timecop. It may be better to create a new gem with a name like rubocop-timecop gem. The following is a different case, but it may be helpful: https://github.com/rubocop/rubocop-rails/pull/152#issuecomment-554839724

rubocop-rails was once packed in the RuboCop core, but with a separate maintenance. And I think that RuboCop Rails shouldn't depend on Timecop's API, just as Rails doesn't depend on Timecop. Thank you.

[sambostock]

Thanks for the info, @koic! I agree that if this cop was advising on the correct use of Timecop, it would make sense as an extension.

However, given this cop's purpose is not to advise on how to use Timecop, but instead to advise against its use, in favour of Rails' own TimeHelpers, do you still think it does not belong in rubocop-rails? I'm not sure it makes sense to create an extension called rubocop-timecop whose only cop would be "do not use Timecop if you are using Rails".

Way back in 2019, this started as a cop I added to an internal Rubocop extension. However, I realized it had nothing to do with our own style guide, and instead would apply to any Rails application. As far as I can tell, no Rails application should need to add the Timecop gem, because the TimeHelpers feature is provided out-of-the-box, and this cop's goal is to highlight that for those who don't know about it.

Perhaps it is simply the name of the cop that is too broad? Would Rails/PreferTimeHelpersOverTimecop be more appropriate?

[pirj]

I side with @sambostock that rubocop-rails is a good fit for this cop. Another similar example of a cop would be one that would suggest using use_transactional_tests to replace DatabaseCleaner.

This cop started in this rspec-style-guide issue. Timecop is not intended to be used in production code, just tests, and can also be used with both RSpec and Minitest.

One note that Rails make regarding this is:

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 sounds like "use what is built-in in Rails, and only replace its parts with what third-party provide when absolutely necessary".

[andyw8]

+1 for having this within rubocop-rails. It's one of a very small number of gems that are so embedded within the ecosystem that it led to Rails adding a feature to make it redundant.

[andyw8]

Therefore, I wonder if we should just mark the cop as an unsafe autocorrection and make assumptions in the correction code.

This seems reasonable to me. For any non-trivial cop, it's very hard to guarantee that it's always safe for autocorrection.

[sambostock]

@koic, @pirj any consensus about if this belongs in rubocop-rails? I personally agree with @pirj's opinion above, based on this being a cop to advise not using Timecop, and instead using built-in Rails features (as opposed to a cop about how to use Timecop).

There is also my question (https://github.com/rubocop/rubocop-rails/pull/38#issuecomment-1089747712) about marking the cop as unsafe.

[pirj]

We’re planning on extracting some cops into a new ‘rubocop-rspec-rails’ extension, but I still think that Minitest users would benefit from having this cop in ‘rubocop-rails’.

[sambostock]

Okay, sounds good @pirj!

I've rebased this branch on the latest master.

Is there anything else that needs to be addressed before we can merge this?

The only thing I can think of is if the cop should be marked as safe or not (see https://github.com/rubocop/rubocop-rails/pull/38#issuecomment-1089703650).

[pirj]

I support the idea of making a cop as unsafe for autocorrection. There's an open issue in rubocop about dynamic safety. It would be nice if we could tell if we're performing an unsafe autocorrection only in certain cases. It's a long shot, though, to implement this mechanism, I don't presently have capacity for that.

Alternatively, there could be two cops, one that handles offences and safe corrections, and another that only handles cases with unsafe. Would it work, @sambostock ? How do you estimate the effort of such a split?

[pirj]

@koic What's your take on having this cop in this repo?

[sambostock]

Alternatively, there could be two cops, one that handles offences and safe corrections, and another that only handles cases with unsafe. Would it work, @sambostock? How do you estimate the effort of such a split?

Given how long this PR has been open (2 days shy of 4 years), I'm inclined to do the following:

• Start off by marking the entire cop as unsafe, because that's the "safer" stance and the fastest to deliver value to consumers.
• If we think there's value in splitting it, then we iterate and extract a "safe" cop, and perhaps rename accordingly.
• Likewise, if dynamic safety wants to be explored, then we iterate and explore supporting dynamic safety in this cop.

My concern is that if we add further complexity to the requirements it will be another few years until we deliver any value at all.

[pirj]

I'm completely fine with making the cop SafeAutocorrect: false, but I'm usually very opposed to making it Safe: false as this effectively makes the cop disabled for everyone unless they explicitly opt in, which is not usually the case.