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

`travel/travel_to` travel time helpers, now raise on nested calls #24890

Merged
merged 1 commit into from Jul 5, 2016

Conversation

@vipulnsward
Copy link
Member

vipulnsward commented May 6, 2016

@kaspth
kaspth reviewed May 6, 2016
View changes
activesupport/CHANGELOG.md Outdated
# 2 days from today

travel_back
travel_to 5.days.from_now

This comment has been minimized.

Copy link
@kaspth

kaspth May 6, 2016

Member

Won't travel_back be what users want to call 90% of the times if they add a second travel_to in tests? Then shouldn't we just call it automatically for them? We could add an reset: false opt-out to travel and travel_to?

This comment has been minimized.

Copy link
@vipulnsward

vipulnsward May 6, 2016

Author Member

Thats something I needed to discuss with @rafaelfranca . We had not discussed this scenario.
But essentially, we would like to achieve orderly calls. travel_to 2.days.from_now, travel_to 5.days.from_now, so makes sense.

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca May 21, 2016

Member

I prefer to also use the block version here but without the nesting. Could you change it?

@kaspth
kaspth reviewed May 6, 2016
View changes
activesupport/lib/active_support/testing/time_helpers.rb Outdated
@@ -93,6 +98,11 @@ def travel(duration, &block)
# end
# Time.current # => Sat, 09 Nov 2013 15:34:49 EST -05:00
def travel_to(date_or_time)
if block_given? && simple_stubs.stubbed?(Time, :now)
raise "Calling `travel_to` with a block, when we have previously already made a call to `travel_to`, can lead to confusing time stubbing. " \
"Please make subsequent calls to `travel_to` without a block."

This comment has been minimized.

Copy link
@kaspth

kaspth May 6, 2016

Member

This text doesn't match the example shown in the changelog. There's no blocks in use there.

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca May 21, 2016

Member

We can go verbose and show the example. I'm 👍 in the block version.

# 2 days from today
travel_to 3.days.from_now do
# 5 days from today
end

This comment has been minimized.

Copy link
@vipulnsward

vipulnsward May 6, 2016

Author Member

@kaspth this is a block?

This comment has been minimized.

Copy link
@kaspth

kaspth May 6, 2016

Member

Ah, right. But this can be written with already today:

travel_to 2.days.from_now do
  # 2 days from today.
end

travel_to 5.days.from_now do
  # 5 days from today.
end
@kaspth
kaspth reviewed May 6, 2016
View changes
activesupport/lib/active_support/testing/time_helpers.rb Outdated
@@ -29,6 +29,11 @@ def unstub_all!
@stubs = {}
end

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

This comment has been minimized.

Copy link
@kaspth

kaspth May 6, 2016

Member

SimpleStubs is suddenly creating arrays in 3 places, would suggest a refactor to a Hash:

@stubs = Hash.new { |h,k| h[k] = {} }

# Which would be written to like:
@stubs[Time.object_id][:now] = Time.now

This would then make this method:

def stubbed?(object, method_name)
  @stubs[object.object_id][method_name]
end

This comment has been minimized.

Copy link
@rafaelfranca

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca May 21, 2016

Member

Use a Concurent::Map here.

travel_to 2.days.from_now do
# 2 days from today
travel_to 3.days.from_now do
# 5 days from today

This comment has been minimized.

Copy link
@kaspth

kaspth May 6, 2016

Member

You're hacking too much time! 3 > 5 😁

This comment has been minimized.

Copy link
@vipulnsward

vipulnsward May 21, 2016

Author Member

Hehe, we discussed and I showed some examples from our production Application actually that does this. It is a Task Management Application, and the test verifies notifications. This is in case of nested cases, where you would like to simulate nested way to achieve cases, and do calls to contexts from already executing operation.

@vipulnsward vipulnsward force-pushed the vipulnsward:travel-to-raise branch 2 times, most recently May 21, 2016
end

MSG
raise travel_to_nested_block_call

This comment has been minimized.

Copy link
@vipulnsward

vipulnsward May 21, 2016

Author Member

Something like this?

@rafaelfranca
rafaelfranca reviewed May 21, 2016
View changes
activesupport/lib/active_support/testing/time_helpers.rb Outdated
module ActiveSupport
module Testing
class SimpleStubs # :nodoc:
Stub = Struct.new(:object, :method_name, :original_method)

def initialize
@stubs = {}
@stubs = Concurrent::Map.new{ |h,k| h[k] = {} }

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca May 21, 2016

Member

space before the {

@rafaelfranca
rafaelfranca reviewed May 21, 2016
View changes
activesupport/lib/active_support/testing/time_helpers.rb Outdated
@@ -93,6 +99,34 @@ def travel(duration, &block)
# end
# Time.current # => Sat, 09 Nov 2013 15:34:49 EST -05:00
def travel_to(date_or_time)
if block_given? && simple_stubs.stubbed?(Time, :now)
travel_to_nested_block_call = <<-MSG

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca May 21, 2016

Member

strip_heredoc

This comment has been minimized.

Copy link
@kaspth

kaspth May 21, 2016

Member

Remember to require the core ext file too 😁

@kaspth
kaspth reviewed May 21, 2016
View changes
activesupport/test/time_travel_test.rb Outdated
travel_to initial_expected_time
travel_to subsequent_expected_time
travel_to initial_expected_time
travel_to subsequent_expected_time

This comment has been minimized.

Copy link
@kaspth

kaspth May 21, 2016

Member

Do we need to do the initial subsequent dance a second time? Could do with some newlines :)

@kaspth
kaspth reviewed May 21, 2016
View changes
activesupport/lib/active_support/testing/time_helpers.rb Outdated
# 2 days from today
end

travel_to 5.days.from_now do

This comment has been minimized.

Copy link
@kaspth

kaspth May 21, 2016

Member

Thinking about this some more it's weird to use travel_to with a relative time. Shouldn't we say travel 5.days?

Also actual travel calls would spout these exceptions mentioning travel_to — that's leaking the implementation to users.

This comment has been minimized.

Copy link
@vipulnsward

vipulnsward May 21, 2016

Author Member

travel_to 5.days.from_now

vs

travel 5.days

For me the first one is more descriptive. travel 5.days from where? 5.days amounts to what duration?, etc.
The first one reads out in a more natural way.

This comment has been minimized.

Copy link
@kaspth

kaspth May 30, 2016

Member

My point is that our API for relative times is travel and not travel_to.

The first one reads out in a more natural way.

Subjective. I personally find the second one more natural. But that's why we have both for the times when one reads better than the other 😁

@kaspth
kaspth reviewed May 21, 2016
View changes
activesupport/lib/active_support/testing/time_helpers.rb Outdated
if block_given? && simple_stubs.stubbed?(Time, :now)
travel_to_nested_block_call = <<-MSG

Calling `travel_to` with a block, when we have previously already made a call to `travel_to`, can lead to confusing time stubbing.

This comment has been minimized.

Copy link
@kaspth

kaspth May 21, 2016

Member

How about:

Calling `travel_to` within another `travel_to` block is unsupported. Move the inner call outside like this:

   travel 2.days do
     # Outer call
   end

   travel 5.days do
     # Inner call
   end

This comment has been minimized.

Copy link
@vipulnsward

vipulnsward May 21, 2016

Author Member

You mean travel_to ? Also this form assumes user has a block like travel 2.days and travel 5.days, which might be different from what user is using, whereas current one gives previous and current forms.

This comment has been minimized.

Copy link
@kaspth

kaspth May 30, 2016

Member

Yes true, but they are examples; users could also say 5.minutes or 60.hours.

I tried to clarify the previous state with:

Calling `travel_to` within another `travel_to` block is unsupported.

Doesn't that capture it good enough, while saving complexity?

I'm just trying to cut down on the exception length because I think it's overwhelming as is 😄

@kaspth
kaspth reviewed May 21, 2016
View changes
activesupport/lib/active_support/testing/time_helpers.rb Outdated
end
@stubs = {}
@stubs = Concurrent::Map.new{ |h,k| h[k] = {} }

This comment has been minimized.

Copy link
@kaspth

kaspth May 21, 2016

Member

@stubs.clear?

@kaspth
kaspth reviewed May 21, 2016
View changes
activesupport/lib/active_support/testing/time_helpers.rb Outdated
@@ -1,32 +1,38 @@
require 'concurrent'

This comment has been minimized.

Copy link
@kaspth

kaspth May 21, 2016

Member

Let's just require 'concurrent/map'?

@kaspth
kaspth reviewed May 21, 2016
View changes
activesupport/lib/active_support/testing/time_helpers.rb Outdated

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_map|

