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

Fix time interval test #5083

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@ChrisBr
Contributor

ChrisBr commented Mar 9, 2018

and use RubyTime.convertTimeInterval for parsing to raise correct ArgumentError and TypeError exceptions

@kares

This comment has been minimized.

Member

kares commented Mar 19, 2018

looks good but it introduced one regression failure as a trade for one fixed, see CI's log.
with JI on places were a Ruby Numeric is expected (such as sleep) Java proxy numbers work as well.
... sample failure for reference https://travis-ci.org/jruby/jruby/jobs/351484156#L1555

@ChrisBr

This comment has been minimized.

Contributor

ChrisBr commented Mar 24, 2018

@kares thanks for the hint.

Unfortunately, the Java::JavaLang::Long gets passed in as a org.jruby.RubyString. So there is no way to distinguish if the RubyString is from the Long or if it was really a String. So seems there is no (easy) way to fix it or do I miss something?

@kares

This comment has been minimized.

Member

kares commented Mar 25, 2018

oh really? that ain't good than and the "regression" is legit, I will look into a way of pushing this forward ...

Fix time interval test
and use RubyTime.convertTimeInterval for parsing to raise correct ArgumentError and TypeError exceptions
@ChrisBr

This comment has been minimized.

Contributor

ChrisBr commented Mar 25, 2018

@kares tried again, seems I did sth wrong the last time 😢 It get's of course passed in as a ConcreteJavaProxy. Anyway, see my updated patch which passes now both tests.

@kares

This comment has been minimized.

Member

kares commented Mar 26, 2018

Thanks Chris, poked around and found out the time-interval method is used for elsewhere.
So, I made sure it works from all the places: d19e850 when a Number proxy is being passed in ...

isn't nice but at least works, for now. the whole piece is a new feature in 9.2, and I recall specifically sleep(java_num) was a reported issue that motivated implementing support for Java numbers.

The commit from here is on master as: fd9c772

@kares kares closed this Mar 26, 2018

@kares kares added this to the JRuby 9.2.0.0 milestone Mar 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment