This is Anti-pattern—thoughts on programming and whatnot by Brandon Weiss.

Testing ActiveRecord Transactions the Right Way

May 20, 2014

Database transactions are a way of making multiple queries to a database such that all of them must succeed or none of them will. This helps prevent data from getting into an unexpected state. Take for example a user signing up.

# app/controllers/users_controller.rb
def create
  # ...

  @user.save!
  @user.subscribe!

  # ...
end

It’s possible that the second query might fail, which would leave the user in a bad state: the user will be created but won’t be charged. Transactions to the rescue!

# app/controllers/users_controller.rb
def create
  # ...

  @user.transaction do
    @user.save!
    @user.subscribe!
  end

  # ...
end

Now, if subscribe! raises an exception the transaction will fail and the database will roll back. It will be like nothing ever happened.

This is all pretty straight-forward, but what can be a little confusing is how to test this. Let’s walk through it.

Transactions only roll back when an exception is raised, so we need to force subscribe! to raise an exception. One way to do this would be with your mocking and stubbing framework of choice.

# test/controllers/users_controller_test.rb
it "must not create a user if billing fails" do
  User.any_instance.stubs(:subscribe!).raises(BillingError)

  post :create

  User.count.must_equal 0
end

But you’ll find that doesn’t quite work. The transaction will fail and correctly roll back the database but the uncaught exception continues to bubble up and fails our test before User.count can be tested.

In order to make them pass we’ll need to catch the exception somehow. One way would be to test the exception.

lambda {
  post :create
}.must_raise BillingError

Another approach would be to just rescue the exception.

begin
  post :create
rescue
end

Either of these will work, and the test will pass, but both of these solutions are janky. They’re not quite right. Asserting the exception isn’t right because we don’t technically care that there was one. Rescuing the exception isn’t right because we’re not actually doing anything in response to the rescue. Both are just hacks we’re using in order to swallow the exception so we can test what we’re actually trying to test: the transaction.

There is a more idiomatic way to do this, and that’s with the suppress kernel method added by ActiveSupport, which exists for this very purpose—suppressing exceptions.

suppress(BillingError) do
  post :create
end

It’s entirely a semantic difference, but semantics exist for a reason. In the future, when this test is read or changed, the semantics will convey that the exception is irrelevant. The use of suppress reinforces the intent of the test.

Shared Examples with Minitest

May 06, 2014

I rarely if ever apply DRY principles to the tests I write. What might improve application code does not necessarily improve test code. Tests should be as verbose and repetitive as they need to be. If you can’t look at an individual test and understand exactly what’s going on without looking through several before blocks or nested contexts, then something is probably wrong. It’s better to be a bit more verbose and repeat yourself a little than make your tests unreadable.

That said, sometimes you need to repeat nearly the exact same test over and over again. A good example would be for a before_action in a controller that ensures a user is authenticated.

# app/controllers/application_controller.rb
class ApplicationController < ActionController::Base

  def current_user
    @current_user ||= User.find_by(id: session[:user_id])
  end

  def authenticate
    redirect_to sign_in_path unless current_user
  end

end
# app/controllers/monitors_controller.rb
class MonitorsController < ApplicationController

  before_action :authenticate

  def index
    # ...
  end

end

Every action that the before_action runs before needs to be tested.

# test/controllers/monitors_controller_test.rb
describe MonitorsController do

  describe "index" do

    it "requires authentication" do
      session[:user_id] = nil
      get :index
      assert_redirected_to sign_in_path
    end

  end

end

You’d have to repeat almost this exact same test for every action, the only thing that would change is the controller and action being requested. This is a perfect candidate for a shared test.

If you’re familiar with RSpec you might know that it has a feature for this called shared examples. Minitest has no corresponding equivalent, and rightly so; sharing tests is an edge case. This pretty well highlights the fundamental difference between RSpec and Minitest: RSpec is everything you might ever need; Minitest is just what you’ll probably need. Since Minitest has no built-in facility for sharing tests, you can just implement your own.1

# test/test_helper.rb
class Minitest::Spec

  def self.it_requires_authentication(&block)
    it "requires authentication" do
      session[:user_id] = nil
      instance_exec(&block)
      assert_redirected_to sign_in_path
    end
  end

end
# test/controllers/monitors_controller_test.rb
describe MonitorsController do

  describe "index" do

    it_requires_authentication do
      get :index
    end

  end

