Abuse of method_missing?

Am I the only one who thinks the following DSL-ey trickery is an abuse of method_missing?

Here's the creation of some named routes.

ActionController::Routing::Routes.draw do |map|
  map.home '', :controller => 'main', :action => 'start'
  map.user_page 'users/:user', :controller => 'users', :action => 'show'
end

You call arbitrary methods on the map object, and that creates a route whose name is the method you called.

Here's the declaration of an ActiveRecord model's attributes using Hobo's new migration-generating style.

class User < ActiveRecord::Base
  fields do
    name :string, :null => false
    email :string
    about :text, :default => "No description available"
  end
end

I haven't looked into the code, but I assume the block is instance_eval'ed against some object whose method_missing builds up attribute meta-data where the name of the missing method becomes the attribute name.

Introducing new symbols into your system by invoking them as methods on bizarre (sometimes hidden) objects strikes me as a nearly useless twisting of Ruby's flexibility. If you're working with something that's purely DSL-ish, that's one thing, but if we're talking about a tiny little internal DSL embedded in otherwise idiomatic Ruby code, and, needless to say, being edited by Ruby developers, this just seems to introduce confusion.

Of the two uses, I actually prefer Hobo's because it goes farther than Rails' away from idiomatic Ruby and towards a DSL. Since I actually see that I'm sending messages to map when creating a named route, it frustrates me that this object exposes its functionality through method_missing, and I'm therefore unable to look up the API reference in the normal way. What is that thing? Does it have any methods that might conflict with my route names? We know about connect. Hopefully that's the only one.[1]

Do you think this sort of use of method_missing is advisable? How have you used and abused it?

[1] Actually, a look at routing.rb shows that ActionController::Routing::Routes.draw yields an ActionController::Routing::RouteSet::Mapper, which also (in the neighborhood of line 1000[2]) defines named_route: not a likely name conflict, but arguably a clearer way to define your routes.

ActionController::Routing::Routes.draw do |map|
  map.named_route :home, '', :controller => 'main', :action => 'start'
  map.named_route :user_page, 'users/:user', :controller => 'users', :action => 'show'
end

[2] Yeah, line one-thousand. The Rails team are trained professionals: please don't try that at home.

Towards More Readable Tests

Making code readable is probably my number two priority after making it function correctly. I want every developer on the team to be able to look at code written by anyone else on the team and understand its intent. Of course that goes for the tests too.

We've had a few new developers join the team recently, and I've heard more than one of them say "Huh, I've never seen that before" about a pattern that shows up in some of our unit tests. It's something I've done and liked, but on further reflection, I'm starting to lean away from it.

Here's a test that could be clearer. (Let's take the interaction being tested as a given and focus on how the tests read. (Let's also not assume I have any interest in fishing. I don't know how I landed on this example.) The mocking library is Mocha.)

test "catches, kills, and bags tasty fish" do
  river = mock
  fish = mock
  river.expects(:give_it_up!).returns(fish)
  fish.expects(:tasty?).returns(true)
  fish.expects(:die!)
  bag = mock
  bag.expects(:put).with(fish)
  
  fisher = Fisher.new(river, bag)
  fisher.fish
end

Here's what makes it hard to read:

  river = mock                              # setup
  fish = mock                               # setup
  river.expects(:give_it_up!).returns(fish) # expectations
  fish.expects(:tasty?).returns(true)       # expectations
  fish.expects(:die!)                       # expectations
  bag = mock                                # setup
  bag.expects(:put).with(fish)              # expectations

As you read that, you have to change gears back and forth as you read lines that create testing fixtures interspersed with lines that specify the interaction we're working on. No good.

After reading a few tests like that (and tests with the same problem magnified), lines like these

  river = mock
  fish = mock
  # ... snip
  bag = mock

start to feel like pure noise. I suppose that was my mindset when I started changing those sorts of tests to look like the following.

test "catches, kills, and bags tasty fish" do
  (river = mock).expects(:give_it_up!).returns(fish = mock)
  fish.expects(:tasty?).returns(true)
  fish.expects(:die!)
  (bag = mock).expects(:put).with(fish)
  
  fisher = Fisher.new(river, bag)
  fisher.fish
end

