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

Good job!
Thanks for this awesome contribution 👍

[pirj]

WDYT of:

Timecop.freeze should be replaced with freeze_time when used
without arguments.
Where an ActiveSupport::Duration (e.g. 1.day) is passed to Timecop.travel, it
should be replaced with travel.
Likewise where time object is passed, the call should be replaced with travel_to.
In case when a parameter is passed to a Timecop.freeze, it should be replaced
with a combination of freeze_time with either travel or travel_to depending
on the type of the parameter.

E.g.:

Timecop.travel(time) { ... } => travel_to(time) { ... }
Timecop.freeze(time) { ... } => freeze_time { travel_to(time) { ... } }
Timecop.travel(time) => travel_to(time)
Timecop.freeze(time) => freeze_time; travel_to(time)

Timecop.travel(duration) { ... } => travel(duration) { ... }
Timecop.freeze(duration) { ... } => freeze_time { travel(duration) { ... } }
Timecop.travel(duration) => travel(duration)
Timecop.freeze(duration) => freeze_time; travel(duration)

Do you think this will work as expected?

I suppose it is possible to detect ActiveSupport::Duration if it's passed as an argument directly. There's an ambiguity if the parameter is not a value, and in this case, it should not autocorrect to retain safe status.

[sambostock]

TimeHelper always freezes time. In fact, freeze_time is implemented by calling travel_to with Time.now. I believe they took the approach of having fewer features to force tests to be clear and explicit.

This means Timecop.freeze(duration) and Timecop.freeze(time) can safely be replaced with travel(duration) and travel_to(time) respectively, without needing to call freeze_time.

We could try to analyze the arguments to check for simple cases matching the <number>.<unit> or <number>.<unit>.<relative direction> patterns and correct accordingly, but this would add a bunch of complexity. I'd be inclined to do this in a separate PR.

[dogweather]

In fact, freeze_time is implemented by calling travel_to with Time.now ...

Yikes, that's a big change in the API semantics going from Timecop to TimeHelper. (If I understand correctly.) Makes me even more convinced that this cop should not be enabled by default.

[pirj]

I'm not sure if there's a direct replacement for this:

started_at = Time.zone.now
Timecop.scale(1.hour) # 1 second wall clock time is 1 hour in tests
Hubspot.track(...) # takes seconds in production, takes fractions of a millisecond in test since it's stubbed
expect(Time.zone.now).to be_within(1.second).from(started_at)

[sambostock]

If the amount of time being measured and scaled up is large enough that Ruby can measure it at all, then it can be explicitly checked against, even if something like nsec is required.

In this case, I'm not sure that test would be checking anything worthwhile. It's coupling to how long the overhead of the stub is, rather than being explicit about what it's meant to be simulating.

Rather than scale time, I'd travel:

Hubspot.stub(:track) do
  travel(0.5.seconds) # after_teardown ensures this is safe
end

started_at = Time.zone.now
Hubspot.track(...)
expect(Time.zone.now).to be_within(1.second).from(started_at)

This way, if a test is asserting that a quick API call works fine, and a slow one is handled however it must be handled, one can just make it appear to take the time it should.

[pirj]

Shouldn't this be (cbase)?
Is it necessary?

[sambostock]

Turns out just cbase works. 👍

I do need it there, to make sure it matches Timecop and ::Timecop, but not Foo::Timecop. I've added a test to document that last case.

[pirj]

It seems that TimeHelpers were introduced in Rails 4.1, and by that time freeze_time was not yet available until Rails 4.2.
Unfortunately, RuboCop does not provide minor version, so we're left with two options:

• raise offences for Rails 4.0, including for freeze for Rails 4.1, which is unsafe;
• skip all the detection for Rails prior to 5.0

[sambostock]

Looks like you have a version error here, though your point is still valid.

• 4.1 introduces TimeHelpers#travel, TimeHelpers#travel_to, & TimeHelpers#travel_back
• 5.2 introduces TimeHelpers#freeze_time alias for TimeHelpers#travel_to(Time.now)
• 6.0 will introduce TimeHelpers#unfreeze_time alias for TimeHelpers#travel_back

Given freeze_time is implemented by passing Time.now to travel_to, every mention of freeze_time is safely replaced with travel_to(Time.now).

---

I'm surprised about the lack of minor version specification, as that means the experience we can provide is rather poor.

Ideally, we would provide the following experience:

• Version < 4.1: No offences or corrections
• 4.1 >= Versions < 5.2: Add offences & corrections, using only travel & travel_to
• 5.2 >= Versions < 6.0: Change offences & corrections to include freeze_time
• 6.0 >= Versions: Change offences & corrections to include unfreeze_time

