Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  Donald Ball July 29th, 2009 @ 04:03 AM

  We just ran afoul of this on our project; this patch would have helped us catch our mistake earlier.

• You flagged this item as spam.
  

  Will Bryant July 29th, 2009 @ 05:22 AM

  Shouldn't the test wrappers just automatically convert everything to a string?

• 

  Michael Koziarski July 29th, 2009 @ 06:01 AM

  • State changed from “new” to “wontfix”

  There's no way I'm going to break all those tests to catch this case.

  I would take a patch like will's talking about where everything passed is to_param'd, but that's about it.

• You flagged this item as spam.
  

  blatyo July 29th, 2009 @ 01:34 PM

  I agree with Will. It should convert it to the string rather than telling the tester to.

• You flagged this item as spam.
  

  David Siñuela (Siu) July 30th, 2009 @ 11:29 AM

  • Tag set to “parameters, params, testing”

  I'm having issues with this too, I think everything should be converted to string/param automatically.

• 

  Xavier Noria July 30th, 2009 @ 04:42 PM

  That patch is gonna break a lot of stuff just to add a little safety for not using proper types, which is your fault anyway ("you" being generic). I don't think this justifies breaking existing test suites.

  Now, since it is the programmer's responsability to pass objects with the correct type and to_param is idiomatic, I'd find automatic to_param to be a convenience that may be useful and fits with its automatic usage by Rails itself.

• You flagged this item as spam.
  

  Adam Milligan July 30th, 2009 @ 05:52 PM

  I didn't really expect this patch to be committed as is. However, the idea that Rails should automatically convert all of your test parameters to something ostensibly correct is worse than the existing functionality.

  Consider this:

  
get :show, :some_boolean_checkbox => false

  This will currently run just fine, although it's not testing the right thing. Would you prefer that Rails call #to_s or #to_param on this parameter? #to_s will return "false", #to_param will return false (the boolean value). Neither is correct.

  It doesn't take very much imagination to think up a number of scenarios in which automatically calling #to_s or #to_param would change the parameters in subtlely incorrect ways.
  

  As far as the argument that writing tests correctly is "your responsibility," as it turns out writing production code correctly is your responsibility as well. Yet, somehow we regularly screw that up, so we write tests to catch those mistakes. The "people just shouldn't make that mistake" argument is never compelling.

  As I said, I didn't really expect anyone would commit this patch. I repurposed it as a plugin (http://github.com/amilligan/wapcaplet/tree/master) for anyone who cares to fix their tests. I'll likely resubmit the patch again with warnings rather than errors, and possibly some configuration options for changing the behavior, but that's just because I enjoy rejection.

• 

  Xavier Noria July 30th, 2009 @ 11:30 PM

  As far as the argument that writing tests correctly is "your responsibility," as it turns out writing production code correctly is your responsibility as well. Yet, somehow we regularly screw that up, so we write tests to catch those mistakes. The "people just shouldn't make that mistake" argument is never compelling.

  The argument is not just that. The argument goes that since this is not fixing something seriously broken, it is only a safety check, I think it is not worth breaking everyone's test suite. It is the solution taken together with its consequences that make me vote for pointing to just using the correct types.

  The idea itself is fine for me, in my view it is a nice to have safety check taken in isolation.

• You flagged this item as spam.
  

  Noel Rappin August 2nd, 2009 @ 03:55 AM

  I'm sympathetic to the idea that this patch would break a lot of existing tests, but I agree with Adam that the responsibility argument is not compelling. I've always felt that silently allowing non-string arguments here was somewhat counter to the rest of Rails, in that Rails often automatically assures type safety in other contexts (such as ActiveRecord fields). It's always seemed a weird oversight that the framework lets me shoot myself in the foot with this particular bullet.

• You flagged this item as spam.
  

  BJ Clark August 2nd, 2009 @ 04:41 AM

  What if, instead of breaking the tests, it just put a warning when a parameter wasn't a string?

• You flagged this item as spam.
  

  Joe Van Dyk August 2nd, 2009 @ 05:43 AM

  I agree, a warning would be nice.

• 

  Michael Koziarski August 2nd, 2009 @ 06:40 AM

  OK, investigate a change which calls to_param on any non-string values
  and prints out a deprecation warning (just because it's a warning
  mechanism, we won't necessarily remove this support in the future).

  We'll see how spammy it is and do a cost-benefit comparison based on that.

  I'm definitely not going to break everyone's tests to just make sure
  that you're passing @foo.to_param instead of @foo.id, but we can
  definitely look at warning folks. On the flipside you can provide
  strongly typed data to controllers using the XML parser, so this whole
  discussion could be moot.

• You flagged this item as spam.
  

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

  I've created a new patch with warnings and options here: https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets...