In Which I Dissect and Improve a Rails Test File I Found on Reddit

by Jason Swett,

I recently found a Reddit post which shared the code of a test and basically asked, “Am I doing this right?”

I’m going to dissect this test and make suggestions for improvement. Here’s the original code as shared in the post.

The original code

Initial overall thoughts

Overall I think the OP did a pretty good job. He or she got the spirit right. The changes I would make are fairly minor.

Before I get into the actual improvements I want to say that I personally am not familiar with the style of using matchers like must_be and I actually wasn’t able to turn up any discussion of must_be using a quick Google search. So as I add improvements to the test code, I’m also going to convert the syntax to the syntax I’m personally used to using.

Validity of fresh object

Let’s first examine the first test, the test for the validity of a User instance:

It’s not a bad idea to assert that a model object must be valid when it’s in a fresh, untouched state. This test could be improved, however, by making it clearer that when we say “it must be valid”, we mean we’re talking about a freshly instantiated object.

Here’s how I might write this test instead:

We can actually make the test even more concise by taking advantage of subject:

Persistence

Let’s look at the next test case, the test that asserts that a user can be persisted.

I don’t see any value in this test. It’s true that an object needs to be able to be successfully saved, but we’ll be doing plenty of saving in the other tests anyway. Plus it’s typically pretty safe to take for granted that saving an ActiveRecord object is going to work. Rather than improving this test, I would just delete it.

Email validity

Lastly let’s take a look at the tests for email validity.

The overall idea is sound although the structure is actually backwards from how I would do it. Rather than saying “here are all the ways the object could be invalid”, I would list the various conditions and then, inside of each condition, assert the validity state. So here’s how I might structure these tests:

I’ll also try to improve each of these test cases individually.

Email uniqueness

This test can be written more concisely. Rather than this:

We can do this:

Email presence, email format validity, MX record validity, and blacklist check

These four test cases are all very similar so I’ll deal with them as a group. Here are the original version:

For the most part these tests are fine. There’s only one significant thing I would do differently. I find it redundant to both assert that there’s an error and assert that the object is invalid. If the object has an error I think it’s safe to assume that it’s also invalid. Here are my versions:

The entire improved test file

Lastly, here’s the full file after I’ve made my improvements:

  •  
  •  
  •  
  •  

3 thoughts on “In Which I Dissect and Improve a Rails Test File I Found on Reddit

  1. Federico

    I prefer four-phase testing (eg: https://robots.thoughtbot.com/four-phase-test) with very big test files, having many let scopes gets confusing easily, you have to go up and down the file to fish for definitions and nested contexts.

    With four-phase testing you have a flat structure of tests. There is some repetition, sure, but it’s worth for making the test easy to read in a linear fashion.

    Reply

Leave a Reply

Your email address will not be published. Required fields are marked *