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

Float vs BigDecimal #4300

Closed
kirs opened this issue Nov 15, 2016 · 3 comments
Closed

Float vs BigDecimal #4300

kirs opened this issue Nov 15, 2016 · 3 comments
Milestone
Invalid or Duplicate

Comments

@kirs
Copy link
Contributor

kirs commented Nov 15, 2016 

# on ruby 2.3.0p0 (2015-12-25 revision 53290) [x86_64-darwin15]
97.18 > BigDecimal.new("97.18")
=> true

Rails relies on the fact that the 97.18 as a float is greater than 97.18 as a BigDecimal due to floating point precision (https://github.com/rails/rails/blob/master/activemodel/test/cases/validations/numericality_validation_test.rb#L85
).

In JRuby that's not the case and a few ActiveModel tests fail:

# on jruby 9.1.6.0 (2.3.1) 2016-11-09 0150a76 Java HotSpot(TM) 64-Bit Server VM 25.45-b02 on 1.8.0_45-b14 +jit [darwin-x86_64]
97.18 > BigDecimal.new("97.18")
=> false

is it something that we can change in JRuby? or should we skip those tests in Rails when running on JRuby?

@headius
@kirs
Copy link
Contributor Author

kirs commented Nov 16, 2016

cc @guilleiguaran

@kares
Copy link
Member

kares commented Dec 5, 2016 •
edited

hey @kirs, thanks for the report. the test sound like it could be improved for the benefit of all :)
doubt there's actually any user reports on the behaviour and see no benefit in relying on the MRI specifics of how floats behave in that particular test ... would be perfect if it simply did compare ~ 97.19 > 97.18
-> should also make understanding the test code easier for any future numericality validation readers/writers

@kirs
Copy link
Contributor Author

kirs commented Dec 5, 2016

the test sound like it could be improved for the benefit of all

I absolutely agree. I ended up improving Rails' test suite to not rely on MRI specifics (rails/rails@5abf662)

@kirs kirs closed this as completed Dec 5, 2016
@kares kares added this to the Invalid or Duplicate milestone Dec 5, 2016
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
Invalid or Duplicate
Development

No branches or pull requests
2 participants
@kares @kirs