end

There’s no magic—it’s just plain Ruby.


  1. I’m defining the class method on Minitest::Spec but where you define the class method depends on what framework you’re using and how you integrated Minitest into it. For Rails, the class you probably need to define it on is ActiveSupport::TestCase

The Culture That Creates the Problem Is Never the Culture That Fixes the Problem

April 29, 2014

In software, there is no such thing as “done.” Done is a choice programmers make between quick but dirty and slow but clean. Done is just a balance held in tension by hundreds of choices made every day. Every decision a programmer makes incurs a cost called “technical debt.” It is an apt analogy. The more you spend up front, the less you’ll have to pay later. Whatever you don’t pay now, you’ll eventually have to pay back, plus interest.

Despite the tradeoff being so obvious, people are incredibly short-sighted. As such, the vast majority of codebases are a giant mess; the accumulation of months—if not years—of technical debt.

Programmers almost always opt to ship faster and incur the technical debt. Then they leave apologetic comments in their code or commit messages attempting to justify their decision to others. They are so concerned that they’ll be perceived as inept that they feel compelled to explain that they know they’re shipping low quality work but that it’s just temporary. Months and often years later you can still find the fossilized remains of these comments buried in codebases. The irony is nearly palpable.

What’s fascinating is that programmers genuinely intend to pay back the technical debt. They whole-heartedly believe that they will have an opportunity to fix the mess later; that this one time is the exception, not the rule. Against all logic and evidence to the contrary, they always think they’ll find time to pay back the debt they’ve accrued. I’m amazed and disappointed by the conviction with which programmers hold on to this fallacy.

For the most part, no one goes back and fixes the problems they create; they’re forgotten as quickly as they’re created. The culture that creates the problem is never the culture that fixes the problem. When people say, “we’ll fix it later” what they’re really saying is, “I don’t care enough to fix it now.” If you don’t care enough to fix it now, you never will; later is always later.

“You are exactly what you practice. You are only what you’re doing right now.” —Leon Gersing

The iconic motto of programming culture is “move fast and break things,” but that comes at a price: a lot of broken things. Be honest with yourself. If you don’t care enough to do it the right way, right now, when will you?

Water Your Controllers, They Are Too DRY

April 21, 2014

When you start out, your Rails controllers usually look something like this.

class UsersController < ApplicationController

  def index
    @users = User.all
    # ...
  end

  def show
    @user = User.find(params[:id])
    # ...
  end

  def new
    @user = User.new
    # ...
  end

  def create
    @user = User.create(params[:user])
    # ...
  end

  def edit
    @user = User.find(params[:id])
    # ...
  end

  def update
    @user = User.find(params[:id])
    # ...
  end

  def destroy
    @user = User.find(params[:id])
    # ...
  end

end

Everything looks good. This is how a controller should look. But… one of the first things you learn as a programmer is Don’t Repeat Yourself (DRY). There’s some repetition here, so you clean it up a bit. You add some more complexity, and DRY it up again. Rinse and repeat a few more times, then fast forward a few months and your controller probably looks like this.

class UsersController < ApplicationController

  before_action :load_user,                    only: [:show, :edit, :update, :destroy]
  before_action :ensure_admin,                 only: [:index]
  before_action :ensure_current_user_or_admin, only: [:edit, :update, :destroy]

  def index
    @users = User.all
    # ...
  end

  def show
    # ...
  end

  def new
    @user = User.new
    # ...
  end

  def create
    @user = User.create(params[:user])
    # ...
  end

  def edit
    # ...
  end

  def update
    # ...
  end

  def destroy
    # ...
  end

