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 NumericData.average test on 2.6.0-dev #34601

Merged
merged 1 commit into from Dec 3, 2018

Conversation

Projects
None yet
4 participants
@evexoio
Copy link
Contributor

evexoio commented Dec 3, 2018

Fixes #34600

Summary

NumericData.average was returning nil on ruby versions <= ruby 2.6.0dev (2018-12-03 trunk 66154)

Now it returns 0.0 instead of nil

@rails-bot rails-bot bot added the activerecord label Dec 3, 2018

@kamipo

This comment has been minimized.

Copy link
Member

kamipo commented Dec 3, 2018

Can you investigate why NumericData.average with Ruby 2.6 returns 0.0?

@evexoio

This comment has been minimized.

Copy link
Contributor

evexoio commented Dec 3, 2018

@kamipo Sure, that's what I'm doing right now.

#34600 (comment)

@evexoio

This comment has been minimized.

Copy link
Contributor

evexoio commented Dec 3, 2018

@kamipo This should be the commit that caused this issue. ruby/ruby@a0e438c#diff-6b866d482baf2bdfd8433893fb1f6d36R144

What's the right action to take right now?

@@ -58,7 +58,7 @@ def test_should_return_integer_average_if_db_returns_such
end

def test_should_return_nil_as_average
assert_nil NumericData.average(:bank_balance)

This comment has been minimized.

@kamipo

kamipo Dec 3, 2018

Member

This assertion actually would be failed without Ruby 2.6.0 if bigdecimal 1.4.0.pre.20181130a is used.

def type_cast_calculated_value(value, type, operation = nil)
case operation
when "count" then value.to_i
when "sum" then type.deserialize(value || 0)
when "average" then value.respond_to?(:to_d) ? value.to_d : value
else type.deserialize(value)

How about checking nil.respond_to?(:to_d)?

--- a/activerecord/test/cases/calculations_test.rb
+++ b/activerecord/test/cases/calculations_test.rb
@@ -57,8 +57,12 @@ def test_should_return_integer_average_if_db_returns_such
     assert_equal 3, value
   end
 
-  def test_should_return_nil_as_average
-    assert_nil NumericData.average(:bank_balance)
+  def test_should_return_nil_to_d_as_average
+    if nil.respond_to?(:to_d)
+      assert_equal BigDecimal(0), NumericData.average(:bank_balance)
+    else
+      assert_nil NumericData.average(:bank_balance)
+    end
   end
 
   def test_should_get_maximum_of_field

This comment has been minimized.

@evexoio

evexoio Dec 3, 2018

Contributor

Makes sense, Thanks for helping out 🙌

@evexoio evexoio force-pushed the evexoio:34600-test_should_return_nil_as_average branch from 664c45a to 89b4612 Dec 3, 2018

@kamipo kamipo merged commit 41e468a into rails:master Dec 3, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

utilum added a commit to utilum/rails that referenced this pull request Dec 4, 2018

Another Ruby 2.6 BigDecimal compatibility issue
This patch modifies XmlMini::Parsing["decimal"] to handle a string that
contains an invalid number. Since [ruby/ruby@a0e438c#diff-6b866d482baf2bdfd8433893fb1f6d36R144](ruby/ruby@a0e438c#diff-6b866d482baf2bdfd8433893fb1f6d36R144) this case raises an `ArgumentError`. `String.to_f` returns 0.0 if there is not a valid number at the start of the argument, so current behavior is conserved.

See https://travis-ci.org/rails/rails/jobs/463180341#L6264

Related: rails#34600, rails#34601

rafaelfranca added a commit that referenced this pull request Dec 4, 2018

Merge pull request #34601 from evexoio/34600-test_should_return_nil_a…
…s_average

Fix NumericData.average test on 2.6.0-dev

albertoalmagro added a commit to albertoalmagro/rails that referenced this pull request Jan 3, 2019

Update documentation for average [ci skip]
Default value for `average` was fixed here rails#34601 but documentation
wasn't updated. This patch updates its documentation.

Clarifies rails#34850
@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Jan 3, 2019

I think this should still return nil. Can someone fix it?

@albertoalmagro

This comment has been minimized.

Copy link
Contributor

albertoalmagro commented Jan 3, 2019

I'm on it @rafaelfranca

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