Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support nested time travel helpers #24690

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions activesupport/CHANGELOG.md
@@ -1,3 +1,15 @@
* `travel/travel_to` travel time helpers, now support nested calls,
and restore properly to time state for respective blocks.

travel_to outer_expected_time do
travel_to inner_expected_time do
#will work with inner_expected_time as time values
end
#will work with outer_expected_time as time values
end

*Vipul A M*

* Add `ActiveSupport.to_time_preserves_timezone` config option to control
how `to_time` handles timezones. In Ruby 2.4+ the behavior will change
from converting to the local system timezone to preserving the timezone
Expand Down
46 changes: 34 additions & 12 deletions activesupport/lib/active_support/testing/time_helpers.rb
@@ -1,32 +1,48 @@
module ActiveSupport
module Testing
class SimpleStubs # :nodoc:
Stub = Struct.new(:object, :method_name, :original_method)
Stub = Struct.new(:object, :method_name, :original_method, :return_value)
Copy link
Contributor

@kaspth kaspth Apr 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we could also get more mileage out of pushing some behavior into the Stub class.

Here's a spike:

class Stub        
  attr_reader :return_values

  def initialize(object, method_name)
    @object, @method_name = object, method_name
    @new_name = "__simple_stub__#{method_name}"

    @return_values = []
  end

  def push(return_value)
    @return_values << return_value

    unless object.singleton_class.respond_to?(@new_name)
      stub = self
      object.singleton_class.send :alias_method, @new_name, @method_name
      object.define_singleton_method(method_name) { stub.return_values.last }
    end
  end

  def pop
    @return_values.pop

    if @return_values.empty?
      singleton_class = object.singleton_class
      singleton_class.send :undef_method, @method_name
      singleton_class.send :alias_method, @method_name, @new_name
      singleton_class.send :undef_method, @new_name
    end
  end
end

Which with (in SimpleStubs):

def [](object, method_name)
  @stubs[[ object.object_id, method_name ]] ||= Stub.new(object, method_name)
end

Would be used like:

simple_stubs[Date, :today].push now.to_date

I think capturing the return values in an array should let us avoid having to keep redefining the methods every stack push 😁


def initialize
@stubs = {}
@stubs = Hash.new { |h,k| h[k] = [] }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would flipping the relationship clarify the intent? Found having stubs be per method and then the stub levels on each method hard to grasp and read.

end

def stub_object(object, method_name, return_value)
key = [object.object_id, method_name]

if stub = @stubs[key]
unstub_object(stub)
if @stubs[key].any?
stub = @stubs[key].last
unstub_object(stub) if stub
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could simplify to:

if stub = @stubs[key].last
  unstub_object(stub) if stub
end

end

new_name = "__simple_stub__#{method_name}"

@stubs[key] = Stub.new(object, method_name, new_name)
@stubs[key].push(Stub.new(object, method_name, new_name, return_value))

object.singleton_class.send :alias_method, new_name, method_name
object.define_singleton_method(method_name) { return_value }
end

def unstub_all!
@stubs.each_value do |stub|
unstub_object(stub)
@stubs.each_value do |stubs|
stub = stubs.first
unstub_object(stub) if stub
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be a good case for splat:

@stubs.each_value do |(stub_masking_original_methods, *)|

@stubs.clear
end

def unstub_to_previous_stub_state(object, method_name, return_value)
key = [object.object_id, method_name]

if @stubs[key].any?
current_stub = @stubs[key].pop

if stub = @stubs[key].last
stub_object(stub.object, stub.method_name, stub.return_value)
else
unstub_object(current_stub)
end
Copy link
Contributor

@kaspth kaspth Apr 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could do with a better method name 😁

Since the stubs per method is essentially turned into a stack we could go with pop_stub.

Here's my take:

def pop_stub(object, method_name)
  key = [ object.object_id, method_name ]

  currently_stubbed_to = @stubs[key].pop

  if stub_next_in_stack = @stubs[key].last
    stub_object(object, method_name, stub_next_in_stack.return_value)
  else
    unstub_object(currently_stubbed_to)
  end
end

Renamed some variables because I found them hard to keep track of.

end
@stubs = {}
end

private
Expand Down Expand Up @@ -98,16 +114,16 @@ def travel_to(date_or_time)
else
now = date_or_time.to_time.change(usec: 0)
end
to_date_value = now.to_date
to_datetime_value = now.to_datetime

simple_stubs.stub_object(Time, :now, now)
simple_stubs.stub_object(Date, :today, now.to_date)
simple_stubs.stub_object(DateTime, :now, now.to_datetime)
stub_unstub_date_time_objects(:stub_object, now, to_date_value, to_datetime_value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With pop_stub we could keep the original code here, but rename stub_object to push_stub.


if block_given?
begin
yield
ensure
travel_back
stub_unstub_date_time_objects(:unstub_to_previous_stub_state, now, to_date_value, to_datetime_value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With pop_stub this could be:

simple_stubs.pop_stub(Time, :now)
simple_stubs.pop_stub(Date, :today)
simple_stubs.pop_stub(DateTime, :now)

And we could kill stub_unstub_date_time_objects and the date and datetime value vars (since stubs remember their return_value we can pull it back it out from them).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should lose the travel_back call. So this code should be inside travel_back.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diabolo travel_back is travelling all the way back. Maybe we can rename it.

end
end
end
Expand All @@ -129,6 +145,12 @@ def travel_back
def simple_stubs
@simple_stubs ||= SimpleStubs.new
end

def stub_unstub_date_time_objects(method, now, to_date_value, to_datetime_value)
simple_stubs.public_send(method, Time, :now, now)
simple_stubs.public_send(method, Date, :today, to_date_value)
simple_stubs.public_send(method, DateTime, :now, to_datetime_value)
end
end
end
end
25 changes: 25 additions & 0 deletions activesupport/test/time_travel_test.rb
Expand Up @@ -45,6 +45,31 @@ def test_time_helper_travel_to
end
end

def test_time_helper_travel_to_with_nested_calls
Time.stub(:now, Time.now) do
outer_expected_time = Time.new(2004, 11, 24, 01, 04, 44)
inner_expected_time = Time.new(2004, 10, 24, 01, 04, 44)
travel_to outer_expected_time do
travel_to inner_expected_time do
assert_equal inner_expected_time, Time.now
end
assert_equal outer_expected_time, Time.now
end
end
end

def test_time_helper_travel_to_with_nested_calls_and_travel_back
current_time = Time.now
Time.stub(:now, Time.now) do
expected_time = Time.new(2004, 11, 24, 01, 04, 44)
travel_to expected_time do
assert_equal expected_time, Time.now
travel_back
end
assert_equal current_time.to_i, Time.now.to_i
end
end

def test_time_helper_travel_to_with_block
Time.stub(:now, Time.now) do
expected_time = Time.new(2004, 11, 24, 01, 04, 44)
Expand Down