Rspec - testing basic functionality - redundant testing?

Question

Being pretty new to testing, I was wondering how many of these tests make sense.

restaurants_controller_spec.rb

require 'spec_helper'
describe RestaurantsController do
 describe "GET #index" do
 before(:each) { get :index }
 let(:restaurants) { create_list :restaurant, 5 }
 it "populates @restaurants" do
 expect(restaurants).to match_array assigns(:restaurants)
 end
 it "renders the :dailycious view" do
 expect(response).to render_template :index
 end
 end
 describe "GET #show" do
 let(:restaurant) { create :restaurant }
 before(:each) { get :show, id: restaurant }
 it "assigns @restaurant" do
 expect(restaurant).to eq assigns(:restaurant)
 end
 it "renders :show" do
 expect(response).to render_template :show
 end
 end
 describe "GET #new" do
 before(:each) { get :new }
 context "user signed in" do
 before(:each) do
 login_user
 get :new
 end
 it "assigns @restaurant" do
 expect(assigns(:restaurant)).to be_a_new(Restaurant)
 end
 it "renders :new" do
 expect(response).to render_template :new
 end
 end
 context "user signed out" do
 it "redirects to login page" do
 expect(response).to redirect_to new_user_session_path
 end
 end
 end
end

support/controllers/controller_macros.rb

module Controllers
 module ControllerMacros
 def login_admin
 @request.env["devise.mapping"] = Devise.mappings[:admin]
 sign_in create(:confirmed_admin)
 end
 def login_user
 @request.env["devise.mapping"] = Devise.mappings[:user]
 sign_in create(:confirmed_user)
 end
 end
end

The new test makes sense to me, since it behaves differently when logged in / logged out, but do the other ones make sense as well?

Especially the index test seems superfluous. Doesn't this just check basic rails functionality? The index tests of other controllers will surely share most of the same code. Isn't this a contradiction to the DRY-Principle?

My setup includes Rspec, Capybara & FactoryGirl.

Next to the explanations I'm glad if you review this code, maybe DRY things up etc. :)

Sidenote: I adjusted my code to this tutorial.

Answer 1

Doesn't this just check basic rails functionality?

Yes and no. Yes in the sense that if you run the scaffold command, and follow the usual RESTful conventions, then that's what you get. But that's not Rails; it's just your specific use/application of the Rails framework.

You could easily make a controller that doesn't have an index action. Or you could make a controller where the index action renders the edit template for some strange reason. And you tests would help you verify and/or avoid that, depending on your needs.

True, there'll be repetition across controllers, but repetition in tests not quite as concerning as repetition in your app code. But you can look at RSpec's"shared examples" if you want to share common specs.

Now, RSpec is about specifications, so when you write a test that checks whether a variable has been assigned, you're specifying and checking the contract between the controller and the view; the view will expect that variable to be there, so you ensure that the controller sets it.

Similarly, when you specify that expect(response).to render_template :show it's because that's what that action is supposed to do. Yes, it's usually ok to assume that the show action will render the show template - but it's not a law.

If you run rspec with the --format documentation option, what you'll see is a detailed explanation of what your code does - or is supposed to do, e.g.:

RestaurantsController
 GET #show
 assigns @restaurant
 renders :show

And yes, that's indeed what it's supposed to do. It's the conventional setup for a show action, so you might think "that goes without saying!". And yeah, it kinda does... if you indeed wrote it that way. Maybe there's a bug.

---

You current code looks fine by the way. A few notes though:

it "renders the :dailycious view" do
 expect(response).to render_template :index
end

Your spec says "renders the :dailycious view", but you're actually checking something else? If the idea is that the index template renders a partial called "dailycious" then you should check that in a view spec. Right now your spec is like asking someone "Did you remember to buy milk?", and the person answering "I went to the store". Well... good for you, but that doesn't answer the question.

Also,

GET #show
 it "assigns @restaurant"

This is just me being super-duper pedantic, but consider rewording that more like "assigns the requested Restaurant record as @restaurant". If you look at the "documentation" output above, all it just tells you that something's been assigned to @restaurant. Not what it is.
Admittedly, this is not a great example, since it's pretty clear already. But being descriptive is a good habit to get into. Try to keep that "documentation" output in mind, and ask yourself if the spec your writing is sufficiently explanatory. Just imagine that every other part of the app is written by someone else entirely, and, besides yourself, that's who your specs are for. As though they're looking at the documentation output to learn how your code works.

On a practical note, you probably don't need to create 5 restaurant records when testing the index action. It's a good idea to create more than 1 record, to ensure that the controller does load multiple records, but you don't need more than 2 records to check that. FactoryGirl has a create_pair method you can use for just this sort of thing.

• davegson: Hey Flambino. Firstly, yeah, I already setup --format documentation in my Guardfile. It's a great tip for everybody! 2) Nice catch with the dailycious view. It was supposed to test if the dailycious layout was rendered. Now it makes sure that both the index & dailycious template is rendered. 3) I'm not too sure about your wording, since nothing is getting requested. I'd prefer assigns a new Restaurant record to @restaurant. 4) Thanks allot! I wasn't aware of the create_pair function. I'll definitley use that instead. 5) Thanks!!! :)) You've been a great help!
• davegson: And your explanation regarding the testing makes a lot of sense to me. Thanks for explaining that.
• Flambino: @TheChamp No prob, glad it was useful. With regard to the "it assigns.." thing, I was looking at the spec for the show action, where a specific ID is indeed being requested. But yes, for the new action, your wording is the right one.