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

rewrite DateTime.local_offset to return current offset #3661

Closed
wants to merge 1 commit into from

Conversation

lest
Copy link
Contributor

@lest lest commented Nov 17, 2011

ActiveSupport tests is failing now for some time zones (e.g. Europe/Minsk and Europe/Moscow):

cd activesupport
TZ=Europe/Minsk rake test

This happens because current time zone is +03:00 but in 2007 year it was +02:00 (for Europe/Minsk).
It means that we need to return current non-DST offset in DateTime.local instead of historical one.

@lest
Copy link
Contributor Author

lest commented Nov 18, 2011

I've searched through rails sources, it's used only in tests:

~/code/rails (git)-[master] % ack "DateTime.local_offset"
activerecord/test/cases/migration_test.rb
627:            assert_equal DateTime.local_offset, bob.moment_of_truth.offset

activerecord/CHANGELOG.md
1031:*   test_native_types expects DateTime.local_offset instead of DateTime.now.offset; fixes test breakage due to dst transition *Geoff Buesing*

activesupport/test/core_ext/date_time_ext_test.rb
46:    assert_equal DateTime.civil(2010, 5, 4, 0, 0, 0, DateTime.local_offset), DateTime.civil_from_format(:local, 2010, 5, 4)
318:      assert_equal Rational(-5, 24), DateTime.local_offset
321:      assert_equal Rational(-6, 24), DateTime.local_offset

activesupport/test/core_ext/time_ext_test.rb
596:    assert_equal Time.time_with_datetime_fallback(:local, 2039, 2, 21, 17, 44, 30), DateTime.civil(2039, 2, 21, 17, 44, 30, DateTime.local_offset)
603:      assert_equal Time.time_with_datetime_fallback(:local, 1900, 2, 21, 17, 44, 30), DateTime.civil(1900, 2, 21, 17, 44, 30, DateTime.local_offset, 0)
625:    assert_equal Time.local_time(2039, 2, 21, 17, 44, 30), DateTime.civil(2039, 2, 21, 17, 44, 30, DateTime.local_offset)
628:      assert_equal Time.local_time(1901, 2, 21, 17, 44, 30), DateTime.civil(1901, 2, 21, 17, 44, 30, DateTime.local_offset)

activesupport/CHANGELOG.md
488:*   test_time_with_datetime_fallback expects DateTime.local_offset instead of DateTime.now.offset *Geoff Buesing*

And it cause tests to fail now in my timezone.

PS. also all russian time zones affected

@lest
Copy link
Contributor Author

lest commented Nov 18, 2011

@tenderlove, @jonleighton I would be glad to hear your thoughts about it?

@jonleighton
Copy link
Member

Please can you add a test to specifically target the problem in this method? (i.e. that will fail in any time zone)

@lest
Copy link
Contributor Author

lest commented Nov 19, 2011

that will fail in any time zone

Hmm. As I think it's impossible to write such sort of test.

But issue could be easily reproduced using TZ environment variable. Also recent tzdata package is needed in system.

This years Russia and Belarus decided to cancel DST and shifted their time zones. That is why for Europe/Minsk time zone in 2007 year offset was 2 hours and now it's 3 hours.

Here is values of Time#utc_offset from irb:

irb(main):001:0> Time.local(2007).utc_offset
=> 7200
irb(main):002:0> Time.now.utc_offset
=> 10800
irb(main):003:0> Time.local(2011, 11, 19).utc_offset
=> 10800

@lest
Copy link
Contributor Author

lest commented Nov 19, 2011

I've got idea how to wrote test and fix it correctry.
Method DateTime.civil_from_format is also affected.

@lest lest closed this Nov 19, 2011
@lest
Copy link
Contributor Author

lest commented Nov 19, 2011

Closed until I refactor it and write with tests

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.

None yet

2 participants