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..e880d107 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 TimeHelpers instead of Timecop. + Enabled: pending + VersionAdded: "<<next>>" + 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/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) +| <<next>> +| - +|=== + +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 |=== diff --git a/lib/rubocop/cop/rspec_rails/timecop.rb b/lib/rubocop/cop/rspec_rails/timecop.rb new file mode 100644 index 00000000..2dcec539 --- /dev/null +++ b/lib/rubocop/cop/rspec_rails/timecop.rb @@ -0,0 +1,209 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module RSpecRails + # 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. + # + # @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_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 } + class Timecop < ::RuboCop::Cop::Base + extend AutoCorrector + + FREEZE_MESSAGE = 'Use `%<replacement>s` instead of `Timecop.freeze`' + FREEZE_WITH_ARGUMENTS_MESSAGE = + 'Use `travel` or `travel_to` instead of `Timecop.freeze`' + RETURN_MESSAGE = 'Use `%<replacement>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`' + + # @!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} + $... + ) + 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? + message = + format(FREEZE_MESSAGE, replacement: preferred_freeze_replacement) + add_offense(node, message: message) 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) + message = + format(RETURN_MESSAGE, replacement: 'travel_back') + add_offense(node, message: message) 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) && !supports_return_with_block? + + 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 + + def preferred_freeze_replacement + return 'travel_to(Time.now)' if target_rails_version < 5.2 + + 'freeze_time' + end + end + end + end +end 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/rspec_rails/timecop_spec.rb b/spec/rubocop/cop/rspec_rails/timecop_spec.rb new file mode 100644 index 00000000..5152fb6e --- /dev/null +++ b/spec/rubocop/cop/rspec_rails/timecop_spec.rb @@ -0,0 +1,198 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::RSpecRails::Timecop, :config do + shared_context 'with Rails 5.1' do + let(:rails_version) { 5.1 } + end + + shared_context 'with Rails 5.2' do + let(:rails_version) { 5.2 } + end + + shared_context 'with Rails 6.0' do + let(:rails_version) { 6.0 } + end + + 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 + + include_context 'with Rails 7.0' + + shared_examples 'flags to constant, and does not correct' do |usage:| + constant = usage.include?('::Timecop') ? '::Timecop' : 'Timecop' + + it 'flags, 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 + + include_examples 'flags to constant, and does not correct', + usage: 'Timecop' + + 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 + + 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 + + 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_correction(<<~RUBY) + #{replacement} + RUBY + end + end + + 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_correction(<<~RUBY) + #{replacement} { assert true } + RUBY + end + end + end + + 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+' do + include_context 'with Rails 5.2' + include_examples 'flags and corrects to', + replacement: 'freeze_time' + end + + context 'with arguments' do + include_examples 'flags send, and does not correct', + usage: 'Timecop.freeze(*time_args)' + end + end + + shared_examples 'return prefers' do + context 'when given no block' do + it 'flags, and corrects to `travel_back`' do + expect_offense(<<~RUBY) + Timecop.return + ^^^^^^^^^^^^^^ Use `travel_back` instead of `Timecop.return` + RUBY + + expect_correction(<<~RUBY) + travel_back + RUBY + end + end + end + + describe '.return' 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 + expect_offense(<<~RUBY) + Timecop.return { assert true } + ^^^^^^^^^^^^^^ Use `travel_back` instead of `Timecop.return` + RUBY + + expect_no_corrections + end + end + + 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 + 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 + + 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 'flags 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