Topic: DRYing up view tests...

So if you've been following my posts, you've seen that I've been progressing from model->controller->view tests.  Using a slew of custom assertions for model testing and the Test::Rails package for divorced controller and view tests.

I feel like I've been able to keep both model and controller tests rather clean and DRY through aggressive refactoring/custom assertions.  However view tests seem to have some un-attractive repetition which so far appears relatively hard to clean up.  And after staring at it some more, I'm realizing some of the problems are shared with the controller tests.

The easiest problem for me to explain, and probably easiest to fix:

In Test::Rail's view tests, many of the assertions take the form of

  assert_something form_url, other_parameter(s)

Where the form_url is typically '/<controller>/<action>'.

So a relatively normal test looks like

  def test_edit_new
    assigns[:model_object]=ModelObject.new
    render
    form_url = '/models/create'
    assert_post_form form_url
    assert_field form_url, :text, :model_object, :field_name
    assert_field form_urm, :text, :model_object, :other_field
    assert_submit form_url, 'Submit'
  end

I find the repetition of the 'form_url' everywhere very ugly.  I'm also not too keen on the assert_post, assert_submit that tend to "wrap" every form test. 

I think the solution will be to create an custom assertion that would be used as follows (Handwaving/prototype code follows)

  def test_edit_new
    assigns[:model_object]=ModelObject.new
    render
    assert_form '/models/create', :submit=>'Submit' {|f|
      f.field :text, :model_object, :field_name
      f.field :text, :model_object, :other_field
    }
  end

With the assert_submit, and assert_post_form handled internally to the assert_form, etc.  This looks like the a standard rails idion too, see the create table migration syntax or some of the form builders, I beleive.  Does this look like the right track to you?  (Of course in addition to f.field, I'll need to delegate most of the current ViewTestCase assertions, as well as provide for the one-two that it seems to miss (textareas).

I'm also not sure if I should push :model_object up into the hashlist of the assert_form to remove it from the field definitions.  Doing so would make the f.field call's nicer, but would limit the ability to work with a single form that has multiple model_objects attached....


I think I'll stop here -- the other place I'm feeling non-DRY is extremely hard to explain and until I understand it better it probably isn't worth trying to explain it to others...

My RoR journey  -- thoughts on learning RoR and lessons learned in applying TDD and agile practices.

Re: DRYing up view tests...

Looks like a decent refactoring. Just a couple other ideas:

assert_form '/models/create', :submit=>'Submit', :model => :model_object do |f|
  f.model_field :text, :field_name # Will call f.field with the :model given above
  f.model_field :text, :a_field, :another_text_field # Assert multiple fields of the same type
end

Railscasts - Free Ruby on Rails Screencasts

Re: DRYing up view tests...

OK, here is some rough code that implements a portion of the above:

  def assert_form(form_url, model_obj, opts={})
    assert_post_form form_url
    @form_url = form_url
    @model_obj = model_obj
    yield self
    assert_submit form_url, (opts.has_key?(:submit) ? opts[:submit] : "Submit")
  end
  def model_field(type, obj_field)
    assert_field @form_url, type, @model_obj, obj_field
  end

Used as:
  assert_form '/users/login', :user, :submit=>'Log-In' do |f|
    f.model_field :text, :username
    f.model_field :password, :password
  end

Relative to ryanb's post, the name of the model is currently a "full" parameter instead of part of the optional hash...  Not sure where I like it better.  And of course I still need to add a ton of other delegates.

However the ugliest part of the prototype is the binding the assert_form parameters to instance variables and yielding self.  I've been trying to dig through the rails source to see the idomatic way of handling this (create table migration, etc)...  I'm assuming I'll probably want to create a new class, that mixes-in the assertions I want and that takes these parameters in its constructor.  Does this sound ruby/rail-ish?

My RoR journey  -- thoughts on learning RoR and lessons learned in applying TDD and agile practices.

Re: DRYing up view tests...

NielsenE wrote:

I'm assuming I'll probably want to create a new class, that mixes-in the assertions I want and that takes these parameters in its constructor.  Does this sound ruby/rail-ish?

Yeah, I think that's a good way to do it, although I don't know if it's possible to mix-in the assertions. You could just pass the test case to the form tester so it has access to the assertions. Something like this perhaps (untested).

class FormTester
  def initialize(test_case, action, model)
    @test_case = test_case
    @action = action
    @model = model
  end

  def field(type, *columns)
    columns.each do |column|
      @test_case.assert_field @action, type, @model, column
    end
  end
end

class Test::Rails::ViewTestCase
  def assert_form(action, model, options = {})
    assert_post_form action
    yield FormTester.new(self, action, model)
    assert_submit action, (options.has_key?(:submit) ? options[:submit] : "Submit")
  end
end


Anyway, you get the idea. smile

Railscasts - Free Ruby on Rails Screencasts

Re: DRYing up view tests...

That worked and feels clean enough to me to go on, so I'm pulling the code out from the single test case I've been testing it in and moving it to the test_helper series of files.  Thanks.

Now that it looks cleaner I'll try to explain the other (minimal) repetition that is bothering me more than it probably should.  I'm probably being too picky here....

Given a test case like:

class UsersViewTest < Test::Rails::ViewTestCase
  def setup
    super
    assigns[:user]=User.new
  end
  def test_sign_up
    render
    assert_form '/users/sign_up', :user, :submit=>'Sign-Up' do |f|
      f.model_field :text, :username
      f.model_field :text, :email
      f.model_field :password, :password
      f.model_field :password, :password_confirmation
    end
  end
  def test_login
    render
    assert_form '/users/login', :user, :submit=>'Log-In' do |f|
      f.model_field :text, :username
      f.model_field :password, :password
    end
  end
end

You can see two remaining duplication smells, one of which I think is benign, the other less so.  The acceptable duplication, to me, is the inclusion of the username and password in both tests.  While it possible to do some something like
  def test_sign_up
    render
    assert_form '/users/sign_up', :user, :submit=>'Sign-Up' do |f|
      shared_fields f
      f.model_field :text, :email
      f.model_field :password, :password_confirmation
    end
  end
  def test_login
    render
    assert_form '/users/login', :user, :submit=>'Log-In' do |f|
      shared_fields f
    end
  end
  protected
  def shared_fields(f)
    f.model_field :text, :username
    f.model_field :password, :password
  end

I think that hides the details of the form in a non-useful way.  I probably would use this technique if I was testing dynamic (AJAX) forms where there are some "constant" fields and others that come and go based on selections -- in that case you want the test to focus on the changes/deltas.  In this case though, I don't think people view the login form and the account creation form as derivatives of each other.  Your thoughts?

The duplication that does bother me, however is that darn 'render' that kicks off both test methods and can't be moved to the setup (as setup doesn't have access to the test method name and therefore can't do its magic method_name->view file lookup).  Due to testcase (fixture in most xUnit but not Rail's fixtures) refactoring, most/all of my assigns happen in the setup and thus there's 'nothing' to customize aside from the view to request and that could be handled by magic.  (A similar arguement exists for controller tests with the initial get/post.  However in that case there is the decision of which get/post is needed as well as the requirement to pass arguments to the controller which would limit the refactorings, even if controller tests had similar "magic" as view tests.)

