From 9340faf56676a57d95b90273c9c2f0b1597c2f3f Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Sat, 16 Feb 2019 16:16:48 -0500 Subject: [PATCH 1/6] 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 --- lib/rubocop/cop/rails/timecop.rb | 198 +++++++++++++++++++++++++ spec/rubocop/cop/rails/timecop_spec.rb | 178 ++++++++++++++++++++++ 2 files changed, 376 insertions(+) create mode 100644 lib/rubocop/cop/rails/timecop.rb create mode 100644 spec/rubocop/cop/rails/timecop_spec.rb diff --git a/lib/rubocop/cop/rails/timecop.rb b/lib/rubocop/cop/rails/timecop.rb new file mode 100644 index 00000000..539d4843 --- /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.source_range.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/spec/rubocop/cop/rails/timecop_spec.rb b/spec/rubocop/cop/rails/timecop_spec.rb new file mode 100644 index 00000000..f2069481 --- /dev/null +++ b/spec/rubocop/cop/rails/timecop_spec.rb @@ -0,0 +1,178 @@ +# 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:| + constant = usage.include?('::Timecop') ? '::Timecop' : 'Timecop' + + it 'adds an offense, and does not correct' do + expect_offense(<<~RUBY, constant: constant) + #{usage} + ^{constant} 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(/\(.*\)$/, '') + 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: usage) + #{usage} + ^{usage} 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: usage) + #{usage} { assert true } + ^{usage} 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 From 12fa433ec570aee3fc53bff7606fce7138815037 Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Wed, 7 Aug 2024 20:49:44 +0300 Subject: [PATCH 2/6] fixup! Small adjustments to make the move to RSpecRails --- CHANGELOG.md | 3 +++ config/default.yml | 7 +++++++ lib/rubocop/cop/{rails => rspec_rails}/timecop.rb | 7 +++---- lib/rubocop/cop/rspec_rails_cops.rb | 1 + spec/rubocop/cop/{rails => rspec_rails}/timecop_spec.rb | 2 +- 5 files changed, 15 insertions(+), 5 deletions(-) rename lib/rubocop/cop/{rails => rspec_rails}/timecop.rb (97%) rename spec/rubocop/cop/{rails => rspec_rails}/timecop_spec.rb (98%) diff --git a/CHANGELOG.md b/CHANGELOG.md index cad75145..ede296e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Master (Unreleased) +- Add a cop that makes `Timecop` illegal, in favour of `ActiveSupport::Testing::TimeHelpers`. ([@sambostock]) + ## 2.30.0 (2024-06-12) - Fix an runtime error for rubocop-rspec +3.0. ([@bquorning]) @@ -79,6 +81,7 @@ [@paydaylight]: https://github.com/paydaylight [@pirj]: https://github.com/pirj [@r7kamura]: https://github.com/r7kamura +[@sambostock]: https://github.com/sambostock [@splattael]: https://github.com/splattael [@tmaier]: https://github.com/tmaier [@ydah]: https://github.com/ydah diff --git a/config/default.yml b/config/default.yml index 51ec8889..63c218a3 100644 --- a/config/default.yml +++ b/config/default.yml @@ -77,6 +77,13 @@ RSpecRails/NegationBeValid: VersionChanged: '2.29' Reference: https://www.rubydoc.info/gems/rubocop-rspec_rails/RuboCop/Cop/RSpecRails/NegationBeValid +RSpecRails/Timecop: + Description: Enforces use of `ActiveSupport::Testing::TimeHelpers` instead of `Timecop`. + Enabled: pending + VersionAdded: "<>" + SafeAutoCorrect: false + Reference: https://www.rubydoc.info/gems/rubocop-rspec_rails/RuboCop/Cop/RSpecRails/Timecop + RSpecRails/TravelAround: Description: Prefer to travel in `before` rather than `around`. Enabled: pending diff --git a/lib/rubocop/cop/rails/timecop.rb b/lib/rubocop/cop/rspec_rails/timecop.rb similarity index 97% rename from lib/rubocop/cop/rails/timecop.rb rename to lib/rubocop/cop/rspec_rails/timecop.rb index 539d4843..efd3c002 100644 --- a/lib/rubocop/cop/rails/timecop.rb +++ b/lib/rubocop/cop/rspec_rails/timecop.rb @@ -2,9 +2,8 @@ module RuboCop module Cop - module Rails - # Disallows all usage of `Timecop`, in favour of - # `ActiveSupport::Testing::TimeHelpers`. + module RSpecRails + # Enforces use of `ActiveSupport::Testing::TimeHelpers` instead of `Timecop`. # # ## Migration # `Timecop.freeze` should be replaced with `freeze_time` when used @@ -93,7 +92,7 @@ module Rails # travel_to(time) # travel(duration) { assert true } # travel_to(time) { assert true } - class Timecop < Base + class Timecop < ::RuboCop::Cop::Base extend AutoCorrector FREEZE_MESSAGE = 'Use `%s` instead of `Timecop.freeze`' diff --git a/lib/rubocop/cop/rspec_rails_cops.rb b/lib/rubocop/cop/rspec_rails_cops.rb index 1a101b34..8edddaf6 100644 --- a/lib/rubocop/cop/rspec_rails_cops.rb +++ b/lib/rubocop/cop/rspec_rails_cops.rb @@ -6,4 +6,5 @@ require_relative 'rspec_rails/inferred_spec_type' require_relative 'rspec_rails/minitest_assertions' require_relative 'rspec_rails/negation_be_valid' +require_relative 'rspec_rails/timecop' require_relative 'rspec_rails/travel_around' diff --git a/spec/rubocop/cop/rails/timecop_spec.rb b/spec/rubocop/cop/rspec_rails/timecop_spec.rb similarity index 98% rename from spec/rubocop/cop/rails/timecop_spec.rb rename to spec/rubocop/cop/rspec_rails/timecop_spec.rb index f2069481..7dd56b88 100644 --- a/spec/rubocop/cop/rails/timecop_spec.rb +++ b/spec/rubocop/cop/rspec_rails/timecop_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -RSpec.describe RuboCop::Cop::Rails::Timecop, :config do +RSpec.describe RuboCop::Cop::RSpecRails::Timecop, :config do shared_examples 'adds an offense to constant, and does not correct' do |usage:| constant = usage.include?('::Timecop') ? '::Timecop' : 'Timecop' From 4fdc87c5d450b1c17e9f44b87d27fb3c6684b6b2 Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Wed, 7 Aug 2024 21:32:06 +0300 Subject: [PATCH 3/6] Appease RuboCop --- config/default.yml | 2 +- lib/rubocop/cop/rspec_rails/timecop.rb | 34 ++- spec/rubocop/cop/rspec_rails/timecop_spec.rb | 265 ++++++++++--------- 3 files changed, 171 insertions(+), 130 deletions(-) diff --git a/config/default.yml b/config/default.yml index 63c218a3..e880d107 100644 --- a/config/default.yml +++ b/config/default.yml @@ -78,7 +78,7 @@ RSpecRails/NegationBeValid: Reference: https://www.rubydoc.info/gems/rubocop-rspec_rails/RuboCop/Cop/RSpecRails/NegationBeValid RSpecRails/Timecop: - Description: Enforces use of `ActiveSupport::Testing::TimeHelpers` instead of `Timecop`. + Description: Enforces use of ActiveSupport TimeHelpers instead of Timecop. Enabled: pending VersionAdded: "<>" SafeAutoCorrect: false diff --git a/lib/rubocop/cop/rspec_rails/timecop.rb b/lib/rubocop/cop/rspec_rails/timecop.rb index efd3c002..c160e444 100644 --- a/lib/rubocop/cop/rspec_rails/timecop.rb +++ b/lib/rubocop/cop/rspec_rails/timecop.rb @@ -3,7 +3,7 @@ module RuboCop module Cop module RSpecRails - # Enforces use of `ActiveSupport::Testing::TimeHelpers` instead of `Timecop`. + # Enforces use of ActiveSupport TimeHelpers instead of Timecop. # # ## Migration # `Timecop.freeze` should be replaced with `freeze_time` when used @@ -96,17 +96,25 @@ class Timecop < ::RuboCop::Cop::Base extend AutoCorrector FREEZE_MESSAGE = 'Use `%s` instead of `Timecop.freeze`' - FREEZE_WITH_ARGUMENTS_MESSAGE = 'Use `travel` or `travel_to` 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}" + 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`' + # @!method timecop_const?(node) def_node_matcher :timecop_const?, <<~PATTERN (const {nil? cbase} :Timecop) PATTERN + # @!method timecop_send(node) def_node_matcher :timecop_send, <<~PATTERN (send #timecop_const? ${:freeze :return :scale :travel} @@ -138,7 +146,9 @@ def on_timecop_send(node, message, arguments) def on_timecop_freeze(node, arguments) if arguments.empty? - add_offense(node, message: format(FREEZE_MESSAGE, replacement: preferred_freeze_replacement)) do |corrector| + message = + format(FREEZE_MESSAGE, replacement: preferred_freeze_replacement) + add_offense(node, message: message) do |corrector| autocorrect_freeze(corrector, node, arguments) end else @@ -147,7 +157,9 @@ def on_timecop_freeze(node, arguments) end def on_timecop_return(node, arguments) - add_offense(node, message: format(RETURN_MESSAGE, replacement: preferred_return_replacement)) do |corrector| + message = + format(RETURN_MESSAGE, replacement: preferred_return_replacement) + add_offense(node, message: message) do |corrector| autocorrect_return(corrector, node, arguments) end end @@ -163,17 +175,19 @@ def on_timecop_travel(node, _arguments) def autocorrect_freeze(corrector, node, arguments) return unless arguments.empty? - corrector.replace(receiver_and_message_range(node), preferred_freeze_replacement) + 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) + 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 + node.parent&.block_type? && node.parent.send_node == node end def receiver_and_message_range(node) diff --git a/spec/rubocop/cop/rspec_rails/timecop_spec.rb b/spec/rubocop/cop/rspec_rails/timecop_spec.rb index 7dd56b88..028f7fd1 100644 --- a/spec/rubocop/cop/rspec_rails/timecop_spec.rb +++ b/spec/rubocop/cop/rspec_rails/timecop_spec.rb @@ -1,10 +1,28 @@ # frozen_string_literal: true RSpec.describe RuboCop::Cop::RSpecRails::Timecop, :config do - shared_examples 'adds an offense to constant, and does not correct' do |usage:| + shared_context 'with Rails 5.1', :rails51 do + let(:rails_version) { 5.1 } + end + + shared_context 'with Rails 5.2', :rails52 do + let(:rails_version) { 5.2 } + end + + shared_context 'with Rails 6.0', :rails60 do + let(:rails_version) { 6.0 } + end + + shared_context 'with Rails 7.0', :rails70 do + let(:rails_version) { 7.0 } + end + + include_context 'with Rails 7.0' + + shared_examples 'flags to constant, and does not correct' do |usage:| constant = usage.include?('::Timecop') ? '::Timecop' : 'Timecop' - it 'adds an offense, and does not correct' do + it 'flags, and does not correct' do expect_offense(<<~RUBY, constant: constant) #{usage} ^{constant} Use `ActiveSupport::Testing::TimeHelpers` instead of `Timecop` @@ -14,158 +32,167 @@ end end - describe 'Timecop' do - include_examples 'adds an offense to constant, and does not correct', usage: 'Timecop' + include_examples 'flags to constant, and does not correct', + usage: 'Timecop' - describe '.*' do - include_examples 'adds an offense to constant, and does not correct', usage: 'Timecop.foo' + describe '.*' do + include_examples 'flags to constant, and does not correct', + usage: 'Timecop.foo' + end + + shared_examples 'flags send, and does not correct' do |usage:, + flow_addendum: false| + usage_without_arguments = usage.sub(/\(.*\)$/, '') + addendum = + if flow_addendum + '. If you need time to keep flowing, simulate it by travelling again.' + else + '' + end + + context 'when given no block' do + it 'flags, and does not correct' do + expect_offense(<<~RUBY, usage: usage) + #{usage} + ^{usage} Use `travel` or `travel_to` instead of `#{usage_without_arguments}`#{addendum} + RUBY + + expect_no_corrections + end end - shared_examples 'adds an offense to send, and does not correct' do |usage:, include_time_flow_addendum: false| - usage_without_arguments = usage.sub(/\(.*\)$/, '') - addendum = - include_time_flow_addendum ? '. If you need time to keep flowing, simulate it by travelling again.' : '' + context 'when given a block' do + it 'flags, and does not correct' do + expect_offense(<<~RUBY, usage: usage) + #{usage} { assert true } + ^{usage} Use `travel` or `travel_to` instead of `#{usage_without_arguments}`#{addendum} + RUBY - context 'given no block' do - it 'adds an offense, and does not correct' do - expect_offense(<<~RUBY, usage: usage) - #{usage} - ^{usage} Use `travel` or `travel_to` instead of `#{usage_without_arguments}`#{addendum} + expect_no_corrections + end + end + end + + describe '.freeze' do + shared_examples 'flags and corrects to' do |replacement:| + context 'when given no block' do + it "flags, and corrects to `#{replacement}`" do + expect_offense(<<~RUBY) + Timecop.freeze + ^^^^^^^^^^^^^^ Use `#{replacement}` instead of `Timecop.freeze` RUBY - expect_no_corrections + expect_correction(<<~RUBY) + #{replacement} + RUBY end end - context 'given a block' do - it 'adds an offense, and does not correct' do - expect_offense(<<~RUBY, usage: usage) - #{usage} { assert true } - ^{usage} Use `travel` or `travel_to` instead of `#{usage_without_arguments}`#{addendum} + context 'when given a block' do + it "flags, and corrects to `#{replacement}`" do + expect_offense(<<~RUBY) + Timecop.freeze { assert true } + ^^^^^^^^^^^^^^ Use `#{replacement}` instead of `Timecop.freeze` RUBY - expect_no_corrections + expect_correction(<<~RUBY) + #{replacement} { assert true } + RUBY 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 'when Rails < 5.2', :rails51 do + include_examples 'flags and corrects to', + replacement: 'travel_to(Time.now)' + end - context 'prior to Rails 5.2', :rails51 do - include_examples 'adds an offense and corrects to', replacement: 'travel_to(Time.now)' - end + context 'with Rails 5.2+', :rails52 do + include_examples 'flags and corrects to', + replacement: 'freeze_time' + 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 'flags send, and does not correct', + usage: 'Timecop.freeze(*time_args)' + end + end - context 'with arguments' do - include_examples 'adds an offense to send, and does not correct', usage: 'Timecop.freeze(*time_args)' + shared_examples 'return prefers' do |replacement| + context 'when given no block' do + it "flags, and corrects to `#{replacement}`" do + expect_offense(<<~RUBY) + Timecop.return + ^^^^^^^^^^^^^^ Use `#{replacement}` instead of `Timecop.return` + RUBY + + expect_correction(<<~RUBY) + #{replacement} + RUBY 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 'when inside a block' do + it "flags, and corrects to `#{replacement}`" do + expect_offense(<<~RUBY) + foo { Timecop.return } + ^^^^^^^^^^^^^^ Use `#{replacement}` instead of `Timecop.return` + RUBY - 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 + expect_correction(<<~RUBY) + foo { #{replacement} } + RUBY end end + end + + context 'when given a block' do + it 'flags, and does not correct' do + expect_offense(<<~RUBY) + Timecop.return { assert true } + ^^^^^^^^^^^^^^ Use `#{replacement}` instead of `Timecop.return` + RUBY - context 'prior to Rails < 6.0', :rails52 do - include_examples 'prefers', 'travel_back' + expect_no_corrections end - context 'since Rails 6.0', :rails60 do - include_examples 'prefers', 'unfreeze_time' + context 'when inside a block' do + it 'flags, 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 - describe '.scale' do - include_examples 'adds an offense to send, and does not correct', usage: 'Timecop.scale(factor)', - include_time_flow_addendum: true + describe '.return' do + context 'when Rails < 6.0', :rails52 do + include_examples 'return prefers', 'travel_back' 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 + context 'with Rails 6.0+', :rails60 do + include_examples 'return prefers', 'unfreeze_time' end end + describe '.scale' do + include_examples 'flags send, and does not correct', + usage: 'Timecop.scale(factor)', + flow_addendum: true + end + + describe '.travel' do + include_examples 'flags send, and does not correct', + usage: 'Timecop.travel(*time_args)', + flow_addendum: true + end + describe '::Timecop' do - include_examples 'adds an offense to constant, and does not correct', usage: '::Timecop' + include_examples 'flags to constant, and does not correct', + usage: '::Timecop' end describe 'Foo::Timecop' do From 7b005c7d7eb6db331acc4f6d31c2e8ec78d82a87 Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Thu, 8 Aug 2024 01:59:08 +0300 Subject: [PATCH 4/6] Teach the cop about travel_back { ... } --- lib/rubocop/cop/rspec_rails/timecop.rb | 28 +++++---- spec/rubocop/cop/rspec_rails/timecop_spec.rb | 61 ++++++++------------ 2 files changed, 36 insertions(+), 53 deletions(-) diff --git a/lib/rubocop/cop/rspec_rails/timecop.rb b/lib/rubocop/cop/rspec_rails/timecop.rb index c160e444..2dcec539 100644 --- a/lib/rubocop/cop/rspec_rails/timecop.rb +++ b/lib/rubocop/cop/rspec_rails/timecop.rb @@ -16,10 +16,10 @@ module RSpecRails # 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. + # without a block. `travel_back` accepts a block starting with Rails 6.1. + # For earlier Rails, 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`, @@ -81,7 +81,7 @@ module RSpecRails # # # good # travel_back - # travel_to(time) { assert true } + # travel_back { assert true } # # # bad # Timecop.scale(factor) @@ -158,7 +158,7 @@ def on_timecop_freeze(node, arguments) def on_timecop_return(node, arguments) message = - format(RETURN_MESSAGE, replacement: preferred_return_replacement) + format(RETURN_MESSAGE, replacement: 'travel_back') add_offense(node, message: message) do |corrector| autocorrect_return(corrector, node, arguments) end @@ -180,16 +180,20 @@ def autocorrect_freeze(corrector, node, arguments) end def autocorrect_return(corrector, node, _arguments) - return if given_block?(node) + return if given_block?(node) && !supports_return_with_block? - corrector.replace(receiver_and_message_range(node), - preferred_return_replacement) + corrector.replace(receiver_and_message_range(node), 'travel_back') end def given_block?(node) node.parent&.block_type? && node.parent.send_node == node end + # travel_back { ... } was introduced in Rails 6.1 + def supports_return_with_block? + target_rails_version >= 6.1 + end + def receiver_and_message_range(node) node.source_range.with(end_pos: node.location.selector.end_pos) end @@ -199,12 +203,6 @@ def preferred_freeze_replacement 'freeze_time' end - - def preferred_return_replacement - return 'travel_back' if target_rails_version < 6.0 - - 'unfreeze_time' - end end end end diff --git a/spec/rubocop/cop/rspec_rails/timecop_spec.rb b/spec/rubocop/cop/rspec_rails/timecop_spec.rb index 028f7fd1..8c2329b5 100644 --- a/spec/rubocop/cop/rspec_rails/timecop_spec.rb +++ b/spec/rubocop/cop/rspec_rails/timecop_spec.rb @@ -118,63 +118,48 @@ end end - shared_examples 'return prefers' do |replacement| + shared_examples 'return prefers' do context 'when given no block' do - it "flags, and corrects to `#{replacement}`" do + it 'flags, and corrects to `travel_back`' do expect_offense(<<~RUBY) Timecop.return - ^^^^^^^^^^^^^^ Use `#{replacement}` instead of `Timecop.return` + ^^^^^^^^^^^^^^ Use `travel_back` instead of `Timecop.return` RUBY expect_correction(<<~RUBY) - #{replacement} + travel_back RUBY end - - context 'when inside a block' do - it "flags, 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 + end - context 'when given a block' do - it 'flags, and does not correct' do + describe '.return' do + context 'with Rails < 6.1', :rails60 do + include_examples 'return prefers' + + it 'flags, but does not correct return with a block' do expect_offense(<<~RUBY) Timecop.return { assert true } - ^^^^^^^^^^^^^^ Use `#{replacement}` instead of `Timecop.return` + ^^^^^^^^^^^^^^ Use `travel_back` instead of `Timecop.return` RUBY expect_no_corrections end - - context 'when inside a block' do - it 'flags, 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 - describe '.return' do - context 'when Rails < 6.0', :rails52 do - include_examples 'return prefers', 'travel_back' - end + context 'with Rails 6.1+', :rails61 do + include_examples 'return prefers' - context 'with Rails 6.0+', :rails60 do - include_examples 'return prefers', 'unfreeze_time' + it 'flags, and corrects return with a block' do + expect_offense(<<~RUBY) + Timecop.return { assert true } + ^^^^^^^^^^^^^^ Use `travel_back` instead of `Timecop.return` + RUBY + + expect_correction(<<~RUBY) + travel_back { assert true } + RUBY + end end end From 3ba93932be8bb8f525266c0b59b123def943704e Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Thu, 8 Aug 2024 01:59:34 +0300 Subject: [PATCH 5/6] Gen docs --- docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_rspecrails.adoc | 110 +++++++++++++++++++ 2 files changed, 111 insertions(+) diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index b4fc11c6..8fe8f210 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -8,6 +8,7 @@ * xref:cops_rspecrails.adoc#rspecrailsinferredspectype[RSpecRails/InferredSpecType] * xref:cops_rspecrails.adoc#rspecrailsminitestassertions[RSpecRails/MinitestAssertions] * xref:cops_rspecrails.adoc#rspecrailsnegationbevalid[RSpecRails/NegationBeValid] +* xref:cops_rspecrails.adoc#rspecrailstimecop[RSpecRails/Timecop] * xref:cops_rspecrails.adoc#rspecrailstravelaround[RSpecRails/TravelAround] // END_COP_LIST diff --git a/docs/modules/ROOT/pages/cops_rspecrails.adoc b/docs/modules/ROOT/pages/cops_rspecrails.adoc index 685e6b6c..ec48a13a 100644 --- a/docs/modules/ROOT/pages/cops_rspecrails.adoc +++ b/docs/modules/ROOT/pages/cops_rspecrails.adoc @@ -377,6 +377,116 @@ expect(foo).to be_invalid.or be_even * https://www.rubydoc.info/gems/rubocop-rspec_rails/RuboCop/Cop/RSpecRails/NegationBeValid +== RSpecRails/Timecop + +|=== +| Enabled by default | Safe | Supports autocorrection | Version Added | Version Changed + +| Pending +| Yes +| Always (Unsafe) +| <> +| - +|=== + +Enforces use of ActiveSupport TimeHelpers instead of Timecop. + +## 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` accepts a block starting with Rails 6.1. +For earlier Rails, 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. + +=== Examples + +[source,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_back { 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 } +---- + +=== References + +* https://www.rubydoc.info/gems/rubocop-rspec_rails/RuboCop/Cop/RSpecRails/Timecop + == RSpecRails/TravelAround |=== From 2b067feef4b562e91fb2cf3f3d3b031abca7022f Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Fri, 9 Aug 2024 11:58:56 +0300 Subject: [PATCH 6/6] Fix RSpec 4 build: trigger_inclusion changes See https://github.com/rubocop/rubocop/pull/10806 and https://github.com/rspec/rspec-core/pull/2878 for details --- spec/rubocop/cop/rspec_rails/timecop_spec.rb | 24 +++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/spec/rubocop/cop/rspec_rails/timecop_spec.rb b/spec/rubocop/cop/rspec_rails/timecop_spec.rb index 8c2329b5..5152fb6e 100644 --- a/spec/rubocop/cop/rspec_rails/timecop_spec.rb +++ b/spec/rubocop/cop/rspec_rails/timecop_spec.rb @@ -1,19 +1,23 @@ # frozen_string_literal: true RSpec.describe RuboCop::Cop::RSpecRails::Timecop, :config do - shared_context 'with Rails 5.1', :rails51 do + shared_context 'with Rails 5.1' do let(:rails_version) { 5.1 } end - shared_context 'with Rails 5.2', :rails52 do + shared_context 'with Rails 5.2' do let(:rails_version) { 5.2 } end - shared_context 'with Rails 6.0', :rails60 do + shared_context 'with Rails 6.0' do let(:rails_version) { 6.0 } end - shared_context 'with Rails 7.0', :rails70 do + shared_context 'with Rails 6.1' do + let(:rails_version) { 6.1 } + end + + shared_context 'with Rails 7.0' do let(:rails_version) { 7.0 } end @@ -102,12 +106,14 @@ end end - context 'when Rails < 5.2', :rails51 do + context 'when Rails < 5.2' do + include_context 'with Rails 5.1' include_examples 'flags and corrects to', replacement: 'travel_to(Time.now)' end - context 'with Rails 5.2+', :rails52 do + context 'with Rails 5.2+' do + include_context 'with Rails 5.2' include_examples 'flags and corrects to', replacement: 'freeze_time' end @@ -134,7 +140,8 @@ end describe '.return' do - context 'with Rails < 6.1', :rails60 do + context 'with Rails < 6.1' do + include_context 'with Rails 6.0' include_examples 'return prefers' it 'flags, but does not correct return with a block' do @@ -147,7 +154,8 @@ end end - context 'with Rails 6.1+', :rails61 do + context 'with Rails 6.1+' do + include_context 'with Rails 6.1' include_examples 'return prefers' it 'flags, and corrects return with a block' do