Factory Refactoring - Done!

After over 50 Pull Requests spread over the last 9 months, I’ve finally finished refactoring the openstreetmap-website test suite to use factories instead of fixtures. Time for a celebration!

As I’ve discussed in previous posts, the openstreetmap-website codebase powers the main OpenStreetMap website and the map editing API. The test suite has traditionally only used fixtures, where all test data was preloaded into the database and the same data used for every test. One drawback of this approach is that any change to the fixtures can have knock-on effects on other tests. For example, adding another diary entry to the fixtures could break a different test which expects a particular number of diary entries to be found by a search query. There are also more subtle problems, including the lack of clear intent in the tests. When you read a test that asserts that a given Node or Way is found, it was often not clear which attributes of the fixture were important - perhaps that Node belonged to a particular Changeset, or had a particular timestamp, or was in a particular location, or a mixture of other attributes. Figuring out these hidden intents for each test was often a major source of the refactoring effort - and there’s 1080 tests in the suite, with more than 325,000 total assertions!

Changing to factories has made the tests independent of each other. Now every test starts with a blank database, and only the objects that are needed are created, using the factories. This means that tests can more easily create a particular database record for the test at hand, without interfering with other tests. The intent of the test is often clearer too, since creating objects from factories happens in the test itself and are therefore explicit about what attributes are the important ones.

An example of the benefits of factories was when I fixed a bug around encoding of diary entry titles in our RSS feeds. I easily created a diary entry with a specific and unusual title, without interfering with any other tests or having to create yet another fixture.

def test_rss_character_escaping
  create(:diary_entry, :title => "<script>")
  get :rss, :format =&gt; :rss

  assert_match "<title>&lt;script&gt;</title>", response.body
end

All in all, this took much, much longer than I was expecting! Looking back, I feel I might have picked a different task, but I’m glad that it’s all done now. I’m certainly glad that I won’t have to do it all again! Moving on, it’s now easier for me and the other developers to write robust tests, and this will help us implement new features more quickly.

Big thanks go to Tom Hughes for reviewing all 51 individual pull requests! Thanks also to everyone who lent me their encouragement.

So the big question is - what will I work on next?