Stringify param values in controller tests. #1203

Merged
merged 2 commits into from May 28, 2011

Conversation

Projects
None yet
5 participants
Contributor

dchelimsky commented May 22, 2011

This reduces false positives that come from using ints in params in
tests, which do not get converted to strings in the tests. In
implementations going through rack, they do get converted to strings.

  • David Chelimsky and Sam Umbach
@dchelimsky dchelimsky Stringify param values in controller tests.
This reduces false positives that come from using ints in params in
tests, which do not get converted to strings in the tests. In
implementations going through rack, they do get converted to strings.

- David Chelimsky and Sam Umbach
9277e72
Owner

pixeltrix commented May 22, 2011

Wouldn't it be better to call to_param? To save rehashing old arguments look at these tickets:

https://rails.lighthouseapp.com/projects/8994/tickets/2984
https://rails.lighthouseapp.com/projects/8994/tickets/2970

Member

josevalim commented May 22, 2011

to_param would definitely be more appropriate as we would be able to pass models to get/post and friends. @dchelimsky, what do you think?

Contributor

dchelimsky commented May 22, 2011

to_param makes sense (I hadn't seen those tickets, sry). That said, I'm a bit confused. https://rails.lighthouseapp.com/projects/8994/tickets/2984 says it's resolved, but I don't see a patch. What happened?

Also - we need to exclude Rack::Test::UploadedFile from this as the to_param conversion does the wrong thing. Are there any other exclusions you can think of?

@dchelimsky dchelimsky use to_param (and change method to name accordingly)
- exclude Rack::Test::UploadedFile to pass existing tests. Are there any
  other types we're missing?
3f0c71c
Member

amatsuda commented May 28, 2011

+1 for to_paramification.

@josevalim josevalim added a commit that referenced this pull request May 28, 2011

@josevalim josevalim Merge pull request #1203 from dchelimsky/stringify-parameter-values-i…
…n-tests

Stringify param values in controller tests.
a46b03e

@josevalim josevalim merged commit a46b03e into rails:master May 28, 2011

Contributor

dchelimsky commented Jul 25, 2011

@josevalim - I see this is in master but not 3-1-stable. Is the intent to include this post 3.1?

Member

josevalim commented Jul 25, 2011

@dchelimsky we can include it on 3-1. Could you please send a pull request for 3-1-stable (I am without my mac lately)?

Contributor

dchelimsky commented Jul 25, 2011

@josevalim - I squashed the two commits into a single commit and cherry-picked it onto 3-1-stable in a separate pull request: #2253

Contributor

dchelimsky commented Mar 8, 2012

Per https://twitter.com/#!/nzkoz/status/177832158617018368, this breaks the ability to functional test json APIs (which support fixnums). Any thoughts on how to preserve the benefits from this change and also support json APIs? The first thing that comes to my mind is a declaration in the test like do_not_convert_params or some such. I'd expect the conversion to be an opt-out, but that's me :)

Member

NZKoz commented Mar 8, 2012

Well, this change breaks backwards compatibility and should only be applied when it's definitely safe, but that's me ;)

I think the simplest option is to only do this to_paramization when the format of the request is :html or nil, that at least lets people include :format=>:json and hopefully move on with their lives?

Member

NZKoz commented Mar 8, 2012

I'd suggest discussing in that issue rather than here as this one is closed and ancient

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment