This project is archived and is in readonly mode.
Parameters to functional test requests should have proper values
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.
-
functional_test_parameters.patch
26.3 KB
-
Donald BallWe just ran afoul of this on our project; this patch would have helped us catch our mistake earlier.
-
Will BryantShouldn't the test wrappers just automatically convert everything to a string?
-
Michael Koziarski- 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.
-
blatyoI agree with Will. It should convert it to the string rather than telling the tester to.
-
David Siñuela (Siu)
- Tag set to parameters, params, testing
I'm having issues with this too, I think everything should be converted to string/param automatically.
-
Xavier NoriaThat 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 MilliganI 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 => falseThis 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
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
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 ClarkWhat if, instead of breaking the tests, it just put a warning when a parameter wasn't a string?
-
Joe Van DykI agree, a warning would be nice.
-
Michael Koziarski
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
I've created a new patch with warnings and options here: https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets...