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

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

Comments

@robin850
Copy link
Contributor

@robin850 robin850 commented Jun 27, 2014

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 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 Aug 7, 2015
@robin850
Copy link
Contributor Author

@robin850 robin850 commented Aug 10, 2015

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
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants