Rails Ruby on Rails → 3.1

Comments and changes to this ticket

• 

  Santiago Pastorino February 3rd, 2011 @ 05:30 PM

  • Importance changed from “” to “Low”

  hey Ken, can you provide more info about which databases return integer values for AVG?.
  Also your patch have no test and you could do ...

  value.respond_to? :to_d ? value.to_d : value

• You flagged this item as spam.
  

  Ken Collins February 3rd, 2011 @ 06:58 PM

  Specifically SQL Server. Details on return type here.
  http://msdn.microsoft.com/en-us/library/ms177677.aspx

  Re the code style, I do like your line better and tend to do that more often, but working so much in ARel lately, I started using the Constant===. Maybe for speed? I dunno. I'm fine either way and actually like the concise respond_to? better.

  Re the tests, I have done short patches like these before since a test would have been deep implementation mock. I can easily do one for regression purposes and would prefer on if that is what you prefer too?

• 

  Santiago Pastorino February 3rd, 2011 @ 07:25 PM

  • State changed from “new” to “open”
  • Milestone set to “3.1”
  • Assigned user set to “Santiago Pastorino”

  Ok please provide a test case and use the respond_to? and I will push the patch ;).
  Thanks.

• You flagged this item as spam.
  

  Ken Collins February 3rd, 2011 @ 07:49 PM

  Trying this right now. Just got to figure out the right stub point. For instance.

  def test_should_return_integer_average_if_db_returns_such
    Account.connection.stubs :execute => [{'avg_id' => 3}]
    value = Account.average(:id)
    assert_equal 3, value
  end
  

  This works with sqlite3 but not mysql. Will come up with something shortly.

• You flagged this item as spam.
  

  Ken Collins February 3rd, 2011 @ 08:15 PM

  OK, uploaded a new patch. Test added, used the respond_to? method. All tests passed in mysql/postgresql/sqlite.

  Also, will this make it into the v3.0.4 release?

  • type_cast_calculated_value.diff 1.7 KB
• 

  Santiago Pastorino February 3rd, 2011 @ 09:03 PM

  Ken please avoid stubs you can find lot of examples in ActiveRecord tests about how things are tested.
  Also you have removed the try, are you sure that this is right? what happens if value is nil?.
  Thanks.

• You flagged this item as spam.
  

  Ken Collins February 3rd, 2011 @ 09:12 PM

  I had a test that directly tested the method but that it would have been discouraged. It looked something like this.

  expected_value = 3
  column = Account.columns_hash['id']
  returned_value = Account.send :type_cast_calculated_value, expected_value, column, 'average'
  assert_equal expected_value, returned_value
  

  Re the nil. To my knowledge there is no NilClass#to_d method. The same thing would happen as before. And lastly, nil would be an expected result from a simple function where there is 0 rows.

  How should I proceed?

• You flagged this item as spam.
  

  Ken Collins February 3rd, 2011 @ 09:35 PM

  FYI, this test is below mine and is even visible in the patch. It outlines the nil question of yours.

  def test_should_return_nil_as_average
    assert_nil NumericData.average(:bank_balance)
  end
  

  I am still working on coming up with a test style that is acceptable. Typically I feel dirty doing these types of test due to their coupling to the low level internals. They are tricky to get right and any feedback would be appreciated. So no database in the blessed 3 are going to return an integer, the stub seemed appropriate and allowed the test to stay high level. Testing the private method directly seems wrong too. Thoughts?

• 

  Repository February 3rd, 2011 @ 10:03 PM

  • State changed from “open” to “committed”

  (from [95d5d9b6c48c08f1fba0c77ecbc97b62b2603824]) The type_cast_calculated_value method will trust DB types before casting to a BigDecimal.

  [#6365 state:committed]

  Signed-off-by: Santiago Pastorino santiago@wyeworks.com
  https://github.com/rails/rails/commit/95d5d9b6c48c08f1fba0c77ecbc97...

• 

  Repository February 3rd, 2011 @ 10:16 PM

  (from [068527baaf9a49862281c4357296262ae73542d0]) The type_cast_calculated_value method will trust DB types before casting to a BigDecimal.

  [#6365 state:committed]

  Signed-off-by: Santiago Pastorino santiago@wyeworks.com
  https://github.com/rails/rails/commit/068527baaf9a49862281c43572962...