This project is archived and is in readonly mode.

#2970 ✓wontfix
Adam Milligan

Parameters to functional test requests should have proper values

Reported by Adam Milligan | July 29th, 2009 @ 03:33 AM

How many times have you written this in a functional test:


post :create, :wibble => { :dingledongle => 7 }

It looks fine, and the test runs fine, but then you deploy and BOOM! because your Wibble model is assuming the dingledongle value is a number, but the real HTTP parameter is, in fact a string.

Or this:


post :index, :parent_id => @parent.id

Tests run fine, everything works, until you change #to_param on the Parent model to return a URL slug and then you're up all night fixing test failures. The :parent_id parameter isn't an ID, after all, it's part of a URL, and it should be a string.

In short, parameters to functional test requests should be strings, because that's what HTTP provides in real life. This patch verifies that this is the case, and throws a warm and friendly exception if you're trying to pass in something improper (it also allows TestUploadedFile instances, nil, and, for parameters used in routing, ActiveRecords.

This patch is about a dozen lines in TestRequest, a pile of tests, and a bunch of changes to tests that were passing in improper values.

Comments and changes to this ticket

  • Donald Ball

    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.

  • Will Bryant

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

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

  • Michael Koziarski

    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.

  • blatyo

    blatyo July 29th, 2009 @ 01:34 PM

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

  • David Siñuela (Siu)

    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

    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.

  • Adam Milligan

    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

    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.

  • Noel Rappin

    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.

  • BJ Clark

    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?

  • Joe Van Dyk

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

    I agree, a warning would be nice.

  • Michael Koziarski

    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.

  • Adam Milligan

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Attachments

Referenced by

Pages