Ruby: Array#chunk

Yesterday I was working on some code that needed to split an array into 3 different chunks. My pair, Philippe Hanrigou, wrote tests similar to the following.

unit_tests do
  test "chunk evenly" do
    assert_equal [[1], [2], [3]], [1, 2, 3].chunk(3)
  end

  test "divide evenly" do
    assert_equal [[1], [2], [3]], [1, 2, 3] / 3
  end
  
  test "chunk unevenly" do
    assert_equal [[1, 3], [2]], [1, 2, 3].chunk(2)
  end
  
  test "chunk creates empty array when there aren't enough elements in the array" do
    assert_equal [[1], [], []], [1].chunk(3)
  end
end

To make the above tests pass, I wrote the following code.

class Array
  def chunk(number_of_chunks)
    chunks = (1..number_of_chunks).collect { [] }
    while self.any?
      chunks.each do |a_chunk|
        a_chunk << self.shift if self.any?
      end
    end
    chunks
  end
  alias / chunk
end

The above code works, but I'm curious to see if there is a better solution. How would you make the tests pass?

Here's the code in it's entirety.

require 'rubygems'
require 'test/unit'
require 'dust'

class Array
  def chunk(number_of_chunks)
    chunks = (1..number_of_chunks).collect { [] }
    while self.any?
      chunks.each do |a_chunk|
        a_chunk << self.shift if self.any?
      end
    end
    chunks
  end
  alias / chunk
end

unit_tests do
  test "chunk evenly" do
    assert_equal [[1], [2], [3]], [1, 2, 3].chunk(3)
  end

  test "divide evenly" do
    assert_equal [[1], [2], [3]], [1, 2, 3] / 3
  end
  
  test "chunk unevenly" do
    assert_equal [[1, 3], [2]], [1, 2, 3].chunk(2)
  end
  
  test "chunk creates empty array when there aren't enough elements in the array" do
    assert_equal [[1], [], []], [1].chunk(3)
  end
end

Rails: Testing Controllers

Yesterday, Mike Clark posted a blog entry asking "How Would You Test This?" The topic of the entry is how to test controllers. Mike's article is good, and you should start there for context.

I think Mike has the Functional tests well covered in his entry. However, I've never been able to accept that Controllers can only be functionally tested. In my earlier Rails days I would probably have written something similar to the example below and considered it a Unit Test. (Sorry Mike, I prefer Mocha to Flexmock)

require File.dirname(__FILE__) + '/../test_helper'

class MenuItemsControllerTest < Test::Unit::TestCase
  
  def test_create_with_valid_menu_item
    controller = MenuItemsController.new
    controller.expects(:params).returns(:menu_item => {:title => 'Classic'})
    controller.expects(:flash).returns(flash_mock = mock)
    controller.expects(:menu_items_url).returns(:menu_items_url)
    controller.expects(:redirect_to).with(:menu_items_url)
    flash_mock.expects(:[]=).with(:notice, "MenuItem was successfully created.")
    controller.create
    assert_not_nil controller.instance_eval { @menu_item }
  end
  
  def test_create_with_invalid_menu_item
    controller = MenuItemsController.new
    controller.expects(:params).returns(:menu_item => {})
    controller.expects(:render).with(:action => :new)
    controller.create
    assert_not_nil controller.instance_eval { @menu_item }
  end

end

There's a few concerns with the above example. Probably the largest concern is that a lot of mocking almost always results in brittle tests. Another concern is that there's so much noise in the test that it's hard to determine what the intent of the test is. Furthermore, the test is clearly specifying not what should be done, but how it should be done.

On my current project, David Vollbracht created something called an IsolatedControllerTest. The IsolatedControllerTest class mocks the render method and does a few other things to give you the ability to test your controllers in isolation. While David had the right idea, it simply didn't catch on. We have a few IsolatedControllerTests; however, the majority of controller tests are all Functional.

As Mike points out, the current situation often leads to a lack of testing controllers. I'm no more okay with that option than Mike is, but we've had other more interesting problems to solve so I put the thought on a back burner. Luckily, Mike isn't willing to settle.

I think part of the problem is that controllers are not Good Citizens. Controllers violate the first rule of Good Citizenship. Upon creation (Controller.new), controllers are not in a valid state. Instead, controllers depend on being initialized within the framework and having their state set post construction time. That ends up being a problem for unit testing since it requires each test to set additional state on newly created controllers.

Another problem is that methods are marked as protected. For example the *_url and *_path methods are all only accessible from within a controller. I'm sure this was done with the best intentions; however, I subscribe to the philosophy that making something public to increase testability is a good idea.

