Rails Ruby on Rails → 3.0.2

Comments and changes to this ticket

• You flagged this item as spam.
  

  Adam Milligan August 2nd, 2009 @ 07:55 AM

  Apparently I fail at markdown. The link for the previous patch is https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets...

• You flagged this item as spam.
  

  Will Bryant August 2nd, 2009 @ 12:01 PM

  Can we at least not warn on integers? They have a trivial string <-> fixnum conversion, and are used absolutely everywhere in tests for IDs.

• You flagged this item as spam.
  

  Adam Milligan August 2nd, 2009 @ 04:55 PM

  Integers are one of the most nefarious culprits in functional tests, so no, allowing integers is a terrible idea.

  In the case of non-path parameters:

  
post :create, :person => { :age => 23 }


  I've seen any number of cases where people wrote code in the controller or model setter expecting an integer value. Tests pass, production blows up.

  In the case of path parameters:

  
get :show, :parent_id => @parent.id, :id => @child.id


  I can't count the number of times this has caused trouble on projects I've worked on. Despite the naming convention those are not IDs. They are URL parameters, for which ActiveRecord provides the #to_param method. The functional testing framework makes this easier by automatically calling #to_param on values passed for path parameters. So, you have to type more in order to make the test wrong.

• You flagged this item as spam.
  

  Zach Brock August 2nd, 2009 @ 08:04 PM

  We're using the plugin version of this (wapcaplet) on our project and it's saved us from a couple of big production bugs. I'd definitely +1 this for inclusion into Rails.

• 

  Michael Koziarski August 3rd, 2009 @ 12:33 AM

  • State changed from “new” to “incomplete”

  I'm with will, just silently .to_s/to_param the integers.

  The vast bulk of tests just pass the id in those places and they'll work fine. Users overloading to_param are in the minority and we shouldn't spam everyone else just to satisfy them.

  We should gently guide people in the right direction, not lecture or abuse them.

• You flagged this item as spam.
  

  Neeraj August 3rd, 2009 @ 02:40 AM

  I will say that majority do not pay attention to warnings. Breaking the test seems like a good idea to me because what's the point of having a faulty test.

• 

  Michael Koziarski August 3rd, 2009 @ 02:42 AM

  Breaking things to lecture people isn't how we do things around here,
  that's not up for discussion.

• 

  Yehuda Katz (wycats) August 4th, 2009 @ 08:33 AM

  • Milestone cleared.

  @koz I agree that we shouldn't just break people's tests, but it does seem like a good idea to have a mode that will alert people to tests that do not reflect reality. I know I'd personally like such a mode. Thoughts?

• You flagged this item as spam.
  

  Will Bryant August 4th, 2009 @ 11:46 AM

  FWIW, I don't think people are actually disagreeing all that much here.

  Neeraj, re your comments, I agree that many people will not read warnings, but as Koz says, we don't normally make things break just to hassle people. However Adam's patch already gives options for this, so we can leave the default to be warnings only, and people who want to catch such problems earlier can set the option to raise an error instead. Everyone should be OK with this option.

  But there's really no reason to not just "do what I mean" with simple value types. Koz and I are saying that we should #to_s or #to_param integers, so Adam, your tests will still give correct results (ie. breakages when the app doesn't handle string parameters), without requiring anyone to write in literally thousands of #to_s calls themselves in their test cases.

  We should always make our test frameworks do the right thing, and I think we're all agreed that the right thing is for parameters to come in in the same format as they do when the app is actually running. The only reason we want a warning/error system at all is that for things other than integers and strings, it's not necessarily clear what the programmer meant to send as the parameter.

  So, I think that with small modification of Adam's patch, everyone should be happy? - accurate test results plus warnings (or optionally errors) when people have supplied a param that's not a string or integer.

  Point for discussion: if someone uses

  post :update, :id => my_models(:some_model)
  

  • should we automatically map ActiveRecord instances to their ID (and from there to a string) using #to_param also, as we will do for Fixnums, or should we rely on the warning/error system? In other words, is it clear from :id => my_models(:some_model) that the programmer means to submit the ID of the instance as the parameter value?
