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

Fix time interval test #5083

Closed
wants to merge 1 commit into from
Closed

Conversation

ChrisBr
Copy link
Contributor

@ChrisBr ChrisBr commented Mar 9, 2018

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

@kares
Copy link
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
Copy link
Contributor Author

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
Copy link
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 ...

@ChrisBr ChrisBr force-pushed the test/fix_time_interval branch from 620349f to 42f556b Compare March 25, 2018 21:28
and use RubyTime.convertTimeInterval for parsing to raise correct ArgumentError and TypeError exceptions
@ChrisBr ChrisBr force-pushed the test/fix_time_interval branch from 42f556b to 81271f6 Compare March 25, 2018 21:28
@ChrisBr
Copy link
Contributor Author

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
Copy link
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants