Warn when parameters to functional test requests have improper values

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

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

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.

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.

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.

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

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?

@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?

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.

@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?

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.

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.

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

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

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