This comment has been minimized.

Copy link
@kaspth

kaspth May 21, 2016

Member

do |object_stubs|?

@kaspth
kaspth reviewed May 21, 2016
View changes
activesupport/lib/active_support/testing/time_helpers.rb Outdated
key = [object.object_id, method_name]

if stub = @stubs[key]
if stub = @stubs[object.object_id][method_name]

This comment has been minimized.

Copy link
@kaspth

kaspth May 21, 2016

Member

We could rename stubbed? to stubbing or something else that's less predicate-ish. That way we can reuse it here 😁

@vipulnsward vipulnsward force-pushed the vipulnsward:travel-to-raise branch May 21, 2016
@kaspth
Copy link
Member

kaspth commented Jun 26, 2016

@vipulnsward hey, you still interested in making this happen? 😁

@vipulnsward
Copy link
Member Author

vipulnsward commented Jun 26, 2016

Oh yes. Its still on my radar.
Let me get to it soon.

@vipulnsward vipulnsward force-pushed the vipulnsward:travel-to-raise branch 2 times, most recently Jul 2, 2016
@vipulnsward
Copy link
Member Author

vipulnsward commented Jul 2, 2016

Alright, ready for another round.
r? @kaspth

@rails-bot rails-bot assigned kaspth and unassigned rafaelfranca Jul 2, 2016
inner_expected_time = Time.new(2004, 10, 24, 01, 04, 44)
travel_to outer_expected_time do
assert_nothing_raised do
travel_to(inner_expected_time)

This comment has been minimized.

Copy link
@kaspth

kaspth Jul 2, 2016

Member

Does this correctly reset the time outside of block? In other words, the inner travel_to pushes to the stack but do we pop all the way?

Shouldn't we test that here?

This comment has been minimized.

Copy link
@kaspth

kaspth Jul 2, 2016

Member

On the other hand I also find it strange that we support without a block travel within a block. Wasn't the point of this to prevent inner block calls?

This comment has been minimized.

Copy link
@vipulnsward

vipulnsward Jul 2, 2016

Author Member

No, not unless you do an explicit travel_back, but that too will reset the complete state, we don't allow travel_back to previous state.

I think this is ok, since we explicity specify our preferred way is to not use multi-block or calls within a block.

@kaspth
kaspth reviewed Jul 2, 2016
View changes
activesupport/test/time_travel_test.rb Outdated
end
end

assert_equal "\nCalling `travel_to` with a block, when we have previously already made a call to `travel_to`, can lead to confusing time stubbing.\n\nInstead of:\n\n travel_to 2.days.from_now do\n # 2 days from today\n travel_to 3.days.from_now do\n # 5 days from today\n end\n end\n\npreferred way to achieve above is:\n\n travel 2.days do\n # 2 days from today\n end\n\n travel 5.days do\n # 5 days from today\n end\n\n", error.message

This comment has been minimized.

Copy link
@kaspth

kaspth Jul 2, 2016

Member

I think you can pass a Regex as the second argument to assert_raises which could simplify this check. Perhaps checking that part of the message is emitted is sufficient coverage rather than strictly asserting on the whole format 😁

@kaspth
Copy link
Member

kaspth commented Jul 2, 2016

How do we fare with putting travel in say setup and then using a travel with a block in a test method?

@kaspth
Copy link
Member

kaspth commented Jul 2, 2016

This also breaks Active Record's tests so there's probably a place where we use nested travels 😁

@vipulnsward
Copy link
Member Author

vipulnsward commented Jul 2, 2016

How do we fare with putting travel in say setup and then using a travel with a block in a test method?

That is exactly what broke the tests on Active Record. Preferred way is to not do that.

     as this can lead to confusing time stubbing.

     Instead of:

         travel_to 2.days.from_now do
           # 2 days from today
           travel_to 3.days.from_now do
             # 5 days from today
           end
         end

     preferred way to achieve above is:

         travel_to 2.days.from_now
         # 2 days from today

         travel_back
         travel_to 5.days.from_now
         # 5 days from today

Closes #24690
Fixes #24689
@vipulnsward vipulnsward force-pushed the vipulnsward:travel-to-raise branch to 919e705 Jul 2, 2016
@kaspth kaspth merged commit 2dfb06a into rails:master Jul 5, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth
Copy link
Member

kaspth commented Jul 5, 2016

Nice! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.