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

Timecop.freeze with Dates is lossy and users should be warned #100

Closed
yaauie opened this issue Jul 18, 2013 · 18 comments
Closed

Timecop.freeze with Dates is lossy and users should be warned #100

yaauie opened this issue Jul 18, 2013 · 18 comments

Comments

@yaauie
Copy link
Contributor

yaauie commented Jul 18, 2013

Realistically, ActiveSupport's Time.zone being set and Timecop.freeze-ing with Date objects are not compatible. Date is a lossy representation of Time in Ruby anyway since it doesn't store offset, and the assumptions we make to handle that are surprising.

I think that freezing to Date objects should be deprecated.

Consider the following:

require 'active_support/all'

# The following timezones were chosen because they will always
# exemplify this bug; other timezone combinations may only show
# it during certain parts of the day.
ENV['TZ'] = 'Pacific/Kiritimati' # UTC+14:00
Time.zone = 'Midway Island'      # UTC-11:00

Timecop.freeze do # prevent drift
  puts Date.today #=> 2013-07-19
  # Date.today is generated using ENV['TZ'] or the computer's clock, 
  # independent of Time.zone.

  Timecop.freeze(Date.today) do
    # But in freezing, we used Time.zone and *assumed* that the date given was in
    # the timezone set for ActiveSupport, then froze time to that day's beginning
    # timestamp.
    # Here we encounter drift.
    puts Date.today #=> 2013-07-20

    Timecop.freeze(Date.today) do
      # And we can drift again.
      puts Date.today #=> 2013-07-21
    end
  end
end
@ClayShentrup
Copy link

You could have demonstrated this bug more concisely as:

Timecop.freeze('2013-04-24'.to_date) { Date.today }
Tue, 23 Apr 2013

Date is a lossy representation of Time in Ruby anyway since it doesn't store offset

This comment makes no sense to me. Offset is irrelevant for the purposes of a date.

@yaauie
Copy link
Contributor Author

yaauie commented Jul 29, 2013

@BrokenLadder because of the interplay of ENV['TZ'] and Time.zone, more setup was required to exemplify the bug:

> Time.now.utc_offset # ENV['TZ'] is nil, but computer's clock is UTC.
# => 0
> Time.zone = -2 # manually set Time.zone to negative offset. 2 bypasses AS's DST issues
# => -2
> Timecop.freeze('2013-04-24'.to_date) { Date.today }
# => Wed, 24 Apr 2013
> Time.zone = 2 # manually set Time.zone to positive offset. 2 bypasses AS's DST issues
# => 2
> Timecop.freeze('2013-04-24'.to_date) { Date.today }
# => Tue, 23 Apr 2013

My point though, was not just that there is a bug, but that the assumptions inherent in making the conversion from Date to a Time or an ActiveSupport::TimeWithZone object make supporting Date as an input faulty at best.

Date is a lossy representation of Time in Ruby anyway since it doesn't store offset

This comment makes no sense to me. Offset is irrelevant for the purposes of a date.

A date is defined as the period between a midnight and the following midnight, so moment-in-time data (such as the timestamp at the beginning of that day) cannot be extracted without additional information or assumptions about the UTC-offset; it is, therefore a lossy representation of time. Timecop is built to freeze the time, not the date, and using Date as an input forces Timecop to make assumptions about the UTC-offset, in this case using the one provided by Time.zone (where Date.today does not).

@ClayShentrup
Copy link

I would certainly agree that you can't convert a Date to a Time, at least without supplying a time zone to whatever function does the conversion.

Your argument that a Date is "lossy" could applied to any time. E.g. a time that is accurate down to the millesecond can only approximate a time in nanoseconds.

@yaauie
Copy link
Contributor Author

yaauie commented Jul 29, 2013

@BrokenLadder this is not an issue of precision; if it were, a round-trip from least-precise representation (Date) to more-precise representation (Time) and back again would always yield the same result.

In our case above, the assumptions inherent in that first conversion (which are simple without Time.zone, since ENV['TZ'] is the sole keeper-of-the-offset) mean that it should not be supported as an input to Timecop.freeze. The presence of activesupport's Time.zone muddies the waters, since there are now multiple keepers-of-the-offset that are and are not respected in various places. Currently Timecop assumes when we give it a Date that we're talking in the current Time.zone, but Date.today is ignorant of the presence of Time.zone altogether.

@ClayShentrup
Copy link

this is not an issue of precision

Your previous comment was about precision:

A date is defined as the period between a midnight and the following midnight, so moment-in-time data (such as the timestamp at the beginning of that day) cannot be extracted without additional information or assumptions about the UTC-offset; it is, therefore a lossy representation of time.

