From d161a5f0b7f365a61fb15a83e54ee7a272491d78 Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Sat, 16 Feb 2019 22:28:52 -0500 Subject: [PATCH] Update Timecop Cop - 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 --- lib/rubocop/cop/rails/timecop.rb | 118 +++++++++++++++++++++---- manual/cops_rails.md | 64 ++++++++++++++ spec/rubocop/cop/rails/timecop_spec.rb | 77 ++++++++++++---- 3 files changed, 227 insertions(+), 32 deletions(-) diff --git a/lib/rubocop/cop/rails/timecop.rb b/lib/rubocop/cop/rails/timecop.rb index 4de0ca0a53..76216fa968 100644 --- a/lib/rubocop/cop/rails/timecop.rb +++ b/lib/rubocop/cop/rails/timecop.rb @@ -3,24 +3,105 @@ module RuboCop module Cop module Rails + # This cop disallows all usage of `Timecop`, in favour of + # `ActiveSupport::Testing::TimeHelpers`. + # + # ## Migration + # `Timecop.freeze` should be replaced with `freeze_time` when used + # without arguments. Where a `duration` has been passed to `freeze`, it + # should be replaced with `travel`. Likewise, where a `time` has been + # passed to `freeze`, it should be replaced with `travel_to`. + # + # `Timecop.return` should be replaced with `travel_back`, when used + # without a block. `travel_back` does not accept a block, so where + # `return` is used with a block, it should be replaced by explicitly + # calling `freeze_time` with a block, and passing the `time` to + # temporarily return to. + # + # `Timecop.scale` should be replaced by explicitly calling `travel` or + # `travel_to` with the expected `durations` or `times`, respectively, + # rather than relying on allowing time to continue to flow. + # + # `Timecop.travel` should be replaced by `travel` or `travel_to` when + # passed a `duration` or `time`, respectively. As with `Timecop.scale`, + # rather than relying on time continuing to flow, it should be travelled + # to explicitly. + # + # All other usages of `Timecop` are similarly disallowed. + # + # ## Caveats + # + # Note that if using RSpec, `TimeHelpers` are not included by default, + # and must be manually included by updating `spec_helper` (and + # `rails_helper` too, if it exists): + # + # ```ruby + # RSpec.configure do |config| + # config.include ActiveSupport::Testing::TimeHelpers + # end + # ``` + # + # @example + # # bad + # Timecop + # + # # bad + # Timecop.freeze + # Timecop.freeze(duration) + # Timecop.freeze(time) + # + # # good + # freeze_time + # travel(duration) + # travel_to(time) + # + # # bad + # Timecop.freeze { assert true } + # Timecop.freeze(duration) { assert true } + # Timecop.freeze(time) { assert true } + # + # # good + # freeze_time { assert true } + # travel(duration) { assert true } + # travel_to(time) { assert true } + # + # # bad + # Timecop.travel(duration) + # Timecop.travel(time) + # + # # good + # travel(duration) + # travel_to(time) + # + # # bad + # Timecop.return + # Timecop.return { assert true } + # + # # good + # travel_back + # travel_to(time) { assert true } class Timecop < Cop - FREEZE_MESSAGE = 'Use `freeze_time` instead of `Timecop.freeze`' - FREEZE_WITH_ARGUMENTS_MESSAGE = 'Use `travel` or `travel_to` instead of `Timecop.freeze`' - RETURN_MESSAGE = 'Use `travel_back` instead of `Timecop.return`' - TRAVEL_MESSAGE = 'Use `travel` or `travel_to` instead of `Timecop.travel`. If you need time to keep flowing, ' \ - 'simulate it by travelling again.' - MSG = 'Use `ActiveSupport::Testing::TimeHelpers` instead of `Timecop`' - - FREEZE_TIME = 'freeze_time' - TRAVEL_BACK = 'travel_back' - - TIMECOP_PATTERN_STRING = <<~PATTERN - (const {nil? (:cbase)} :Timecop) - PATTERN + FREEZE_MESSAGE = + 'Use `freeze_time` instead of `Timecop.freeze`'.freeze + FREEZE_WITH_ARGUMENTS_MESSAGE = + 'Use `travel` or `travel_to` instead of `Timecop.freeze`'.freeze + RETURN_MESSAGE = + 'Use `travel_back` instead of `Timecop.return`'.freeze + TRAVEL_MESSAGE = + 'Use `travel` or `travel_to` instead of `Timecop.travel`. If you ' \ + 'need time to keep flowing, simulate it by travelling again.'.freeze + MSG = + 'Use `ActiveSupport::Testing::TimeHelpers` instead of ' \ + '`Timecop`'.freeze + + FREEZE_TIME = 'freeze_time'.freeze + TRAVEL_BACK = 'travel_back'.freeze + + TIMECOP_PATTERN_STRING = '(const {nil? (:cbase)} :Timecop)'.freeze def_node_matcher :timecop, TIMECOP_PATTERN_STRING - def_node_matcher :timecop_send, <<~PATTERN + def_node_matcher :timecop_send, <<-PATTERN.strip_indent (send #{TIMECOP_PATTERN_STRING} ${:freeze :return :travel} $... @@ -88,12 +169,17 @@ def autocorrect_freeze(corrector, node, arguments) end def autocorrect_return(corrector, node, _arguments) + return if was_passed_block?(node) + corrector.replace(receiver_and_message_range(node), TRAVEL_BACK) end + def was_passed_block?(node) + node.send_type? && node.parent && + node.parent.block_type? && node.parent.send_node == node + end + 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 end diff --git a/manual/cops_rails.md b/manual/cops_rails.md index db810a77fc..850d94ec86 100644 --- a/manual/cops_rails.md +++ b/manual/cops_rails.md @@ -2160,6 +2160,70 @@ EnforcedStyle | `flexible` | `strict`, `flexible` * [https://github.com/rubocop-hq/rails-style-guide#time](https://github.com/rubocop-hq/rails-style-guide#time) * [http://danilenko.org/2012/7/6/rails_timezones](http://danilenko.org/2012/7/6/rails_timezones) +## Rails/Timecop + +Enabled by default | Supports autocorrection +--- | --- +Enabled | Yes + +This cop disallows all usage of `Timecop`, in favour of +`ActiveSupport::Testing::TimeHelpers`. + +FIXME: Add moar + +Note that if using RSpec, `TimeHelpers` are not included by default, +and must be manually included by updating `spec_helper` (and +`rails_helper` too, if it exists): + +```ruby +RSpec.configure do |config| + config.include ActiveSupport::Testing::TimeHelpers +end +``` + +### Examples + +```ruby +# bad +Timecop + +# bad +Timecop.freeze +Timecop.freeze(duration) +Timecop.freeze(time) + +# good +freeze_time +travel(duration) +travel_to(time) + +# bad +Timecop.freeze { assert true } +Timecop.freeze(duration) { assert true } +Timecop.freeze(time) { assert true } + +# good +freeze_time { assert true } +travel(duration) { assert true } +travel_to(time) { assert true } + +# bad +Timecop.travel(duration) +Timecop.travel(time) + +# good +travel(duration) +travel_to(time) + +# bad +Timecop.return +Timecop.return { assert true } + +# good +travel_back +travel_to(time) { assert true } +``` + ## Rails/UniqBeforePluck Enabled by default | Supports autocorrection diff --git a/spec/rubocop/cop/rails/timecop_spec.rb b/spec/rubocop/cop/rails/timecop_spec.rb index 747b117879..225030df5a 100644 --- a/spec/rubocop/cop/rails/timecop_spec.rb +++ b/spec/rubocop/cop/rails/timecop_spec.rb @@ -7,7 +7,7 @@ context 'without a block' do context 'without arguments' do it 'adds an offense' do - expect_offense(<<~RUBY) + expect_offense(<<-RUBY.strip_indent) Timecop.freeze ^^^^^^^^^^^^^^ Use `freeze_time` instead of `Timecop.freeze` RUBY @@ -16,11 +16,26 @@ it 'autocorrects to `freeze_time`' do expect(autocorrect_source('Timecop.freeze')).to(eq('freeze_time')) end + + context 'spread over multiple lines' do + it 'adds an offense' do + expect_offense(<<-RUBY.strip_indent) + Timecop + ^^^^^^^ Use `freeze_time` instead of `Timecop.freeze` + .freeze + RUBY + end + + it 'autocorrects to `freeze_time`' do + expect(autocorrect_source("Timecop\n .freeze")) + .to(eq('freeze_time')) + end + end end context 'with arguments' do it 'adds an offense' do - expect_offense(<<~RUBY) + expect_offense(<<-RUBY.strip_indent) Timecop.freeze(123) ^^^^^^^^^^^^^^^^^^^ Use `travel` or `travel_to` instead of `Timecop.freeze` RUBY @@ -37,26 +52,26 @@ context 'with a block' do context 'without arguments' do it 'adds an offense' do - expect_offense(<<~RUBY) + expect_offense(<<-RUBY.strip_indent) Timecop.freeze { } ^^^^^^^^^^^^^^ Use `freeze_time` instead of `Timecop.freeze` RUBY end it 'autocorrects to `freeze_time`' do - expect(autocorrect_source('Timecop.freeze { }')).to(eq('freeze_time { }')) + expect(autocorrect_source('Timecop.freeze { }')) + .to(eq('freeze_time { }')) end end context 'with arguments' do it 'adds an offense' do - expect_offense(<<~RUBY) + expect_offense(<<-RUBY.strip_indent) Timecop.freeze(123) { } ^^^^^^^^^^^^^^^^^^^ Use `travel` or `travel_to` instead of `Timecop.freeze` RUBY end - # FIXME: Is this how NOT autocorrecting something should be tested? it 'does not autocorrect' do source = 'Timecop.freeze(123) { }' @@ -69,34 +84,64 @@ describe 'Timecop.return' do context 'without a block' do it 'adds an offense' do - expect_offense(<<~RUBY) + expect_offense(<<-RUBY.strip_indent) Timecop.return ^^^^^^^^^^^^^^ Use `travel_back` instead of `Timecop.return` RUBY end - it 'autocorrects to `travel_back`' do - expect(autocorrect_source('Timecop.return')).to(eq('travel_back')) + context 'in Rails < 6.0', :rails5 do + it 'autocorrects to `travel_back`' do + expect(autocorrect_source('Timecop.return')).to(eq('travel_back')) + end + + context 'inside a block' do + it 'autocorrects to `travel_back`' do + expect(autocorrect_source('foo { Timecop.return }')) + .to(eq('foo { travel_back }')) + end + end end + + # context 'in Rails > 6.0', :rails6 do + # it 'autocorrects to `unfreeze`' do + # expect(autocorrect_source('Timecop.return')).to(eq('unfreeze')) + # end + + # context 'inside a block' do + # it 'autocorrects to `unfreeze`' do + # expect(autocorrect_source('foo { Timecop.return }')) + # .to(eq('foo { unfreeze }')) + # end + # end + # end end context 'with a block' do it 'adds an offense' do - expect_offense(<<~RUBY) + expect_offense(<<-RUBY.strip_indent) Timecop.return { } ^^^^^^^^^^^^^^ Use `travel_back` instead of `Timecop.return` RUBY end - it 'autocorrects to `travel_back`' do - expect(autocorrect_source('Timecop.return { }')).to(eq('travel_back { }')) + it 'does not autocorrect' do + expect(autocorrect_source('Timecop.return { }')) + .to(eq('Timecop.return { }')) + end + + context 'inside a block' do + it 'does not autocorrect' do + expect(autocorrect_source('foo { Timecop.return { } }')) + .to(eq('foo { Timecop.return { } }')) + end end end end describe 'Timecop.travel' do it 'adds an offense' do - expect_offense(<<~RUBY) + expect_offense(<<-RUBY.strip_indent) Timecop.travel(123) { } ^^^^^^^^^^^^^^^^^^^ Use `travel` or `travel_to` instead of `Timecop.travel`. If you need time to keep flowing, simulate it by travelling again. RUBY @@ -105,7 +150,7 @@ describe 'Timecop.*' do it 'adds an offense' do - expect_offense(<<~RUBY) + expect_offense(<<-RUBY.strip_indent) Timecop.foo ^^^^^^^ Use `ActiveSupport::Testing::TimeHelpers` instead of `Timecop` RUBY @@ -114,7 +159,7 @@ describe 'Timecop' do it 'adds an offense' do - expect_offense(<<~RUBY) + expect_offense(<<-RUBY.strip_indent) Timecop.foo ^^^^^^^ Use `ActiveSupport::Testing::TimeHelpers` instead of `Timecop` RUBY @@ -123,7 +168,7 @@ describe '::Timecop' do it 'adds an offense' do - expect_offense(<<~RUBY) + expect_offense(<<-RUBY.strip_indent) ::Timecop.foo ^^^^^^^^^ Use `ActiveSupport::Testing::TimeHelpers` instead of `Timecop` RUBY