One of the larger shortcomings that controllers have is that they don't behave like POROs (Plain Old Ruby Objects). Ideally, testing controllers should be as easy as writing the following tests.

require File.dirname(__FILE__) + '/../test_helper'

class MenuItemsControllerTest < Test::Unit::TestCase
  
  def test_redirection_to_menu_items_on_success
    controller = MenuItemsController.new(:params => { :menu_item => {:title => 'Classic'} })
    controller.expects(:redirect_to).with(controller.menu_items_url)
    controller.create
  end
  
  def test_flash_notice_on_success
    controller = MenuItemsController.new(:params => { :menu_item => {:title => 'Classic'} })
    controller.create
    assert_equal "MenuItem was successfully created.", controller.flash[:notice]
  end
  
  def test_menu_item_is_available_for_view
    controller = MenuItemsController.new(:params => { :menu_item => {:title => 'Classic'} })
    controller.create
    assert_instance_variable(:menu_item).exists_in(controller)
  end
  
end

I look forward to the day when I can create controllers (via Controller.new) and I will have a valid object that I can easily test (with tests similar to the ones above).

Security Myth: Generic Login Error

Update begins with the quote

Several years ago I was working on a web application that had a login screen. I created separate error messages based on whether the user could not be found or the password was invalid. It wasn't a requirement, but I thought it was a nice to have (and I hadn't begun doing Agile, yet). When I demoed the feature to my boss he asked "Isn't that a security concern? Now hackers will know what are valid usernames." At the time I thought his observation was fair and I removed the feature.

Fast-forward a few years. These days, Several of my logins are my email address. Actually, my logins are usually an email address I set up for individual sites. For example, I might create americanairlines@jayfieldsthoughts.com if I were going to give American Airlines my email address (Don't bother emailing me at that address, it's not real). However, sometimes I don't bother to create an address for a site; I'll use something generic such as throwaway1@jayfieldsthoughts.com. Of course, this creates a problem when I go to a site that I use about once a year. Did I sign up with a specific address or did I use a throwaway one? The usual workflow from that point is to try a specific email address, and click the "forgot password" link if it fails. In forgot password I can try my specific email and a few throwaways if necessary. I know when I find a match, because the site tells me that "an email has been sent."

Here's where I have an issue. Maybe I can't find out from the login screen what is a valid username and what isn't, but it only takes me a click to get to a screen that tells me what a valid username is. Do we really believe that a hacker is going to give up on the login screen and not just hit the "forgot password" link like I do? I don't believe that, which brings me to the question: Why not just show me on the login screen that the email address is invalid.

Of course, this doesn't apply to sites that use non-email usernames. But, those sites that do, please improve my user experience and save me the extra click. You aren't providing me any extra protection. In fact, the only people you are slowing down are your users.

If the goal is to stop attackers enumerating valid account names then the forgotten password screen should not indicate the difference between a hit on username and a miss.

That's not a problem for the valid user as they'll get an e-mail to the valid account, but it stops the attacker from getting that information. -- Rory McCune

I forgot to address this. I considered the idea of changing the forgot password screen to display "an email may or may not have been sent" message following a submit. While it's an option, I basically dismissed the idea as a user experience so poor it wasn't worth the additional security. The message alone isn't horrible, but the problem is that a speedy email isn't always guaranteed. So, I can imagine a scenario where I misspell my email address, submit and get the "no confirmation" confirmation, and never receive an email. Or, I submit various email addresses waiting for an email to show up eventually. Later, I'm disappointed to find that I got it right the first time, but the server wasn't sending out emails very quickly and I wasted 15 minutes trying to guess all possible email combinations.

If security were such an issue that I had to display a meaningless message, I believe a superior solution would be to create unique usernames instead.

Good observation though and thanks for the comment Rory.

Ruby: Creating Custom Assertions

I've previously written about creating custom assertions for delegation and Validatable validations. Both solutions followed the same pattern, which is very similar to the example below.

Model.verify do
  fluent.interface
  fluent.interface
  ...
end

I do like the readability provided by the pattern; however, it isn't a practical solution.

My colleague and friend, John Hume, also created a solution for testing delegation: Handoff. The thing I really like about Handoff is that it follows the traditional test definition pattern.

def test_description
  assert_handoff....
end

At first glance this might not seem like a big deal; however, following the traditional pattern is crucial when you want to run a test in isolation.

