Replacing RSpec controller tests, part 3: Removing business logic from controllers
After the Rails core team announced major changes to controller-level testing upon the release of Rails 5.0, I wrote a couple of pieces in Everyday Rails about how to shift testing responsibility up, to either request specs, or browser-based feature specs (or system specs, in the current Rails testing landscape). However, relying solely on high level tests like these can lead to slower test suites. And even if they’re reasonably fast, they can still require extra setup like checking for proper authentication checking or populating a database with extraneous details.
Here’s an intentionally short example. It’s taken from the Rails 4.1/RSpec 3.1 edition of my book, Everyday Rails Testing with RSpec, but the concept applies to newer and older versions of RSpec and Rails alike. And as I’ll discuss later in this article, it applies to more complicated examples, too. Let’s look at the existing implementation and test coverage first.
This controller action checks for the value of
params[:letter] and performs one of two different behaviors, depending on the parameter’s presence:
:letter parameter is found, this finder is run:
The controller code is exercised through a couple of tests:
I've updated this code sample slightly to use the recently renamed
factory_bot gem for creating test data.
Wow, that’s simple–just five lines of controller code, and a single line of code in the model, right? And thorough test coverage for both use cases (letter provided, no letter provided)? But if my 2017 self were to code review my younger self’s work, I might raise the following questions and concerns:
- What happens if a user provides an unexpected value, such as a number or non-alphanumeric character?
- Hmm, it looks like the controller code and model code are in violation of the DRY principle–they both specify an
order for sorting found contacts, but the order in the model appears to be incomplete. Should it sort by last name and first name, too? And can this code be rewritten to remove duplication, so that ordering behavior remains consistent?
- Testing the controller code directly requires usage of the
assigns method, which has been soft-deprecated in Rails 5 and will eventually be removed. Could we try writing this code so that it’s more testable at a different level?
Let’s think about the code’s testability for a moment. In addition to the controller tests listed above, we also currently rely on model-level tests, explicitly interacting with
It bothers me enough that test coverage for this relatively simple feature is spread across two levels of the application. It bothers me even more that I have to go through the HTTP stack to test part of it at all. So, how to improve upon this design?
For the sake of discussion
In the interest of keeping things as simple as possible, perhaps the next step would be to move all the logic into the
Contact model. For example:
Check out how much this simplifies the controller action:
With logic removed from the controller action, we can move coverage of this functionality over to the Contact model–for example:
I still think it’s worth having some coverage to indicate that everything is wired together. In 2017, that probably means a system spec (or feature or request spec, if you’re not on Rails 5.1 and RSpec 3.7). But now, higher-level specs can focus more on a happy path integration, and less on the implementation details of how the query is built and performed.
Finally, we can delete the original section of coverage in the controller spec. Even though it’s still passing, it’s duplicate coverage now, and not needed.
And with the code more isolated, we can start to see the potential issues with redundant logic and inconsistent sorting:
I can think of a few ways to rewrite this to fix the issues mentioned. Maybe
self.by_letter can be inlined into
self.search_by_letter, or converted to a private method. Either way, there’s a good chance the test coverage for
self.by_letter is redundant, and could be removed.
But that’s outside the scope of this article. In the meantime, we’ve now got tests written in a way that those changes can be tested efficiently, without relying on a deprecated test layer.
You may be rolling your eyes at me right now. This is too simple, right? It sure is–and that’s my point. Many times, complex controller code starts out this simple, but quickly expands. In fact, maybe we’re already stressing the Contact model too much with this new method. Maybe an even better solution is an object dedicated to this search by letter process, something like:
There’s a lot to like about this approach. It gives us room to grow as our simple search-by-letter functionality becomes more complex. What if we also need to check params for location, employment status, or other factors? We can test these various options directly, in isolation, without the overhead of the rest of the stack. Meanwhile, our higher-level test continues to ensure that the new object is wired to the rest of the app correctly.
I’m by no means the first person to suggest pulling code out of controllers and into standalone objects. In just the past couple of weeks, there have been a few excellent articles on the subject, ranging from Avdi Grimm’s “Enough with the Service Objects, Already”, to Aaron Lasseigne’s rebuttal “Why Aren’t We Using More Service Objects Already”, to Tiago Farias’s tutorial on using the interactor gem to make extracting this code easy.
Work to keep this type of code out of controllers. Learn about moving domain logic into focused, standalone objects (whether you call them services, procedures, workflows, or something else). Isolate your test coverage so that the details of how those objects do their work can be written and run quickly.
I’ll admit, I balked at this general idea for too long. Rails convention (by way of the file structure that gets generated with
rails new) suggests limiting application code to models, views, controllers, and concerns. And it could be confusing to a developer new to a project to stumble upon a design that relies on unconventional (to Rails) techniques.
But as I continue to grow as a developer, I’ve come to realize that the chances that I will have to work with a codebase in the coming weeks, months, and years are much higher than the chances of a new developer joining my team. And even when that does happen, it’s not like we’re deviating so far off track that new developers can’t pick things up in short order.
I’d argue that taking such an approach makes it easier for new developers to pick up on the domain requirements of a project. If you begin to think of each class as a tiny program that eventually talks to other programs, you’ll find that it’s far simpler to reason about your application as a whole.
Of the three scenarios I’ve provided throughout this series for preparing your codebase for the eventual retirement of controller-level testing, this is my favorite. That said, I know it’s not always practical to to do refactoring to this degree. That’s why it may be better to start with moving high-level tests up to features or requests, then refactor application code so that it can be more easily tested in isolation, then simplify the high-level coverage to ensure that objects are communicating with each other.
blog comments powered by