From c14c39308be5d9fa9e8a6d87ad8229819edd8cef Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Sat, 16 Feb 2019 16:16:48 -0500 Subject: [PATCH] Add Timecop cop 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` or `unfreeze_time` (autocorrected) - `Timecop.scale` & `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 --- CHANGELOG.md | 1 + changelog/new_add_timecop_cop.md | 1 + config/default.yml | 5 + legacy-docs/cops_rails.md | 87 +++++++++++ lib/rubocop/cop/rails/timecop.rb | 198 +++++++++++++++++++++++++ lib/rubocop/cop/rails_cops.rb | 1 + spec/rubocop/cop/rails/timecop_spec.rb | 180 ++++++++++++++++++++++ 7 files changed, 473 insertions(+) create mode 100644 changelog/new_add_timecop_cop.md create mode 100644 lib/rubocop/cop/rails/timecop.rb create mode 100644 spec/rubocop/cop/rails/timecop_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index d5dc1687aa..123f8ee747 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -784,3 +784,4 @@ [@bensheldon]: https://github.com/bensheldon [@gurix]: https://github.com/gurix [@etiennebarrie]: https://github.com/etiennebarrie +[@sambostock]: https://github.com/sambostock diff --git a/changelog/new_add_timecop_cop.md b/changelog/new_add_timecop_cop.md new file mode 100644 index 0000000000..511fd54083 --- /dev/null +++ b/changelog/new_add_timecop_cop.md @@ -0,0 +1 @@ +* [#38](https://github.com/rubocop/rubocop-rails/pull/38): Add `Timecop` cop. ([@sambostock][]) diff --git a/config/default.yml b/config/default.yml index 017bf704c0..a172f693ff 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1023,6 +1023,11 @@ Rails/TimeZoneAssignment: - spec/**/*.rb - test/**/*.rb +Rails/Timecop: + Description: 'Prefer `ActiveSupport::Testing::TimeHelpers` over `Timecop`.' + Enabled: pending + VersionAdded: <> + Rails/ToFormattedS: Description: 'Checks for consistent uses of `to_fs` or `to_formatted_s`.' StyleGuide: 'https://rails.rubystyle.guide/#prefer-to-fs' diff --git a/legacy-docs/cops_rails.md b/legacy-docs/cops_rails.md index fa50a7c582..f691cf76ea 100644 --- a/legacy-docs/cops_rails.md +++ b/legacy-docs/cops_rails.md @@ -2571,6 +2571,93 @@ EnforcedStyle | `flexible` | `strict`, `flexible` * [https://rails.rubystyle.guide#time](https://rails.rubystyle.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`. + +## 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` (or +`rails_helper`): + +```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 | Safe | Supports autocorrection | VersionAdded | VersionChanged diff --git a/lib/rubocop/cop/rails/timecop.rb b/lib/rubocop/cop/rails/timecop.rb new file mode 100644 index 0000000000..96d9f597ab --- /dev/null +++ b/lib/rubocop/cop/rails/timecop.rb @@ -0,0 +1,198 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # 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.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.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.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. + # + # ## RSpec Caveats + # + # Note that if using RSpec, `TimeHelpers` are not included by default, + # and must be manually included by updating `rails_helper` accordingly: + # + # ```ruby + # RSpec.configure do |config| + # config.include ActiveSupport::Testing::TimeHelpers + # end + # ``` + # + # Moreover, because `TimeHelpers` relies on Minitest teardown hooks, + # `rails_helper` must be required (instead of `spec_helper`), or a + # similar adapter layer must be in effect. + # + # @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 } + # + # # bad + # Timecop.scale(factor) + # Timecop.scale(factor) { assert true } + # + # # good + # travel(duration) + # travel_to(time) + # travel(duration) { assert true } + # travel_to(time) { assert true } + class Timecop < Base + extend AutoCorrector + + FREEZE_MESSAGE = 'Use `%s` instead of `Timecop.freeze`' + FREEZE_WITH_ARGUMENTS_MESSAGE = 'Use `travel` or `travel_to` instead of `Timecop.freeze`' + RETURN_MESSAGE = 'Use `%s` instead of `Timecop.return`' + FLOW_ADDENDUM = 'If you need time to keep flowing, simulate it by travelling again.' + TRAVEL_MESSAGE = "Use `travel` or `travel_to` instead of `Timecop.travel`. #{FLOW_ADDENDUM}" + SCALE_MESSAGE = "Use `travel` or `travel_to` instead of `Timecop.scale`. #{FLOW_ADDENDUM}" + MSG = 'Use `ActiveSupport::Testing::TimeHelpers` instead of `Timecop`' + + def_node_matcher :timecop_const?, <<~PATTERN + (const {nil? cbase} :Timecop) + PATTERN + + def_node_matcher :timecop_send, <<~PATTERN + (send + #timecop_const? ${:freeze :return :scale :travel} + $... + ) + PATTERN + + def on_const(node) + return unless timecop_const?(node) + + timecop_send(node.parent) do |message, arguments| + return on_timecop_send(node.parent, message, arguments) + end + + add_offense(node) + end + + private + + def on_timecop_send(node, message, arguments) + case message + when :freeze then on_timecop_freeze(node, arguments) + when :return then on_timecop_return(node, arguments) + when :scale then on_timecop_scale(node, arguments) + when :travel then on_timecop_travel(node, arguments) + else add_offense(node) + end + end + + def on_timecop_freeze(node, arguments) + if arguments.empty? + add_offense(node, message: format(FREEZE_MESSAGE, replacement: preferred_freeze_replacement)) do |corrector| + autocorrect_freeze(corrector, node, arguments) + end + else + add_offense(node, message: FREEZE_WITH_ARGUMENTS_MESSAGE) + end + end + + def on_timecop_return(node, arguments) + add_offense(node, message: format(RETURN_MESSAGE, replacement: preferred_return_replacement)) do |corrector| + autocorrect_return(corrector, node, arguments) + end + end + + def on_timecop_scale(node, _arguments) + add_offense(node, message: SCALE_MESSAGE) + end + + def on_timecop_travel(node, _arguments) + add_offense(node, message: TRAVEL_MESSAGE) + end + + def autocorrect_freeze(corrector, node, arguments) + return unless arguments.empty? + + corrector.replace(receiver_and_message_range(node), preferred_freeze_replacement) + end + + def autocorrect_return(corrector, node, _arguments) + return if given_block?(node) + + corrector.replace(receiver_and_message_range(node), preferred_return_replacement) + end + + def given_block?(node) + node.send_type? && node.parent && node.parent.block_type? && node.parent.send_node == node + end + + def receiver_and_message_range(node) + node.location.expression.with(end_pos: node.location.selector.end_pos) + end + + def preferred_freeze_replacement + return 'travel_to(Time.now)' if target_rails_version < 5.2 + + 'freeze_time' + end + + def preferred_return_replacement + return 'travel_back' if target_rails_version < 6.0 + + 'unfreeze_time' + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index fa167e1811..273afbf55e 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -121,6 +121,7 @@ require_relative 'rails/to_s_with_argument' require_relative 'rails/top_level_hash_with_indifferent_access' require_relative 'rails/transaction_exit_statement' +require_relative 'rails/timecop' require_relative 'rails/uniq_before_pluck' require_relative 'rails/unique_validation_without_index' require_relative 'rails/unknown_env' diff --git a/spec/rubocop/cop/rails/timecop_spec.rb b/spec/rubocop/cop/rails/timecop_spec.rb new file mode 100644 index 0000000000..fb5a41a09f --- /dev/null +++ b/spec/rubocop/cop/rails/timecop_spec.rb @@ -0,0 +1,180 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::Timecop, :config do + shared_examples 'adds an offense to constant, and does not correct' do |usage:| + timecop_end_index = usage.index('Timecop') + 'Timecop'.length + carets = '^' * timecop_end_index + + it 'adds an offense, and does not correct' do + expect_offense(<<~RUBY) + #{usage} + #{carets} Use `ActiveSupport::Testing::TimeHelpers` instead of `Timecop` + RUBY + + expect_no_corrections + end + end + + describe 'Timecop' do + include_examples 'adds an offense to constant, and does not correct', usage: 'Timecop' + + describe '.*' do + include_examples 'adds an offense to constant, and does not correct', usage: 'Timecop.foo' + end + + shared_examples 'adds an offense to send, and does not correct' do |usage:, include_time_flow_addendum: false| + usage_without_arguments = usage.sub(/\(.*\)$/, '') + carets = '^' * usage.length + addendum = + include_time_flow_addendum ? '. If you need time to keep flowing, simulate it by travelling again.' : '' + + context 'given no block' do + it 'adds an offense, and does not correct' do + expect_offense(<<~RUBY) + #{usage} + #{carets} Use `travel` or `travel_to` instead of `#{usage_without_arguments}`#{addendum} + RUBY + + expect_no_corrections + end + end + + context 'given a block' do + it 'adds an offense, and does not correct' do + expect_offense(<<~RUBY) + #{usage} { assert true } + #{carets} Use `travel` or `travel_to` instead of `#{usage_without_arguments}`#{addendum} + RUBY + + expect_no_corrections + end + end + end + + describe '.freeze' do + context 'without arguments' do + shared_examples 'adds an offense and corrects to' do |replacement:| + context 'given no block' do + it "adds an offense, and corrects to `#{replacement}`" do + expect_offense(<<~RUBY) + Timecop.freeze + ^^^^^^^^^^^^^^ Use `#{replacement}` instead of `Timecop.freeze` + RUBY + + expect_correction(<<~RUBY) + #{replacement} + RUBY + end + end + + context 'given a block' do + it "adds an offense, and corrects to `#{replacement}`" do + expect_offense(<<~RUBY) + Timecop.freeze { assert true } + ^^^^^^^^^^^^^^ Use `#{replacement}` instead of `Timecop.freeze` + RUBY + + expect_correction(<<~RUBY) + #{replacement} { assert true } + RUBY + end + end + end + + context 'prior to Rails 5.2', :rails51 do + include_examples 'adds an offense and corrects to', replacement: 'travel_to(Time.now)' + end + + context 'since Rails 5.2', :rails52 do + include_examples 'adds an offense and corrects to', replacement: 'freeze_time' + end + end + + context 'with arguments' do + include_examples 'adds an offense to send, and does not correct', usage: 'Timecop.freeze(*time_args)' + end + end + + describe '.return' do + shared_examples 'prefers' do |replacement| + context 'given no block' do + it "adds an offense, and corrects to `#{replacement}`" do + expect_offense(<<~RUBY) + Timecop.return + ^^^^^^^^^^^^^^ Use `#{replacement}` instead of `Timecop.return` + RUBY + + expect_correction(<<~RUBY) + #{replacement} + RUBY + end + + context 'inside a block' do + it "adds an offense, and corrects to `#{replacement}`" do + expect_offense(<<~RUBY) + foo { Timecop.return } + ^^^^^^^^^^^^^^ Use `#{replacement}` instead of `Timecop.return` + RUBY + + expect_correction(<<~RUBY) + foo { #{replacement} } + RUBY + end + end + end + + context 'given a block' do + it 'adds an offense, and does not correct' do + expect_offense(<<~RUBY) + Timecop.return { assert true } + ^^^^^^^^^^^^^^ Use `#{replacement}` instead of `Timecop.return` + RUBY + + expect_no_corrections + end + + context 'inside a block' do + it 'adds an offense, and does not correct' do + expect_offense(<<~RUBY) + foo { Timecop.return { assert true } } + ^^^^^^^^^^^^^^ Use `#{replacement}` instead of `Timecop.return` + RUBY + + expect_no_corrections + end + end + end + end + + context 'prior to Rails < 6.0', :rails52 do + include_examples 'prefers', 'travel_back' + end + + context 'since Rails 6.0', :rails60 do + include_examples 'prefers', 'unfreeze_time' + end + end + + describe '.scale' do + include_examples 'adds an offense to send, and does not correct', usage: 'Timecop.scale(factor)', + include_time_flow_addendum: true + end + + describe '.travel' do + include_examples 'adds an offense to send, and does not correct', usage: 'Timecop.travel(*time_args)', + include_time_flow_addendum: true + end + end + + describe '::Timecop' do + include_examples 'adds an offense to constant, and does not correct', usage: '::Timecop' + end + + describe 'Foo::Timecop' do + it 'adds no offenses' do + expect_no_offenses(<<~RUBY) + Foo::Timecop + RUBY + end + end +end