Rework Faker::Time::between (#1417)

* Add Faker::Time.between_dates, remove period from between

This is an API breaking change.

Before this change, issues such as #1280 existed due to some
confusion about the behavior of Faker::Time.between. In essence,
between could produce values outside of the range of (from..to)
if the range is smaller than the provided period.

After this change, between now behaves more like Faker::Date.between
and as such the period argument has been removed. For the old
behavior, a user should switch to calling between_dates, which has
the same method signature as the old between method. The :between
period is no longer valid.

Documentation still needs to be updated, and additional tests need
to be written.

* Remove inheritance of Faker::Date by Faker::Time

This is an API breaking change.

Before this change, Faker::Time re-used some behavior of Faker::Date
by first inheriting from Date, then using `super`. This relationship
was one of convenience, though, and created some odd behavior for
Time. For example, calling Faker::Time.birthday was possible, as
birthday is defined in Faker::Date, but it would unintuitively return
a Date object.

Now, Faker::Time explicitly calls out to Faker::Date methods
everywhere necessary, and the inheritance relationship has been
removed. This does remove Faker::Time::birthday, and
Faker::Time::between_except from the interface - but as these methods
both return Date objects and are not documented in Faker::Time docs,
they were never indicated for direct use anyway.

* Improve Faker::Time.between tests for v2 behavior

Adds a new test to prevent regression of #1280, and adds return type
checking for new method between_dates.

* Improve coverage of Faker::Time docs

* Update docs

* Add keyword args for Faker::Time

* Delete time.md

* Docs

doc/default/time.md

@@ -1,24 +1,35 @@
 # Faker::Time
 
 ```ruby
-# Random date between dates
+# Random Time between two times
 Faker::Time.between(from: DateTime.now - 1, to: DateTime.now) #=> "2014-09-18 12:30:59 -0700"
-
-# Random date between dates (within specified part of the day)
+ # Random Stringified time between two times, formatted to the specified I18n format
+# (Examples are from a Rails console with rails-i18n 5.1.1 defaults loaded)
+I18n.locale = 'en-US'
+Faker::Time.between(from: DateTime.now - 1, to: DateTime.now, format: :default) #=> "Tue, 16 Oct 2018 10:48:27 AM -05:00"
+Faker::Time.between(from: DateTime.now - 1, to: DateTime.now, format: :short) #=> "15 Oct 10:48 AM"
+Faker::Time.between(from: DateTime.now - 1, to: DateTime.now, format: :long) #=> "October 15, 2018 10:48 AM"
+ I18n.locale = 'ja'
+Faker::Time.between(from: DateTime.now - 1, to: DateTime.now, format: :default) #=> "2018/10/15 10:48:27"
+Faker::Time.between(from: DateTime.now - 1, to: DateTime.now, format: :short) #=> "18/10/15 10:48"
+Faker::Time.between(from: DateTime.now - 1, to: DateTime.now, format: :long) #=> "2018年10月16日(火) 10時48分27秒 -0500"
+ # Random Time between two dates, within specified part of the day
 # You can install the as-duration gem to facilitate time manipulation like 45.minutes + 2.hours
 # (not needed if you already have activesupport, which is included with Rails)
 require 'as-duration'
-Faker::Time.between(from: 2.days.ago, to: Date.today, period: :all) #=> "2014-09-19 07:03:30 -0700"
-Faker::Time.between(from: 2.days.ago, to: Date.today, period: :day) #=> "2014-09-18 16:28:13 -0700"
-Faker::Time.between(from: 2.days.ago, to: Date.today, period: :night) #=> "2014-09-20 19:39:38 -0700"
-Faker::Time.between(from: 2.days.ago, to: Date.today, period: :morning) #=> "2014-09-19 08:07:52 -0700"
-Faker::Time.between(from: 2.days.ago, to: Date.today, period: :afternoon) #=> "2014-09-18 12:10:34 -0700"
-Faker::Time.between(from: 2.days.ago, to: Date.today, period: :evening) #=> "2014-09-19 20:21:03 -0700"
-Faker::Time.between(from: 2.days.ago, to: Date.today, period: :midnight) #=> "2014-09-20 00:40:14 -0700"
-
-# Random time in the future (up to maximum of N days)
+ Faker::Time.between_dates(from: 2.days.ago, to: Date.today, period: :all) #=> "2014-09-19 07:03:30 -0700"
+Faker::Time.between_dates(from: 2.days.ago, to: Date.today, period: :day) #=> "2014-09-18 16:28:13 -0700"
+Faker::Time.between_dates(from: 2.days.ago, to: Date.today, period: :night) #=> "2014-09-20 19:39:38 -0700"
+Faker::Time.between_dates(from: 2.days.ago, to: Date.today, period: :morning) #=> "2014-09-19 08:07:52 -0700"
+Faker::Time.between_dates(from: 2.days.ago, to: Date.today, period: :afternoon) #=> "2014-09-18 12:10:34 -0700"
+Faker::Time.between_dates(from: 2.days.ago, to: Date.today, period: :evening) #=> "2014-09-19 20:21:03 -0700"
+Faker::Time.between_dates(from: 2.days.ago, to: Date.today, period: :midnight) #=> "2014-09-20 00:40:14 -0700"
+ # Random Time in the future (up to maximum of N days)
 Faker::Time.forward(days: 23, period: :morning) # => "2014-09-26 06:54:47 -0700"
-
-# Random time in the past (up to maximum of N days)
+ # Random Time in the past (up to maximum of N days)
 Faker::Time.backward(days: 14, period: :evening) #=> "2014-09-17 19:56:33 -0700"
+ # Random Stringified time in the past, future, or between dates, formatted to the specified I18n format
+Faker::Time.backward(days: 5, period: :morning, format: :short) #=> "14 Oct 07:44"
+Faker::Time.forward(days: 5, period: :evening, format: :long) #=> "October 21, 2018 20:47"
+Faker::Time.between_dates(from: 5.days.ago, to: 5.days.from_now, period: :afternoon, format: :default) #=> "Fri, 19 Oct 2018 15:17:46 -0500"
 ```

lib/faker/default/time.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 module Faker
-  class Time < Faker::Date
+  class Time < Base
     TIME_RANGES = {
       all: (0..23),
       day: (9..17),
@@ -13,17 +13,26 @@ class Time < Faker::Date
     }.freeze
 
     class << self
-      def between(from:, to:, period: :all, format: nil)
-        time = period == :between ? rand(from..to) : date_with_random_time(super(from: from, to: to), period)
+      def between(from:, to:, format: nil)
+        from = get_time_object(from)
+        to = get_time_object(to)
+
+        time = Faker::Base.rand_in_range(from, to)
+        time_with_format(time, format)
+      end
+
+      def between_dates(from:, to:, period: :all, format: nil)
+        date = Faker::Date.between(from: from, to: to)
+        time = date_with_random_time(date, period)
         time_with_format(time, format)
       end
 
       def forward(days: 365, period: :all, format: nil)
-        time_with_format(date_with_random_time(super(days: days), period), format)
+        time_with_format(date_with_random_time(Faker::Date.forward(days: days), period), format)
       end
 
       def backward(days: 365, period: :all, format: nil)
-        time_with_format(date_with_random_time(super(days: days), period), format)
+        time_with_format(date_with_random_time(Faker::Date.backward(days: days), period), format)
       end
 
       private
@@ -33,7 +42,7 @@ def date_with_random_time(date, period)
       end
 
       def time_with_format(time, format)
-        format.nil? ? time : I18n.l(DateTime.parse(time.to_s), format: format)
+        format.nil? ? time : I18n.l(time, format: format)
       end
 
       def hours(period)
@@ -49,6 +58,12 @@ def minutes
       def seconds
         sample((0..59).to_a)
       end
+
+      def get_time_object(time)
+        time = ::Time.parse(time) if time.is_a? String
+        time = time.to_time if time.respond_to?(:to_time)
+        time
+      end
     end
   end
 end

test/faker/default/test_faker_time.rb

@@ -55,20 +55,22 @@ def test_invalid_period_error
     to   = Date.today + 15
 
     assert_raise ArgumentError do
-      @tester.between(from, to, :invalid_period)
+      @tester.between_dates(from, to, :invalid_period)
     end
   end
 
-  def test_return_type
+  def test_return_types_are_time_objects
     random_backward = @tester.backward(days: 5)
-    random_between_dates = @tester.between(from: Date.today, to: Date.today + 5)
-    random_between_times = @tester.between(from: Time.now, to: Time.now + TEN_HOURS)
+    random_between_dates = @tester.between_dates(from: Date.today, to: Date.today + 5)
+    random_between_with_date_args = @tester.between(from: Date.today, to: Date.today + 5)
+    random_between_with_time_args = @tester.between(from: Time.now, to: Time.now + TEN_HOURS)
     random_forward = @tester.forward(days: 5)
 
     [
       random_backward,
       random_between_dates,
-      random_between_times,
+      random_between_with_date_args,
+      random_between_with_time_args,
       random_forward
     ].each do |result|
       assert result.is_a?(Time), "Expected a Time object, but got #{result.class}"
@@ -82,10 +84,12 @@ def test_format
     100.times do
       period = @time_ranges.keys.to_a.sample
 
+      random_between_dates = @tester.between_dates(from: from, to: to, period: period, format: format)
       random_backward = @tester.backward(days: 30, period: period, format: format)
-      random_between  = @tester.between(from: from, to: to, period: period, format: format)
+      random_between  = @tester.between(from: from, to: to, format: format)
       random_forward  = @tester.forward(days: 30, period: period, format: format)
-      [random_backward, random_between, random_forward].each do |result|
+
+      [random_backward, random_between, random_between_dates, random_forward].each do |result|
         assert result.is_a?(String), "Expected a String, but got #{result.class}"
         assert_nothing_raised 'Not a valid date string' do
           date_format = '%m/%d/%Y %I:%M %p'
@@ -105,20 +109,24 @@ def test_time_period
       period_range    = @time_ranges[period]
 
       random_backward = @tester.backward(days: 30, period: period)
-      random_between  = @tester.between(from: from, to: to, period: period)
+      random_between  = @tester.between_dates(from: from, to: to, period: period)
       random_forward  = @tester.forward(days: 30, period: period)
 
       [random_backward, random_between, random_forward].each_with_index do |result, index|
         assert period_range.include?(result.hour.to_i), "#{%i[random_backward random_between random_forward][index]}: \"#{result}\" expected to be included in Faker::Time::TIME_RANGES[:#{period}] range"
       end
     end
+  end
 
-    from = Time.now
-    to   = Time.now + 100
+  def test_between_in_short_window
+    # This test intentionally specifies a small window, because previous versions of between's
+    # default behavior would only constrain the date range, while allowing the time range to
+    # wander.
+    from = Time.utc(2018, 'jun', 12, 16, 14, 44)
+    to   = Time.utc(2018, 'jun', 12, 16, 19, 52)
 
     100.times do
-      period          = :between
-      random_between  = @tester.between(from: from, to: to, period: period)
+      random_between = @tester.between(from: from, to: to)
       assert random_between >= from, "Expected >= \"#{from}\", but got #{random_between}"
       assert random_between <= to, "Expected <= \"#{to}\", but got #{random_between}"
     end