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

ActiveSupport Date.current differs from Date.today when Time.zone is set #416

Closed
wants to merge 8 commits into from

Conversation

jch
Copy link

@jch jch commented Mar 15, 2024

Edit: #416 (comment) for diagnosis

Original description:

I suspect

time = time_klass.at(@time.dup.localtime)
calling localtime to be causing this issue. My system localtime is -7, so this cast is changing the date to the previous day. Diff is a failing test.

…to UTC

I suspect https://github.com/travisjeffery/timecop/blob/e134761e942c612f8199f2d83793995653f810cb/lib/timecop/time_stack_item.rb#L59
calling localtime to be causing this issue. My system localtime is -7,
so this cast is changing the date to the previous day.
@joshuacronemeyer
Copy link
Collaborator

@jch this test does not fail for me. also checkout the each_timezone test helper we've got. I was careful to make sure it had a timezone where today is tomorrow for me and still cant get this test to fail.

Focus only on the failing test case with zone
Fixes failing test test_date_current_same_as_date_today_with_time_zone
@jch
Copy link
Author

jch commented Aug 14, 2024

@joshuacronemeyer thanks for taking a look. how were you running the test? I realized I didn't have the _test suffix on the filename so it wasn't running in rake test. Updated it, and running the specific test file fails for me with the latest master changes merged:

$  ruby test/active_support_date_current_test.rb
Run options: --seed 39850

# Running:

F.

Finished in 0.050811s, 39.3616 runs/s, 39.3616 assertions/s.

  1) Failure:
TestActiveSupportDateCurrent#test_date_current_same_as_date_today_with_time_zone [test/active_support_date_current_test.rb:24]:
--- expected
+++ actual
@@ -1 +1 @@
-#<Date: 2024-03-15 ((2460385j,0s,0n),+0s,2299161j)>
+#<Date: 2024-03-14 ((2460384j,0s,0n),+0s,2299161j)>


2 runs, 2 assertions, 1 failures, 0 errors, 0 skips

The test doesn't fail if I use each_timezone test helper. I suspect this is ENV["TZ"] is used by some railtie to set Time.zone explicitly but haven't confirmed. What is your system timezone set to?

I didn't see this previously, but the README hints at this old issue https://rails.lighthouseapp.com/projects/8994/tickets/6410-dateyesterday-datetoday and describes the unexpected behavior.

Setting breakpoints within the freeze block, Date.current uses the Time extensions, whereas Date.today uses the Date extensions:

        # @time: ActiveSupport::TimeWithZone: Fri, 15 Mar 2024 00:00:00.000000000 UTC +00:00
        # time: Time: 2024-03-14 17:00:00 -0700
        #0	Timecop::TimeStackItem#time(time_klass=Time) at ~/timecop/lib/timecop/time_stack_item.rb:79
        #1	Time.mock_time at ~/timecop/lib/timecop/time_extensions.rb:8
        #2	Time.now at ~/timecop/lib/timecop/time_extensions.rb:14
        #3	TZInfo::Timezone#now at ~/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/tzinfo-2.0.6/lib/tzinfo/timezone.rb:993 (to_local(Time.now))
        #4	ActiveSupport::TimeZone#today at ~/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/activesupport-7.1.3.4/lib/active_support/values/time_zone.rb:515 (tzinfo.now.to_date)
        #5	Date.current  3/15 <----- CORRECT
        Date.current
        
        # @time: ActiveSupport::TimeWithZone: Fri, 15 Mar 2024 00:00:00.000000000 UTC +00:00
        # time: Time 2024-03-14 17:00:00 -0700
        #0	Timecop::TimeStackItem#time(time_klass=Time) at ~/timecop/lib/timecop/time_stack_item.rb:79
        #1	Timecop::TimeStackItem#date(date_klass=Date) at ~/timecop/lib/timecop/time_stack_item.rb:99 (calls to_date)
        #2	Date.mock_date at ~/timecop/lib/timecop/time_extensions.rb:34
        #3	Date.today  3/14 <----- INCORRECT
        Date.today

My original suspicion was wrong because the call to TimeStackItem#time actually sets the same value for @time and the returned time. The difference being Date.current uses casts to the set UTC timezone before calling to_date, whereas Date.today calls to_date on the returned value directly.

I've proposed a change in 603b64d for Date.today to mirror the behavior of Date.current by changing TimeStackItem#date:

diff --git a/lib/timecop/time_stack_item.rb b/lib/timecop/time_stack_item.rb
index ef49150..0767f71 100644
--- a/lib/timecop/time_stack_item.rb
+++ b/lib/timecop/time_stack_item.rb
@@ -96,7 +96,9 @@ class Timecop
     end
 
     def date(date_klass = Date)
-      date_klass.jd(time.__send__(:to_date).jd)
+      t = time
+      t = t.respond_to?(:in_time_zone) ? t.in_time_zone : t
+      date_klass.jd(t.__send__(:to_date).jd)
     end

@joshuacronemeyer
Copy link
Collaborator

@jch can you create a new bundle and reproduce this issue in isolation? Your test is not failing on my machine OR on the time cop build... https://github.com/travisjeffery/timecop/actions/runs/10393723273/job/28788507003?pr=416

@jch
Copy link
Author

jch commented Aug 15, 2024

Testing this out in a codespace, the test passes because the system time in the instance is UTC and matches Time.zone in my test. I switched to the test helper each_timezone which will change the system time. This was a weird gotcha, and I realize now I should use Time.zone.today instead of Date.today in my rails app for a time zone aware "today" https://stackoverflow.com/a/19016136

Closing this out as I believe that Timecop shouldn't add additional behavior that differs from ActiveSupport defaults, even if this was confusing. Hopefully this closed issue will help others if they run into a similar issue.

Thanks @joshuacronemeyer for checking me 🙇

@jch jch closed this Aug 15, 2024
@joshuacronemeyer
Copy link
Collaborator

Been there. Timezones are the worst!

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

Successfully merging this pull request may close these issues.

2 participants