With major version restrictions, that means we can only provide the following:

• Versions < 5.0: No offences or corrections
• 5.0 >= Versions < 6.0: Add offences & corrections, using only travel & travel_to
• 6.0 >= Versions: Change offences & corrections to include freeze_time & unfreeze_time

---

Can you point me in the right direction if I want to investigate version granularity? I did try to look into where RSpec gets the various :rails4/:rails5 contexts, but I couldn't figure it out. We mostly use Minitest style tests at Shopify 😅

[pirj]

It turns out it's possible to specify a more granular Rails version in specs, e.g. by defining it as

let(:rails_version) { '5.1.4' }

as it's possible to specify it as such in .rubocop.yml file.
I'm not sure if it's worth adding those as shared contexts, but this can be used in the cop specs, and in cop itself (I imagine something like a per-Rails-version strategy).
WDYT?

Sorry for late response. Are you still up for moving this forward?

[sambostock]

Yup 👍 I'll swing back around to this when I have time.

[pirj]

Why is this inside a in Rails < 6.0 context?

[pirj]

Is this really necessary to cover this case? I.e. it's detected in any context.

[sambostock]

This edge case can cause false positives for was_passed_block? and forces this change:

 def was_passed_block?(node)
   node.send_type? && node.parent &&
-    node.parent.block_type?
+    node.parent.block_type? && node.parent.send_node == node
 end

This is because the in both the cases of

foo { Timecop.return }
# (block
#   (send nil :foo)
#   (args)
#   (send (const nil :Timecop) :return)   ←
# )

and

Timecop.return { assert true }
# (block
#   (send (const nil :Timecop) :return)   ←
#   (args)
#   (send nil :assert (true))
# )

the parent node of

Timecop.return
#   (send (const nil :Timecop) :return)

is a block node, so we must make sure to distinguish between the two. Hence testing:

• passed no block
• passed no block, in a block
• passed a block
• passed a block, in a block

[pirj]

I think .foo case is covered in the example above already.

[sambostock]

I've removed the .foo, but I've left the example of just Timecop on its own, which I think is what I had meant this example to be in the first place.

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

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

[sambostock]

As per #38 (comment), I think we would mark it as an unsafe autocorrection. I don't think there are any cases where it's unsafe to say "don't use Timecop", unless it's an exotic use case where something like Timecop.scale is being used (and in that case, I'd expect the app to just disable the cop).

[sambostock]

@koic, @pirj, I noticed a merge conflict, so I've rebased again.

Still awaiting a decision with regards to merging or rejecting this PR. I've noticed custom cop PRs closed in favour of this one, so it would be good to make a decision.

[pirj]

LGTM! Thank you so much for the effort and dedication.

[sambostock]

@koic, @pirj, I was chatting with someone and this PR came up, so I rebased it again.

How can we move forward with this PR? It seems there is community consensus that this makes sense in rubocop-rails (as it is saying "don't use Timecop, use Rails!").

As an aside, I've opened #1139 to fix the "re-use" spellcheck error. I'm not sure why that fired, because the errors are in files this PR doesn't touch.

[pirj]

Ran the cop against https://github.com/pirj/real-world-rspec:

57497 files inspected, 1146 offenses detected, 131 offenses autocorrectable

All offences seem legit.

@koic what are your thoughts on this?

[pirj]

@koic what is your opinion on this cop?

[koic]

I apologize for the delayed response regarding the appropriateness of incorporating it into RuboCop Rails. After giving it some thought, I think that creating a separate custom RuboCop gem for Timecop would be more appropriate, as Timecop has never been adopted as a standard in Rails. Another approach could be to offer it as a custom gem that facilitates migration from Timecop to Rails, which I think could be a viable option.

[rwojsznis]

I'm silently observing this PR since 2019 and I referred this PR multiple times during various code reviews 😆 and oh boy what a sad decision 😢

I don't think if number of downloads can be correlated with adoption, but rails have 488M downloads and timecop have 182M; now it would be great to know how many Rails project indeed use timecop, which - at some point - I think was the standard for fiddling with time in tests :).

anyone planning to salvage all this work into new gem? 👀 rubocop-rails-dlc-2024-part1 😆

[pirj]

This cop would fit fine into our future rubocop-rspec_rails, WDYT @rubocop/rubocop-rspec ?

[pirj]

rubocop/rubocop-rspec_rails#48 revived in another extension