private

  def load_user
    @user = User.find(params[:id)
  end

  def ensure_admin
    redirect_to root_url unless current_user.admin?
  end

  def ensure_current_user_or_admin
    redirect_to root_url unless current_user == @user || current_user.admin?
  end

end

The common logic from the beginning of various actions has been moved into private methods on the controller that are triggered before the actions run. All that remains in each action is the unique logic. I’ve seen this done in virtually every Rails app I’ve worked on and it drives me absolutely nuts.

It might look innocuous enough right now, but that’s only because this is an example with most of the code omitted to emphasize the relevant parts. But imagine if these actions were all ten lines long or more, and there were ten or twenty more actions.

Later, Future You (or even worse, someone else on your team) has to update an action in the middle of the file. You open it up, go to the action, and pause. The action uses instance variables that aren’t defined in the action. You scroll all the way to the top to see what is happening in the before_action. There are five or more callbacks, each with ten or more action names, with a mix of only and except. There are probably so many they wrap or scroll offscreen. Each callback references a private method at the bottom of the file, so you jump down to the bottom of the controller to follow along. You repeat this several times. Finally, you go back to the action to put it all together. At this point you’ve probably lost your train of thought and need to start over. Best-case scenario you have to make changes holding all of that logic in your head.

Clearly, this is not an improvement. It is way, way worse than repeating a few lines at the beginning of each action. You have sacrificed readability, comprehension, and maintainability, which is horribly ironic, because that is likely what you thought you were achieving when you abstracted the logic out in the first place.

The reason this pattern is so common in Rails controllers even though it so obviously does not work is that DRY is generally one of the first principles taught to new programmers. It’s ostensibly easy to understand and apply, but it’s taught all wrong and thus usually misunderstood. DRY probably should have been DRYURYIBTNRY—Don’t Repeat Yourself Unless Repeating Yourself Is Better Than Not Repeating Yourself.

DRY is just a general guideline. It’s a simple acronym to help new programmers start thinking about the problems caused by duplication. It only makes sense to use DRY if it will make your code more readable, comprehensible, and maintainable. DRY is not the ruthless removal of all repetitive code; achieving DRY for the sake of itself does nothing. Less is not always more, sometimes more can be considerably less.

Strip Leading Whitespace from Heredocs in Ruby

February 11, 2014

A “heredoc” in programming is short for “here document”. If you’re not familiar with the concept I encourage you to read up on it. It’s basically a way to declare a file or long string inline. Formatting is preserved and quotes are ignored, which means strings with quotes in them don’t have to be escaped.

For example, if you needed a string with some spacing, indentation, and quotes you could do something like this:

"<pre>\n  <code>puts \"foobar\"</code>\n<pre>"

But reading and writing that hurts my brain. With a heredoc you could just do this1:

<<-EOS
<pre>
  <code>puts "foobar"</code>
</pre>
EOS

There are all sorts of uses for heredocs, but I find them especially useful in tests when I need to compare some kind of long, formatted output.

Case in point, we just switched to Redcarpet to process the very Markdown with which we write this blog. But before I did I wrote several tests, one of which ensures that Redcarpet renders fenced code blocks.

describe "render_html_from_markdown" do

  it "renders fenced code blocks" do
    markdown = "```\ndef foobar\n  true\nend\n```"
    output   = "<pre><code>def foobar\n  true\nend\n</code></pre>\n"

    render_html_from_markdown(markdown).must_equal(output)
  end

end

This works, but heredocs can dramatically improve the readability of this test.

describe "render_html_from_markdown" do

  it "renders fenced code blocks" do
    markdown = <<-EOS
      ```
      def foobar
        true
      end
      ```
    EOS

    render_html_from_markdown(markdown).must_equal <<-EOS
      <pre><code>def foobar
        true
      end
      </code></pre>
    EOS
  end

end

The HTML output isn’t quite formatted how I would have done it by hand, but the important thing is that the test is significantly more readable.

There is, however, one problem—the test actually fails now. It fails because I’ve indented the heredocs for readability, like you would normally with any code, but the leading whitespace of the indentation is interpreted as formatting for the heredoc, unfortunately causing the strings to mismatch.

If you’re using Rails (or ActiveSupport) there’s a strip_heredoc helper method that solves this very problem. It looks for the least indented line in the string and removes that amount of leading whitespace from each line2.

describe "render_html_from_markdown" do

  it "renders fenced code blocks" do
    markdown = <<-EOS.strip_heredoc
      ```
      def foobar
        true
      end
      ```
    EOS

    render_html_from_markdown(markdown).must_equal <<-EOS.strip_heredoc
      <pre><code>def foobar
        true
      end
      </code></pre>
    EOS
  end

end

  1. The actual acronym or word you use to delimit the heredoc doesn’t really matter. The two most common are EOS (end of string) and EOF (end of file), but you can use anything you like (e.g., “SQL”). 

  2. The syntax is admittedly a little awkward but methods called on heredocs are called on the starting delimiter.