A large problem with my delegation custom assertion is that you must run all the tests in a file if you want the delegation tests to run. It's possible to create ways to run the individual lines; however, with each custom assertion a new way to run it in isolation must be devised. A far simpler solution is to stick to the common test definition pattern and leverage existing tools that know how to run one test at a time.

The next release of Validatable will include custom assertions that follow the traditional test definition pattern.

Rails: How we test

Last weekend, at The Rails Edge (which was fantastic), Mike Clark (jokingly) told me that there was enough buzz around "The Jay Fields Way of Testing" that I should trademark it. I took that as a huge compliment; however, it's also not fair to the teams I've worked with. On all the project teams I am apart of, "How we test" is a collaborative decision. We don't test "The Jay Fields Way", we test our way.

So, what is "our way" as of Monday, September 3rd, 2007?

File Structure
We use RAILS_ROOT/test/unit & RAILS_ROOT/test/functional, which Rails provides by default. Under unit and functional we mirror the structure of the RAILS_ROOT/app folder.

For example, a user model that lives in

RAILS_ROOT/app/models/user.rb

would have unit tests in

RAILS_ROOT/test/unit/models/user_test.rb

and functional tests in

RAILS_ROOT/test/functional/models/user_test.rb

Tests In General
We use dust to define our tests, and we use the disallow_setup! method to ensure that setup methods aren't being added to the codebase.

Unit Tests
Unit tests are where we test classes in isolation. Within unit tests, dependencies are mocked or stubbed to ensure that a breaking test is broken because a feature of the class under test has changed. We defer testing object interactions to the functional tests. As a result, there are rarely cascading failures.

In general, there are far more unit tests than functional tests. This is a result of testing permutations, edge cases, etc within the unit test suite. If a format class contains 10 logical code paths, there should be at least 10 unit tests to verify that it works correctly; however, you may only need 1 functional test to verify that the format class works correctly with the other objects it interacts with.

Unit tests utilize a test helper specific to unit tests (RAILS_ROOT/test/unit/unit_test_helper.rb), which is the one and only require statement at the top of each unit test. In the unit_test_helper.rb we require mocha, UnitRecord and any other library that we need for unit testing. We require mocha only in unit_test_helper.rb to ensure that mocking/stubbing via mocha is only done in the unit tests. UnitRecord is a gem by Dan Manges that disables access to the database and provides the ability to easily unit test ActiveRecord::Base subclasses.

Functional Tests
Functional tests are where we verify that all the pieces of the application interact seamlessly. While stubs do exist, they are hand-coded and only stub external systems. Ideally, no stubs would exist; however, a trade-off is necessary when hitting external systems that significantly decrease your ability to quickly run the functional tests.

Functional tests often need small graphs of objects. Generally, this type of code is put in a setup method or created by fixtures. Both setup and fixtures provide a solution; however, we've found a more maintainable solution is to create a factory. Dan Manges has a great entry on our Functional Test Factory. A Factory has been very helpful; however, one thing to stress is that the Factory contains one create method per model. What's implied in that statement is that the methods are not defined on a scenario basis. If you need an Apartment that has a Renter that has a Job, you'll need to create each model within the test. Adding a method for a specific scenario (to the Factory) is the road to a completely unmaintainable Factory. We toyed with the idea of creating a fluent interface builder; however, Rails associations mostly remove the need.

For example, the following snippet can create an Apartment that has a Renter that has a job.

Factory.create_apartment(:renter => Factory.create_user(:job => Factory.create_job))

External Tests
In functional tests we stub external services; however, it's important to verify that the API of an external service hasn't changed. To perform that verification we write external services tests that interact with the external services. Since these tests are slower to run, they are generally only run on the continuous integration server or while debugging a contract change.

Why Bother?
There are several reasons for each decision we made; however, the context is the most important factor to consider. We work on large teams where the tests are run as often as possible and need to execute quickly. Those same tests also need to be as readable as possible, because it's likely that you'll be looking at tests you didn't write more often than not. While I think the above ideas are also good for small teams, I haven't personally put them to use on a small team.

Integration Tests, Selenium Tests, View Tests, aren't you missing something?
Yes, the above discussion does leave off the area of Acceptance Testing. We've yet to come up with something that works from project to project. On one project we were very successful with creating a DSL that executed as Rails integration tests locally and ran as Selenium on the build. While this might be a great solution, I haven't seen it used enough to recommend it.

View tests, in my experience, aren't necessary unless you are putting logic in your view. I prefer to keep the logic out of the view and ignore view tests entirely.

Look for updates on this entry, I'm sure I've forgotten a few things.