A UTC offset in this example is effectively a "less significant digit". Trying to get the time that starts a Date when you don't know the time zone is exactly like trying to get the minute of the current hour when you only have value of the hour hand. Mathematically speaking, your comment is absolutely about precision.

@yaauie
Copy link
Contributor Author

yaauie commented Jul 29, 2013

@BrokenLadder an offset is a shift applied to a "less significant digit" (the hours, or more correctly the minutes), which has an effect different than that of just a less significant digit in that it can "overflow" into the part of the value we care about.

But let's stop arguing over that, since we seem to agree on the main point; I'll bring it back to what you said earlier:

I would certainly agree that you can't convert a Date to a Time, at least without supplying a time zone to whatever function does the conversion.

This is the point, this is the whole reason why this feature should be deprecated. The function that does the conversion in Timecop has to make assumptions to do this conversion, which is what leads to surprising behaviour.

@ClayShentrup
Copy link

But how does this require any more assumptions than, say, '2013-04-20'?

@yaauie
Copy link
Contributor Author

yaauie commented Jul 29, 2013

@BrokenLadder that assumption is just in another place:

Time.parse knows nothing about your Time.zone, so unless your string contains offset-date it assumes you want it parsed in the context of ENV['TZ'] or your system clock; because Date.today also makes this same assumption (and does not provide the opportunity to provide offset data), the two will always agree with each other (but of course, Time.zone.now.to_date will not necessarily agree).

@ClayShentrup
Copy link

the two will always agree with each other

Timecop.freeze('2013-04-24'.to_date) { Date.today }
=> Tue, 23 Apr 2013
Timecop.freeze('2013-04-24') { Date.today }
=> Wed, 24 Apr 2013

@yaauie
Copy link
Contributor Author

yaauie commented Jul 29, 2013

the two will always agree with each other

The first example you gave does not go through Time.parse, since it is supplying a Date object to Timecop.freeze, and is not representative of the context in which it was quoted. ActiveSupport's String#to_date makes no assumptions about timezone because Date objects don't need this, but it forces Timecop to make an assumption when converting the provided Date object to a Time.

Timecop.freeze('2013-04-24'.to_date) { Date.today }
#=> Tue, 23 Apr 2013

In the second example (which does go through Time.parse), the same assumptions are being made about utc-offset by both Time.parse and Date.today, the two will always agree, regardless of timezone settings in Time.zone or ENV['TZ'].

Timecop.freeze('2013-04-24') { Date.today }
=> Wed, 24 Apr 2013

@code-R
Copy link

code-R commented Aug 6, 2013

@yaauie any updates on this issue?
Can we assume that using date with Timecop.freeze is going to be deprecated?

@yaauie
Copy link
Contributor Author

yaauie commented Aug 6, 2013

I'm not a maintainer on this project, and @travisjeffery hasn't chimed in.

At this point you can assume that it is a Bad Idea and should freeze to something that cleanly and unambiguously resolves to a Time, knowing also that the interplay between Date.today and ActiveSupport's Time.zone is messy at best.

@tehprofessor
Copy link

This is a pretty big caveat that definitely bit me in the arse today, +1 to getting this fixed.

@justin808
Copy link

I just ran into this issue and it cost me a bundle of time. Definitely worth fixing. I worked around the issue for myself by simply calling to_time on the date argument.

@kucaahbe
Copy link

👍 to_time fixes the issue, but investigation takes some time :/

@baxang
Copy link

baxang commented Jul 3, 2015

A workaround: Date.current seems work well.

@ChrisCPO
Copy link

Related issue? =>

AdminGlobalSetting.interviews_date #=> 2016-06-12 db_column is just date
date = AdminGlobalSetting.interviews_date - 1.day 
puts date #=> 2016-06-11
  Timecop.freeze(date) do
        puts Date.today #=> 2016-06-10
        DateChecker.new.perform
      end

I'm passing in the 11th and its freezing the 10th?

Update
after reading above

date #=> 2016-06-11
Timecop.freeze(date) do
        Date.today #=> 2016-06-10
        Date.current #=> 2016-06-11
        DateChecker.new.perform
      end

alexcameron89 pushed a commit to alexcameron89/active_shipping that referenced this issue Nov 14, 2016
When running the tests locally, I had two tests fail due to an issue
with Timecop. A similar issue has been opened on Timecop's repo, but it
has not been addressed:
travisjeffery/timecop#100

It seems that using a string in Timecop.freeze fixes this issue. This
pull request is opened for conversation and direction on the best way to
resolve the issue.
@StevenXL
Copy link

@yaauie Great catch. This issue saved me some time.

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

No branches or pull requests

10 participants