Skip to content

Fix time-only DateTime.parse under freeze#440

Open
konieczkow wants to merge 2 commits intotravisjeffery:masterfrom
konieczkow:fix-time-only-datetime-parse
Open

Fix time-only DateTime.parse under freeze#440
konieczkow wants to merge 2 commits intotravisjeffery:masterfrom
konieczkow:fix-time-only-datetime-parse

Conversation

@konieczkow
Copy link

Summary

DateTime.parse('HH:MM') under Timecop.freeze used to fall back to parsed_date + travel_offset_days, where:

travel_offset_days = (@travel_offset / 86400).round

Because @travel_offset was computed against the host clock, the rounding flipped whenever the host time was ~12 hours away from the frozen time.

Example

Freezing at 2017-08-10 10:00 UTC while the real clock is 2025-11-13 22:00 UTC yields an offset of 3015.5 days, so round adds an extra day and:

DateTime.parse("01:00")
⇒ 2017-08-09 01:00   # Should be 2017-08-10 01:00

If the host clock moves back to 21:00, the offset becomes 3015.458..., rounds to 3015, and the result “jumps forward.”

Fix

The parser now explicitly constructs the DateTime from the mocked year/month/day whenever only hour/minute are present and sets the seconds to 0.
Results no longer depend on the host clock.

Added two regression tests:

  • test_date_time_parse_time_only_hour_minute
    Freezes at now + 40h so the former rounding bug reproduces deterministically.

  • test_date_time_parse_time_without_seconds
    Validates time-only inputs (00:00, 01:00, 12:30, 23:59) and ensures they always anchor to the mocked date.

DateTime.parse('HH:MM') previously fell back to parsed_date + travel_offset_days,
with travel_offset_days = (@travel_offset / 86400).round. Because @travel_offset
measures the gap between the frozen moment and the real system clock, that rounding
stepped up or down depending on how close the real clock was to a 12‑hour boundary.
For example, freezing at 2017-08-10 10:00 UTC while the host clock is
2025-11-13 22:00 UTC yields an offset of N.5 days, so rounding adds an
extra day and parsing “01:00” returns 2017-08-09 instead of 2017-08-10.

Anchor hour/minute-only parses to the mocked year/month/day and add deterministic
regression tests to ensure time-only input no longer depends on the host clock.
@joshuacronemeyer
Copy link
Collaborator

Hi @konieczkow I wanted to mention that I started looking at your bug/PR yesterday and I really appreciate you taking time to track this down. I haven't had a chance to finish reviewing it yet, but i'm going to this week. I was very interested in the faketime tool you used. I noticed your tests don't fail for me without it. Ideally we'd have a test case that would always demonstrate this issue, but I'm willing to move forward with some manual steps as well.

Do you have any ideas for improving the test so it always fails?

@konieczkow
Copy link
Author

Hi @konieczkow I wanted to mention that I started looking at your bug/PR yesterday and I really appreciate you taking time to track this down. I haven't had a chance to finish reviewing it yet, but i'm going to this week. I was very interested in the faketime tool you used. I noticed your tests don't fail for me without it. Ideally we'd have a test case that would always demonstrate this issue, but I'm willing to move forward with some manual steps as well.

Do you have any ideas for improving the test so it always fails?

Hi @joshuacronemeyer! Apologies for the previous test.

For some context, I found this bug while debugging why my tests were failing seemingly at random, which later turned out to be "only between 23:00-24:00".

Could you check if this test fails for you without the changes?

def test_date_time_parse_time_without_seconds
  real_now = Time.now_without_mock_time
  offset = 12 * 3600 + 30  # 12 hours + buffer
  freeze_time = real_now.hour >= 12 ? real_now - offset : real_now + offset
  
  Timecop.freeze(freeze_time) do
    frozen_date = Date.today
    [
      ['00:00', 0, 0],
      ['12:00', 12, 0],
      ['12:30', 12, 30],
      ['23:59', 23, 59]
    ].each do |time_str, expected_hour, expected_min|
      parsed = DateTime.parse(time_str)
      assert_equal frozen_date, parsed.to_date, "Wrong date for '#{time_str}'"
      assert_equal expected_hour, parsed.hour, "Wrong hour for '#{time_str}'"
      assert_equal expected_min, parsed.min, "Wrong minute for '#{time_str}'"
      assert_equal 0, parsed.sec, "Wrong seconds for '#{time_str}'"
    end
  end
end

The trick is creating a 12+ hour offset while staying on the same calendar day. This forces travel_offset_days to round to +/-1 instead of 0, exposing the bug regardless of when the test runs.

If it works, I'll update the PR.

@joshuacronemeyer
Copy link
Collaborator

well done @konieczkow this test is working for me!

Can you change the name of it to be more descriptive? one reason is there is already a test called test_date_time_parse_time_without_seconds so your test might get overwritten or overwrite the existing one, but also because it will help me remember what this exact scenario is.

@joshuacronemeyer
Copy link
Collaborator

@konieczkow i'm still waiting for you on this. If I don't hear from you i'll change the test name myself. Thanks.

- test_date_time_parse_hhmm_uses_frozen_date_not_real_clock: regression test ensuring parsed time uses frozen date, not host clock
- test_date_time_parse_hhmm_format_returns_correct_time: basic HH:MM format parsing
@konieczkow
Copy link
Author

Apologies for the delay! I've renamed the tests to be more descriptive:

  • test_date_time_parse_hhmm_uses_frozen_date_not_real_clock — the regression test
  • test_date_time_parse_hhmm_format_returns_correct_time — basic HH:MM parsing

Hopefully these are clearer. Let me know if you had something else in mind.

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