This project is archived and is in readonly mode.

#2984 ✓resolved
Adam Milligan

Warn when parameters to functional test requests have improper values

Reported by Adam Milligan | August 2nd, 2009 @ 07:52 AM | in 3.0.2

This is a replacement for the ticket here.

In that previous ticket I made the argument that functional tests shouldn't allow passing parameters with values that wouldn't come from an HTTP request; the patch I provided threw exceptions in such cases. This patch defaults to displaying a warning message for parameters with improper values. I also added support for modifying this behavior via class methods on ActionController::TestCase. Specifically, if you want exceptions then you can use ActionController::TestCase.treat_parameter_type_warnings_as_errors. If you choose to live your life in fear you can use ActionController::TestCase.ignore_parameter_type_warnings.

I removed changes I made in my previous patch to fix tests that were passing non-string values. Therefore, this patch causes a fair number of warning messages (the default) in the ActionPack test suite. I'll leave it up to general opinion whether to leave these warnings, silence them, or fix them.

Comments and changes to this ticket

  • Adam Milligan
  • Will Bryant

    Will Bryant August 2nd, 2009 @ 12:01 PM

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

  • Adam Milligan

    Adam Milligan August 2nd, 2009 @ 04:55 PM

    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.

  • Zach Brock

    Zach Brock August 2nd, 2009 @ 08:04 PM

    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.

  • Michael Koziarski

    Michael Koziarski August 3rd, 2009 @ 12:33 AM

    • State changed from “new” to “incomplete”

    I'm with will, just silently .to_s/to_param the integers.

    The vast bulk of tests just pass the id in those places and they'll work fine. Users overloading to_param are in the minority and we shouldn't spam everyone else just to satisfy them.

    We should gently guide people in the right direction, not lecture or abuse them.

  • Neeraj

    Neeraj August 3rd, 2009 @ 02:40 AM

    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.

  • Michael Koziarski

    Michael Koziarski August 3rd, 2009 @ 02:42 AM

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

  • Yehuda Katz (wycats)

    Yehuda Katz (wycats) August 4th, 2009 @ 08:33 AM

    • Milestone cleared.

    @koz I agree that we shouldn't just break people's tests, but it does seem like a good idea to have a mode that will alert people to tests that do not reflect reality. I know I'd personally like such a mode. Thoughts?

  • Will Bryant

    Will Bryant August 4th, 2009 @ 11:46 AM

    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?
  • Yehuda Katz (wycats)

    Yehuda Katz (wycats) August 4th, 2009 @ 05:33 PM

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

  • Michael Koziarski

    Michael Koziarski August 4th, 2009 @ 10:40 PM

    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.

  • Will Bryant

    Will Bryant August 5th, 2009 @ 12:47 AM

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

  • Adam Milligan

    Adam Milligan August 5th, 2009 @ 01:07 AM

    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.

  • Will Bryant

    Will Bryant August 5th, 2009 @ 01:36 AM

    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.

  • Michael Koziarski

    Michael Koziarski August 5th, 2009 @ 03:05 AM

    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

  • Mikel Lindsaar

    Mikel Lindsaar January 24th, 2010 @ 10:35 PM

    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

  • Dan Pickett

    Dan Pickett May 9th, 2010 @ 06:29 PM

    • Tag changed from action_controller, parameters, params, patch, tested, testing to action_controller, bugmash, parameters, params, patch, tested, testing
    • Assigned user set to “Ryan Bigg”

    This is fairly stale - assigning to Ryan for his review. Ryan, please remove the bugmash tag if you find this no longer relevant. Thanks!

  • Ryan Bigg

    Ryan Bigg May 9th, 2010 @ 10:15 PM

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

  • Rizwan Reza

    Rizwan Reza May 16th, 2010 @ 03:48 AM

    • Tag changed from action_controller, bugmash, parameters, params, patch, tested, testing to action_controller, parameters, params, patch, tested, testing
    • State changed from “incomplete” to “resolved”

    Please reopen if needed.

  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:01 PM

    • Milestone set to 3.0.2
    • Importance changed from “” to “”

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