How Ryan Davis Schooled Me

A few weeks ago, I submitted a pull request to the minitest project. Ryan Davis (a.k.a. zenspider) merged my pull request, but also cleaned up the tests I had written. As someone relatively new to Ruby, I found his cleanup very instructive, so I’d like to share it with you. I understood everything he did, but I didn’t think of doing it that way myself. Being a Smalltalker, I understand quite a bit of “the Ruby way”, but Ruby has a number of unique features as well. I feel like I’ve learned the language and its features pretty well, but I still have much to learn about how and when to use those features to full advantage.

In MiniTest::Unit, there is a hook message named record that is sent to record the results of each test case as it is run. Minitest itself doesn’t use this method, but subclasses in other projects do. In my case, I also use minitest-reporters to hook into the test runner in RubyMine, and it uses record.

I found that record wasn’t being sent when there was a failure or exception in a teardown method. When using a mocking framework, teardown is typically where mock verification is done, and I found that I wasn’t seeing failure messages.

record didn’t have any test coverage, so I first wrote some tests around the original behavior of the code, then changed a test to expose the broken behavior, and finally fixed the bug.

I followed a pattern I saw in another part of the test suite and made a new test suite:

class TestMiniTestUnitRecording < MetaMetaMetaTestCase
 
  def setup
    super
    @recorder = Class.new MiniTest::Unit do
      def record suite, method, assertions, time, error
        recording[method] = error
      end
 
      def recording
        @recording ||= {}
      end
    end.new
    MiniTest::Unit.runner = @recorder
  end
 
  # ...
end

In setup, I create a new, anonymous subclass of MiniTest::Unit that overrides record, storing the error into a hash that is keyed off of the method name. I provide a getter to return the hash for later use in test assertions. I tell minitest to use my new class as it’s test runner.

I also wrote a little helper method to run the test case and yield the recording to a block where assertions could be made:

# ...
 
  def with_recording
    with_output do
      @tu.run
    end
 
    yield @recorder.recording
  end
 
  # ...

@tu is defined in MetaMetaMetaTestCase and is the test suite to be run.

With that, I wrote a number of tests that looked like this:

# ...
 
  def test_record_failing
    Class.new MiniTest::Unit::TestCase do
      def test_failure
        assert false
      end
    end
 
    with_recording do |recording|
      assert_instance_of MiniTest::Assertion, recording["test_failure"]
    end
  end
 
  # ...

In each test, I define an anonymous subclass of TestCase with any test and/or teardown method needed for that test. Then, in a with_recording block, I make the necessary assertions.

Overall, I didn’t think these tests were too bad; a bit wordier than I liked, but they followed a pattern used elsewhere in the test suite.

Here’s what Ryan refactored to:

class TestMiniTestUnitRecording < MetaMetaMetaTestCase
 
  def assert_run_record(*expected, &block)
    def @tu.record suite, method, assertions, time, error
      recording[method] << error
    end
 
    def @tu.recording
      @recording ||= Hash.new { |h,k| h[k] = [] }
    end
 
    MiniTest::Unit.runner = @tu
 
    Class.new MiniTest::Unit::TestCase, &block
 
    with_output do
      @tu.run
    end
 
    recorded = @tu.recording.fetch("test_method").map(&:class)
 
    assert_equal expected, recorded
  end
 
  # ...
 
  def test_record_failing
    assert_run_record MiniTest::Assertion do
      def test_method
        assert false
      end
    end
  end
 
  # ...
 
end

I can see the pieces of what I’d written in his refactored version, but the actual tests themselves are so much cleaner. There is no extra noise in those tests.

As part of merging my pull request, he decided to change the semantics of record a bit (for the better), which is why he maps method names to arrays instead of to single instances, but that’s not relevant here.

Here are some of the things I find interesting about his solution:

• He rolled all of the common boilerplate code into an assertion method, so no setup is required.

• He defined the record and recording methods as per-instance methods on @tu rather than creating a new, anonymous subclass. Understanding what is going on here is one of the keys to understanding Ruby’s object model. This is one of the significant differences between Ruby and Smalltalk. Were I to do this again, I think this technique is a little deeper in the bag of tricks than I’d choose to go in this situation. I’d probably still create a new subclass here. Still, it’s an interesting technique and worth learning and thinking about.

• He defines the anonymous subclass of TestCase in the assertion method, using the block from the test to define the necessary test and teardown methods. This is brilliant, in my opinion. I’m not sure I ever would have thought of doing it this way, and yet seeing it, it makes perfect sense.

• He uses assert_equal on the class of the recorded exception, which is actually a more accurate test than the assert_instance_of that I’d used.

This was definitely educational for me, and I hope it is for you as well.

Thanks to Ryan Davis for the opportunity to learn from his code, for merging the pull request, and for giving me permission to blog about the experience.

Afterword

I had already scheduled this post for this week, and over the weekend saw a similar post by Clay Allsopp: How A Pull Request Rocked My World.

I’d never really thought about the educational value of pull requests before. Here are two examples of that, one from the submitter side, and one from the receiver side. Pull requests are a great way to learn about writing better code in a real-world context.