Refactoring and writing tests are both critical skills to being a programmer. Learning best practices and reading docs are important components of those abilities, but actually implementing them on a real-life project can be a difficult skill to acquire without directly shadowing someone.
So I decided to show the real-life progression of some new controller specs I wrote and refactored for a first-hand look at the process.
The new feature being tested in the code below involves a search function where a user is searching food products and can select certain ingredients, then select whether they want to find products with or without those ingredients. We also do the same thing with certain allergens.
So here is the state of my initial commit to the spec file:
describe AllergenSearchController, type: :controller do
let!(:egg_product) { create(:product) }
describe 'products that contain' do
it 'will return egg product when egg selected' do
egg_product.allergen_list << "Egg"
egg_product.save
get :index, search: { contain: 'true', allergen: ['Egg'] }
expect(assigns(:products)).to eq([egg_product])
end
it 'will return nothing when no allergens selected' do
get :index, search: { contain: 'true' }
expect(assigns(:products)).to eq([])
end
end
describe 'products that do not contain' do
before(:each) do
egg_product.allergen_list << "Egg"
egg_product.ingredient_list << "Egg"
egg_product.save
end
it 'will return product when corresponding ingredient is not selected' do
get :index, search: { contain: 'false', allergen: ['Wheat'] }
expect(assigns(:products)).to eq([egg_product])
end
it 'will not return product when corresponding ingredient is selected' do
get :index, search: { contain: 'false', allergen: ['Wheat', 'Egg']}
expect(assigns(:products).include?(egg_product)).to eq(false)
end
it 'will return nothing when no allergens selected' do
get :index, search: { contain: 'false', allergen: [''] }
expect(assigns(:products)).to eq([])
end
end
end
Corinne first tipped me off in a pull request that there were a number of issues evident.
After running Rubocop on the file, I had many issues, including lines being too long and misuse of double quotes as Corinne mentioned. Going through and beginning to fix those brought some other issues to my attention:
1) My initial assignment block does nothing to relate itself to eggs in that line or its applicable factory.
let!(:egg_product) { create(:product) }
I’m adding the relevant egg information in the tests themselves with:
egg_product.allergen_list << "Egg"
We aren't testing the assignment of an allergen or ingredient to a product, so this makes the tests unnecessarily long and introduces complexity that isn’t relevant to the test itself.
2) My describe statements are not very clear, lacking a reference to a method or specific action. If someone who was unfamiliar with the project looked at this file without already knowing about the AllergenSearchController, it would likely be confusing what this is actually testing.
3) Some expect statements are super specific and could easily break with changes to the search functionality.
expect(assigns(:products)).to eq([egg_product])
Instead of saying our search function should return at least a product, we are saying it has to return that product and only that product. So some of our expect lines aren’t properly reflecting what we’re actually trying to test.
The only positive takeaway from this initial attempt is the scope of the tests. There are not any combination of search inputs we’re missing and we aren’t testing anything superfluous. Otherwise, the implementation is lacking and basically every section is in need of refactoring.
So, here is my first attempt at some refactoring:
describe AllergenSearchController, type: :controller do
let!(:product) { create(:product, allergen_list: ['Milk'], ingredient_list: ['Egg']) }
describe 'products that contain' do
it 'will not return product when selected item is in the ingredient_list' do
get :index, search: { contain: 'true', allergen: ['Egg'] }
expect(assigns(:products).include?(product)).to eq(false)
end
it 'will return product when selected item is in the allergen_list' do
get :index, search: { contain: 'true', allergen: ['Milk'] }
expect(assigns(:products).include?(product)).to eq(true)
end
it 'will return nothing when no allergens selected' do
get :index, search: { contain: 'true' }
expect(assigns(:products).include?(product)).to eq(false)
end
end
describe 'products that do not contain' do
it 'will return product when corresponding ingredient is not in allergen list' do
get :index, search: { contain: 'false', allergen: ['Egg'] }
expect(assigns(:products).include?(product)).to eq(true)
end
it 'will not return product when at least one corresponding ingredient is in allergen list' do
get :index, search: { contain: 'false', allergen: ['Milk', 'Wheat'] }
expect(assigns(:products).include?(product)).to eq(false)
end
it 'will return nothing when no allergens selected' do
get :index, search: { contain: 'false', allergen: [''] }
expect(assigns(:products).include?(product)).to eq(false)
end
end
end
Lots of improvements! Let’s break down some of them:
1) The initial let block now has the attributes we need to actually get the tests to fire. No more repeated shovel operators constantly assigning things
let!(:product) { create(:product, allergen_list: ['Milk'], ingredient_list: ['Egg']) }
2) Changing our expect statements to consistently check for specific results within the result instead of asserting the result will exactly be something gives us flexibility in the future.
expect(assigns(:products).include?(product)).to eq(false)
For example, let’s say in the future we add or remove filters that might change our default returns. We’d likely have to come and completely re-work all those expect statements. But now we only need to change these statements if we completely change the functionality.
Now some things that still smell:
Our describe and actual test text blocks are far too complicated. Even as a developer familiar with the project, I’m having trouble trying to decipher exactly what each block is actually doing. Instead of describing the behavior of the code, I often am getting caught up describing the code itself. If I saw the following in a test suite result page I was trying to learn, I would be completely lost as to what was going wrong:
that do not contain will not return product when at least one corresponding ingredient is in allergen list
We also have a some repeated code that should be moved to a more high-level variable.
Now for the final product!
describe AllergenSearchController, type: :controller do
let!(:product) { create(:product, allergen_list: ['Milk']) }
subject(:products) { assigns(:products) }
context 'when search for allergens' do
context 'and selection is not allergen' do
it 'will not return product' do
get :index, search: { contain: 'true', allergen: ['Egg'] }
expect(products.include?(product)).to eq(false)
end
end
context 'and selection includes allergen' do
it 'will return product' do
get :index, search: { contain: 'true', allergen: ['Milk', 'Wheat'] }
expect(products.include?(product)).to eq(true)
end
end
context 'and nothing selected' do
it 'will return no products' do
get :index, search: { contain: 'true' }
expect(products).to eq([])
end
end
end
context 'when search for without allergens' do
context 'and selection is not an allergen' do
it 'will return product' do
get :index, search: { contain: 'false', allergen: ['Egg'] }
expect(products.include?(product)).to eq(true)
end
end
context 'and selection includes allergen' do
it 'will not return product' do
get :index, search: { contain: 'false', allergen: ['Milk', 'Wheat'] }
expect(products.include?(product)).to eq(false)
end
end
context 'and nothing selected' do
it 'will return no products' do
get :index, search: { contain: 'false', allergen: [''] }
expect(products).to eq([])
end
end
end
end
So much cleaner and easier to read! Thanks to Corinne and BetterSpecs I was able to turn my ridiculous test descriptions into actual logical statements!
Also, all of our tests were checking themselves against assigns(:products), which was a great opportunity for using a subject:
subject(:products) { assigns(:products) }
This is not a massive change but it does make individual lines more concise and makes the main purpose of the test file more explicit. And the cumulative effect of all these minor formatting and contextual changes is much larger than sometimes expected.
It sounds a little ridiculous, but this section might have been the most difficult and time-consuming part. Taking the time to really think about how to organize and label your tests (and code in general) isn’t easy or intuitive. Doing it well means having a thorough understanding of your code, the purpose of said code, and being able to translate both of those to other people. Pull requests and a strong grasp of a style guide are great tools for improving in this area.
So while this refactoring did not do much in the sense of changing the actual tests themselves, it has greatly improved the overall quality of this test file and the codebase at large. It will also make it easier to bring on more developers and collaborators!
I hope seeing the progression of a real-life refactor helps show how best practices are actually applied in real life.