Here, the creation of mocks is all inlined so that you can read straight through the interaction. The assignment of local variables embedded at the first occurence of each item in the interaction (thanks to Ruby's lack of variable declaration) isn't too distracting once you're accustomed to seeing it. Each test takes up less screen real estate than before, and the world's all around better than it was.

Or at least that's how I felt based on the before and after states of some pretty long tests I had to work with.

But here's something that's no better than before (probably worse): look at the test above and tell me which collaborators come into play when a Fisher catches a tasty fish, as opposed to the following, in which the fish gets thrown back.

test "catches and releases non-tasty fish" do
  (river = mock).expects(:give_it_up!).returns(fish = mock)
  fish.expects(:tasty?).returns(false)
  river.expects(:put).with(fish)
  
  fisher = Fisher.new(river, :ignored_bag)
  fisher.fish
end

It requires some pretty annoying code scanning when mock creation is embedded at various funny spots along the way. Those lines that felt like noise when they interrupted the specification of the interaction suddenly seem a lot more useful.

Having reconsidered, I now think the clearest presentation is closer to the first version, but different in a key way.

test "catches and releases non-tasty fish" do
  river = mock
  fish = mock
  
  river.expects(:give_it_up!).returns(fish)
  fish.expects(:tasty?).returns(false)
  river.expects(:put).with(fish)
  
  fisher = Fisher.new(river, :ignored_bag)
  fisher.fish
end

test "catches, kills, and bags tasty fish" do
  river = mock
  fish = mock
  bag = mock
  
  river.expects(:give_it_up!).returns(fish)
  fish.expects(:tasty?).returns(true)
  fish.expects(:die!)
  bag.expects(:put).with(fish)
  
  fisher = Fisher.new(river, bag)
  fisher.fish
end

Like the list of characters at the beginning of a script, these tests tell the reader who's involved in the interaction before jumping into the story, which makes the story much easier to follow. The tests take up more lines, but are easier for the reader to comprehend, particularly one who isn't interested in all the details. (Imagine looking through your codebase before a big refactoring wondering "how often do we use that FishingBag I want to change?" If collaborators are called out, you can quickly focus on the relevant interactions and read just those in detail.)

In the case of the longer tests that pushed me towards the inline style, the list of players would have been quite a bit uglier than the one above. That shouldn't have discouraged me from keeping it separated from the rest of the test: it should have told me that the object I was trying to test had too much on its plate. When something like that happens, hopefully I can figure out a design that won't require more players than I can easily keep track of all at once. If not, at least the next developer who comes along will have a clean tally of all the collaborators and a fighting chance of improving the design herself.

Good luck!

Moving to RSpec

RSpec's been on my list of tools to adopt for a while now. My current project is unlikely to migrate from Test::Unit, and there's no plan for me to migrate off that project, so I decided to switch Nestegg over to use it. It's not the sort of hard-core dive-in that's going to force me to get fluent, but it's a nice first baby step. (I'm mean really baby: the whole gem is only one module.)

Here's what the migration looked like.

I started by running newgem -t rspec fakegem to get a template for my spec directory and Rakefile, then pulled the spec-related stuff that generated into nestegg.

The spec folder had

• spec_helper.rb, which just does require 'spec',
• spec.opts, which just hash --colour, and
• fakegem_spec.rb, which is a do-nothing spec.

The Rakefile had this up top

begin
  require 'spec/rake/spectask'
rescue LoadError
  puts 'To use rspec for testing you must install rspec gem:'
  puts '$ sudo gem install rspec'
  exit
end

and this down below

desc "Run the specs under spec"
Spec::Rake::SpecTask.new do |t|
  t.spec_opts = ['--options', "spec/spec.opts"]
  t.spec_files = FileList['spec/**/*_spec.rb']
end

desc "Default task is to run specs"
task :default => :spec

After a sudo gem install rspec, I was up and running. The default rake target ran my Test::Unit suite, then the do-nothing spec. Sweet.

I moved the do-nothing spec to nestegg/nesting_exception_spec.rb and copied each of my test names over (which was especially effortless since my Test::Unit suite was using RSpec-ish test declarations). I also remembered reading that you could pass a class to the describe method, so I landed with this

describe Nestegg::NestingException do
  
  it "includes cause in backtrace" do
    violated "Be sure to write your specs"
  end
  
  it "includes cause's backtrace after cause in backtrace" do
    violated "Be sure to write your specs"
  end
  
  it "removes duplicated backtrace elements from nesting exception" do
    violated "Be sure to write your specs"
  end
  
  it "defaults cause to current raised exception ($!)" do
    violated "Be sure to write your specs"
  end
  
end

Then I started at the top and copied the body of the first test into the spec. I'd anticipated some pain related to the helper methods that lived in my test, but those turned out to be completely portable. The only thing I had to modify in each test was the assertion. With special thanks to the very helpful Test::Unit Cheat Sheet, here are the before-and-afters.

• assert_true e.backtrace.include?("cause: StandardError: #{cause.message}")

  became

  e.backtrace.should include("cause: StandardError: #{cause.message}")

• assert_equal ["cause: StandardError: #{cause.message}", "line_one", "line_two"], e.backtrace[-3..-1]

  became

  e.backtrace[-3..-1].should == ["cause: StandardError: #{cause.message}", "line_one", "line_two"]

• assert_match(/#{__FILE__}:\d+:in `test_.+'$/, e.backtrace[0])
  assert_equal "cause: StandardError: msg", e.backtrace[1]

  became

  e.backtrace[0..1].should == ["#{__FILE__}:#{line}", "cause: StandardError: msg"]

  Note that on that one I'd done a Regexp match in the Test::Unit version, because I didn't want to put the test method name in the content of the test. Since RSpec apparently doesn't create methods out of examples, the backtrace line I'm interested in is free of any method name. To enable a simple equality check, I saved the line number on which the exception was raised in a temporary variable.

• assert_equal expected_cause, raised_error.cause

  became

  raised_error.cause.should == expected_cause

See here for the whole shebang. The original test is here. The two files are incredibly similar, no?

Just like that, Nestegg was converted. I was a little sad I hadn't used mocha, since that could have meant switching to RSpec's mocking library. Oh well. That'll give me something to look forward to when I migrate Handoff.