I'd love to get rid of the requirement to call render (while at the same time reserving the ability to call render with parameters when needed).  The only approach I've thought of so far would be to hack into ViewTestCase (and possibly into the assertions in action_controller/assertions.rb and the code in test_process.rb) to do a lazy-load (if needed) for @response.  Ie,

  if @response.body.nil? render

I might be able to do this with a simple method rename/chain.  Of course it'll need to tweak render's rules to look up the callstack (if possible?) to find the first method with a method_name like test_* as opposed to the immediate caller.

Thus if no render was explicitly called a parameter-less render would be inserted before the first assertion; otherwise whatever explicit render was called would be respected.

Is there a simpler way?  Is this even worthwhile or am I chasing shadows here?

My RoR journey  -- thoughts on learning RoR and lessons learned in applying TDD and agile practices.

Re: DRYing up view tests...

The ultimate goal of refactoring IMO is to make the code more readable and maintainable. It is always good to ask yourself before each refactoring, "how does this improve the code?".

I agree with your thoughts on the first refactoring about the username and password. You are able to remove the duplication, but it doesn't become more readable or maintainable - maybe less so.

The second refactoring, removing the 'render' call, I think lessens both readability and maintainability. It would be confusing to me if some methods have a "render" call and some don't. It is not obvious that "render" gets called automatically before the first assertion if it has not been called already. The refactoring seems a little too complicated as well.

That said, it is definitely good to question every piece of duplication and any other code smells, but, not all smells are bad. smile

Railscasts - Free Ruby on Rails Screencasts

Re: DRYing up view tests...

ryanb wrote:

The second refactoring, removing the 'render' call, I think lessens both readability and maintainability. It would be confusing to me if some methods have a "render" call and some don't. It is not obvious that "render" gets called automatically before the first assertion if it has not been called already. The refactoring seems a little too complicated as well.

That said, it is definitely good to question every piece of duplication and any other code smells, but, not all smells are bad. smile

Actually this kinda brings me back to an earlier issue I've had with Test::Rails -- I'm not too happy with how view tests use the method_name magic and controller test don't.  I'd expect them to both work the same, ie render <template_name> and get/post <action_name> or "bare" render and get/post.  The magic view template method_name look up feels like my proposed second refactoring -- it removed the duplicated (method_name and template) but at the expense of lessened clarity.  Of course I guess I can choose not to use the magic and use explicit templates in my calls to render.... and thereby be less contrained on test method naming.

My RoR journey  -- thoughts on learning RoR and lessons learned in applying TDD and agile practices.

Re: DRYing up view tests...

Well, the Test::Rails naming magic does seem to fit the convention over configuration pattern you see in Rails. In a sense, your "render" refactoring just pushes that a little further. The real question is, how far is too far? It is a balancing act between clarity and convenience.

You are right though, it is confusing that the controller "render" method behaves differently.

Railscasts - Free Ruby on Rails Screencasts