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.

Sign up for GitHub

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

Jump to bottom

Wrong julian day for DateTime with a negative rational offset #1770

Closed
robin850 opened this issue Jun 27, 2014 · 2 comments
Closed

Wrong julian day for DateTime with a negative rational offset #1770

robin850 opened this issue Jun 27, 2014 · 2 comments
Milestone
JRuby 9.0.1.0

Comments

@robin850
Copy link
Contributor

Hello,

There seems to be a problem with the Julian day of a DateTime object when the latter has been initialized specifying a negative offset. With JRuby 1.7.13 and MRI 2.1.2:

>> require 'date'
=> true
>> without_offset = DateTime.civil(1999, 12, 31, 19, 0, 0);
>> with_offset = DateTime.civil(1999, 12, 31, 19, 0, 0, Rational(-5, 24))

>> RUBY_ENGINE
=> "jruby"
>> without_offset.jd
=> 2451544
>> with_offset.jd
=> 2451545 # Incremented by 1

>> RUBY_ENGINE
=> "ruby"
>> without_offset.jd
=> 2451544
>> with_offset.jd
=> 2451544

The bug isn't reproducible with any Rational object. It looks like it should be negative and close to 0 (e.g. -5 / 24.0 works but 5 / 24.0 don't). Here's a failing test:

diff --git a/test/mri/date/test_date_new.rb b/test/mri/date/test_date_new.rb
index 0bbbfee..bebafbe 100644
--- a/test/mri/date/test_date_new.rb
+++ b/test/mri/date/test_date_new.rb
@@ -31,6 +31,13 @@ class TestDateNew < Test::Unit::TestCase
     end
   end

+  def test_jd__with_offset
+    without_offset = DateTime.civil(1999, 12, 31, 19, 0, 0)
+    with_offset = DateTime.civil(1999, 12, 31, 19, 0, 0, Rational(-5, 24))
+
+    assert_equal without_offset.jd, with_offset.jd
+  end
+
   def test_ordinal
     d = Date.ordinal
     dt = DateTime.ordinal

Have a nice day.
@rtyler
Copy link

rtyler commented Aug 7, 2015

@robin850 For what it's worth, the JRuby 1.7.x branch strives to be Ruby 1.9.3 compatible, so I don't think that the original comparison between 1.7.3 and MRI 2.1.2 is correct.

Regardless, the failing test you've provided will pass on JRuby 9.0.0.0:

[2] pry(main)> require 'date'
=> true
[3] pry(main)> without_offset = DateTime.civil(1999, 12, 31, 19, 0, 0)
=> #<DateTime: 1999-12-31T19:00:00+00:00 ((2451544j,68400s,0n),+0s,2299161j)>
[4] pry(main)> with_offset = DateTime.civil(1999, 12, 31, 19, 0, 0, Rational(-5, 24)))  
=> #<DateTime: 1999-12-31T19:00:00-05:00 ((2451544j,86400s,0n),-18000s,2299161j)>
[5] pry(main)> without_offset.jd == with_offset.jd
=> true
[6] pry(main)>

@rtyler rtyler closed this as completed Aug 7, 2015
@robin850
Copy link
Contributor Author

I don't think that the original comparison between 1.7.3 and MRI 2.1.2 is correct

Actually this should pass even under 1.9.3 AFAIK.

Regardless, the failing test you've provided will pass on JRuby 9.0.0.0

What do you think about adding the provided regression test (or as a RubySpec as it looks like you are directly merging MRI's tests now) even though it's already fixed ?

@enebo enebo added this to the JRuby 9.0.1.0 milestone Aug 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
JRuby 9.0.1.0
Development

No branches or pull requests
3 participants
@enebo @rtyler @robin850