• 

  Yehuda Katz (wycats) August 4th, 2009 @ 05:33 PM

  @will while I generally agree that trying to do the right thing automatically for users as the default with no complaints is the right thing in Rails, I disagree with regard to tests. A test that accidentally works might actually be masking a bug, while code that accidentally works by definition doesn't.

  That said, I absolutely agree that changing the default to break virtually everyone's tests overnight is unacceptable. That's why I like "warn by default, optionally raise, optionally shut off warnings".

  Thoughts?

• 

  Michael Koziarski August 4th, 2009 @ 10:40 PM

  I don't see the merits to giving people an option to raise if there's
  warnings there, however we could extract that to a single method so
  it's easy for plugins to override.

• You flagged this item as spam.
  

  Will Bryant August 5th, 2009 @ 12:47 AM

  @Yehuda, can you suggest a scenario where automatically converting IDs/other integers to strings, like they would come in from HTTP, would make a test accidentally work, masking a bug?

• You flagged this item as spam.
  

  Adam Milligan August 5th, 2009 @ 01:07 AM

  Will, look about six messages up, I already gave an example. Here it is again:

  post :create, :person => { :age => 23 }
  

  Here's the controller:

  class PersonController < ApplicationController
    def create
      if params[:age] < 18
        do_something
      else
        do_something_else
      end
    end
  end
  

  Or, perhaps in the model:

  class Person
    def age=(new_age)
      raise "Too young!" if age < 18
      write_attribute(:age, new_age)
    end
  end
  

  Neither of these are examples of good code, but that doesn't mean code like this, lots of it, doesn't exist. In either case the test will pass with an integer parameter, and fail horribly with a string parameter. I'd prefer to have it fail horribly in a test rather than in production.

• You flagged this item as spam.
  

  Will Bryant August 5th, 2009 @ 01:36 AM

  No, that's an example of an inaccurate test pass due to the integer parameter being passed through to the controller as an integer.

  What Koz and I are talking about is the test framework converting those integer parameters to strings, so:

  post :create, :person => { :age => 23 }
  

  will effectively get converted to:

  post :create, :person => { :age => '23' }
  

  So the test would then fail.

• 

  Michael Koziarski August 5th, 2009 @ 03:05 AM

  What will said.

  I don't believe there are any scenarios where this would continue
  masking bugs if we called to_param on all the provided values

• 

  Mikel Lindsaar January 24th, 2010 @ 10:35 PM

  Adam, is this still an issue you wish to resolve?

  Will's suggestion of updating your patch to call :to_param on integers seems sound, you should also log a warning that this is happening so that we can guide people to do the right thing.

  Mikel

• 

  Dan Pickett May 9th, 2010 @ 06:29 PM

  • Tag changed from “action_controller, parameters, params, patch, tested, testing” to “action_controller, bugmash, parameters, params, patch, tested, testing”
  • Assigned user set to “Ryan Bigg”

  This is fairly stale - assigning to Ryan for his review. Ryan, please remove the bugmash tag if you find this no longer relevant. Thanks!

• 

  Ryan Bigg May 9th, 2010 @ 10:15 PM

  I think this has already been patched but I can't find the ticket.

• 

  Rizwan Reza May 16th, 2010 @ 03:48 AM

  • Tag changed from “action_controller, bugmash, parameters, params, patch, tested, testing” to “action_controller, parameters, params, patch, tested, testing”
  • State changed from “incomplete” to “resolved”

  Please reopen if needed.

• 

  Jeremy Kemper October 15th, 2010 @ 11:01 PM

  • Milestone set to “3.0.2”
  • Importance changed from “” to “”

  [bulk edit]