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

zone_spec.rb fails SPEC test "Time#zone returns UTC when called on a UTC time" #3304

Closed
tdaitx opened this issue Sep 2, 2015 · 5 comments · Fixed by #3310
Closed

zone_spec.rb fails SPEC test "Time#zone returns UTC when called on a UTC time" #3304

tdaitx opened this issue Sep 2, 2015 · 5 comments · Fixed by #3310
Labels
core JDK compatibility JRuby 1.7.x JRuby 9000
Milestone
JRuby 1.7.22

Comments

@tdaitx
Copy link
Contributor

tdaitx commented Sep 2, 2015

As described in #3303 the combination of joda-time <= 2.8.0 and OpenJDK 8u60 or earlier causes joda-time to return a [+-]hh:mm TZ format when calling DateTimeZone.getShortName() or DateTimeZone.getName(). This in turn causes RubyTime.zone() to return GMT instead of UTC by following a different code path then usual.

Current result

jruby-1.7.21$ ./bin/jruby spec/mspec/bin/mspec run spec/ruby/core/time/zone_spec.rbjruby 1.7.21 (1.9.3p551) 2015-09-01 fffffff on OpenJDK 64-Bit Server VM 1.8.0_66-internal-b01 +jit [linux-amd64]
..F

1)
Time#zone returns UTC when called on a UTC time FAILED
Expected "GMT"
 to equal "UTC"

/build/jruby-shb6eX/jruby-1.7.21/spec/ruby/core/time/zone_spec.rb:19:in `(root)'
org/jruby/RubyBasicObject.java:1574:in `instance_eval'
org/jruby/RubyEnumerable.java:1412:in `all?'
org/jruby/RubyFixnum.java:275:in `times'
org/jruby/RubyArray.java:1613:in `each'
/build/jruby-shb6eX/jruby-1.7.21/spec/ruby/core/time/zone_spec.rb:4:in `(root)'
org/jruby/RubyKernel.java:1059:in `load'
org/jruby/RubyBasicObject.java:1574:in `instance_eval'
org/jruby/RubyArray.java:1613:in `each'

Finished in 0.061000 seconds

Expected result

As per Time#zone documentation it should return UTC.

Offending code

I believe that the if (zone.equals("+00:00") bellow should return UTC instead of GMT.

RubyTime.zone() [890-908]

        String zone = dt.getZone().getShortName(dt.getMillis());

        Matcher offsetMatcher = TIME_OFFSET_PATTERN.matcher(zone);

        if (offsetMatcher.matches()) {
            boolean minus_p = offsetMatcher.group(1).toString().equals("-");
            int hourOffset  = Integer.valueOf(offsetMatcher.group(2));

            if (zone.equals("+00:00")) {
                zone = "GMT";
            } else {
                // try non-localized time zone name
                zone = dt.getZone().getNameKey(dt.getMillis());
                if (zone == null) {
                    char sign = minus_p ? '+' : '-';
                    zone = "GMT" + sign + hourOffset;
                }
            }
        }
@headius
Copy link
Member

headius commented Sep 3, 2015

joda was updated for JRuby 1.7.22, released last week, and 9.0.1.0 released yesterday. Are you seeing these issues with the latest releases?

@headius
Copy link
Member

headius commented Sep 3, 2015

Neither 1.7.22 nor 9.0.1.0 appear to fail the spec in question here, so I will be marking this fixed in those releases. Let me know if you're still seeing an issue.

@headius headius closed this as completed Sep 3, 2015
@headius headius added this to the JRuby 1.7.22 milestone Sep 3, 2015
@tdaitx
Copy link
Contributor Author

tdaitx commented Sep 4, 2015

@headius you are right that running it against joda-time 1.8.1 or earlier will prevent the issues, but I wouldn't call that fixing them. The underlying problem is still there, if joda-time getShortName presents yet another problem in the future, then the issues will show up again.

IMHO JRuby itself should be more resilient and behave as closely as expected by the spec.

The following patch would fix it.

--- a/core/src/main/java/org/jruby/RubyTime.java 2015-09-04 09:45:25.733338886 -0300
+++ b/core/src/main/java/org/jruby/RubyTime.java 2015-09-04 09:45:16.441344417 -0300
@@ -896,7 +896,7 @@
             int hourOffset  = Integer.valueOf(offsetMatcher.group(2));

             if (zone.equals("+00:00")) {
-                zone = "GMT";
+                zone = "UTC";
             } else {
                 // try non-localized time zone name
                 zone = dt.getZone().getNameKey(dt.getMillis());

Let me know if that is a reasonable fix. I can also create a pull request for it.

@mkristian
Copy link
Member

@tdaitx both jruby-1.7.22 and jruby-9.0.0.0 default to joda-2.8.2: https://github.com/jruby/jruby/blob/9.0.1.0/pom.rb#L80

I do not understand your remark.

if your patch brings JRuby closer to MRI and passes all specs then go for PR. @headius ???

tdaitx added a commit to tdaitx/jruby that referenced this issue Sep 4, 2015
@tdaitx
return UTC instead of GMT (fixes jruby#3304)
b681987 
As per Ruby's Time#zone [1] UTC should be used instead of GMT
since Ruby 1.8. This also fixes jruby#3304.

[1] http://ruby-doc.org/core-2.2.3/Time.html#method-i-zone
@tdaitx tdaitx mentioned this issue Sep 4, 2015
Merged
@tdaitx
Copy link
Contributor Author

tdaitx commented Sep 4, 2015

@mkristian I meant that the tests work fine when joda-time behaves as expected, otherwise JRuby will fail the tests, IMHO it shouldn't depend so much on joda-time.

I have created #3310 to fix this. =)

Thanks!

mkristian added a commit that referenced this issue Sep 8, 2015
@mkristian
Merge pull request #3310 from tdaitx/fix-3304-return-utc
0596e57 
return UTC instead of GMT (fixes #3304)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
core JDK compatibility JRuby 1.7.x JRuby 9000
Projects
None yet
Milestone
JRuby 1.7.22
Development

Successfully merging a pull request may close this issue.

return UTC instead of GMT (fixes #3304) tdaitx/jruby
3 participants
@headius @mkristian @tdaitx