From e23bf233e65ef1990163512cf8fe5bea3cd1d9b3 Mon Sep 17 00:00:00 2001 From: Sydney Young Date: Thu, 4 Aug 2016 08:55:55 -0400 Subject: [PATCH 1/2] Refactor Requirements Controller Spec Resolves #1594 --- .../requirements_controller_spec.rb | 203 ++++++------------ spec/support/controller_helpers.rb | 24 ++- spec/support/mockers/category.rb | 20 ++ spec/support/mockers/equipment_item.rb | 20 ++ spec/support/mockers/equipment_model.rb | 31 +++ spec/support/mockers/mocker.rb | 102 +++++++++ spec/support/mockers/requirement.rb | 11 + spec/support/mockers/reservation.rb | 18 ++ spec/support/mockers/user.rb | 17 ++ .../shared_examples/controller_examples.rb | 12 ++ 10 files changed, 312 insertions(+), 146 deletions(-) create mode 100644 spec/support/mockers/category.rb create mode 100644 spec/support/mockers/equipment_item.rb create mode 100644 spec/support/mockers/equipment_model.rb create mode 100644 spec/support/mockers/mocker.rb create mode 100644 spec/support/mockers/requirement.rb create mode 100644 spec/support/mockers/reservation.rb create mode 100644 spec/support/mockers/user.rb create mode 100644 spec/support/shared_examples/controller_examples.rb diff --git a/spec/controllers/requirements_controller_spec.rb b/spec/controllers/requirements_controller_spec.rb index 4c7d1874f..3772b8a33 100644 --- a/spec/controllers/requirements_controller_spec.rb +++ b/spec/controllers/requirements_controller_spec.rb @@ -1,213 +1,142 @@ +# frozen_string_literal: true require 'spec_helper' -# note, these tests are complex in order to test the admin security features -# -- namely, it was necessary to test two contexts for each method: the user -# being an admin, and not. describe RequirementsController, type: :controller do - before(:each) do - mock_app_config - @requirement = FactoryGirl.create(:requirement, contact_name: 'Adam Bray') - end + # NOTE: many of these are essentially just testing permissions + before(:each) { mock_app_config } describe 'GET index' do context 'is admin' do before(:each) do - sign_in FactoryGirl.create(:admin) + allow(Requirement).to receive(:all).and_return(Requirement.none) + mock_user_sign_in(UserMock.new(:admin)) get :index end - it { is_expected.to respond_with(:success) } - it { is_expected.to render_template(:index) } - it { is_expected.not_to set_flash } + it_behaves_like 'successful request', :index it 'should populate an array of all requirements' do - expect(assigns(:requirements)).to eq([@requirement]) + expect(Requirement).to have_received(:all).twice end end context 'not an admin' do - it 'should redirect to root url if not an admin' do - sign_in FactoryGirl.create(:user) + before do + mock_user_sign_in get :index - expect(response).to redirect_to(root_url) - end - end - end - describe 'GET show' do - context 'is an admin' do - before(:each) do - sign_in FactoryGirl.create(:admin) - get :show, id: @requirement - end - it { is_expected.to respond_with(:success) } - it { is_expected.to render_template(:show) } - it { is_expected.not_to set_flash } - it 'should set @requirement to the selected requirement' do - expect(assigns(:requirement)).to eq(@requirement) - end - end - context 'not an admin' do - it 'should redirect to root url if not an admin' do - sign_in FactoryGirl.create(:user) - get :show, id: @requirement - expect(response).to redirect_to(root_url) end + it_behaves_like 'redirected request' end end + describe 'GET new' do context 'is admin' do before(:each) do - sign_in FactoryGirl.create(:admin) + allow(Requirement).to receive(:new) + mock_user_sign_in(UserMock.new(:admin)) get :new end - it { is_expected.to respond_with(:success) } - it { is_expected.to render_template(:new) } - it { is_expected.not_to set_flash } + it_behaves_like 'successful request', :new it 'assigns a new requirement to @requirement' do - expect(assigns(:requirement)).to be_new_record - expect(assigns(:requirement).is_a?(Requirement)).to be_truthy + expect(Requirement).to have_received(:new).twice end end context 'not an admin' do - it 'should redirect to root url if not an admin' do - sign_in FactoryGirl.create(:user) + before do + mock_user_sign_in get :new - expect(response).to redirect_to(root_url) - end - end - end - describe 'GET edit' do - context 'is admin' do - before(:each) do - sign_in FactoryGirl.create(:admin) - get :edit, id: @requirement - end - it 'should set @requirement to the selected requirement' do - expect(assigns(:requirement)).to eq(@requirement) - end - it { is_expected.to respond_with(:success) } - it { is_expected.to render_template(:edit) } - it { is_expected.not_to set_flash } - end - context 'not admin' do - it 'should redirect to root url if not an admin' do - sign_in FactoryGirl.create(:user) - get :edit, id: @requirement - expect(response).to redirect_to(root_url) end + it_behaves_like 'redirected request' end end - describe 'PUT update' do + + describe 'POST create' do context 'is admin' do - before(:each) do - sign_in FactoryGirl.create(:admin) - end - context 'with valid attributes' do + before(:each) { mock_user_sign_in(UserMock.new(:admin)) } + context 'successful save' do + let!(:req) { FactoryGirl.build_stubbed(:requirement) } before(:each) do - put :update, - id: @requirement, - requirement: FactoryGirl.attributes_for(:requirement, - contact_name: 'John Doe') + allow(Requirement).to receive(:new).and_return(req) + allow(req).to receive(:save).and_return(true) + post :create, requirement: { contact_name: 'name' } end - it 'should set @requirement to the correct requirement' do - expect(assigns(:requirement)).to eq(@requirement) - end - it 'should update the attributes of @requirement' do - @requirement.reload - expect(@requirement.contact_name).to eq('John Doe') + it 'saves a new requirement' do + expect(Requirement).to have_received(:new).twice + expect(req).to have_received(:save) end - it { is_expected.to redirect_to(@requirement) } + it { is_expected.to redirect_to(req) } it { is_expected.to set_flash } end context 'with invalid attributes' do before(:each) do - put :update, - id: @requirement, - requirement: FactoryGirl.attributes_for(:requirement, - contact_name: '') + req = RequirementMock.new(save: false) + allow(Requirement).to receive(:new).and_return(req) + post :create, requirement: { contact_name: 'name' } end - it 'should not update the attributes of @requirement' do - @requirement.reload - expect(@requirement.contact_name).not_to eq('') - expect(@requirement.contact_name).to eq('Adam Bray') - end - it { is_expected.to render_template(:edit) } it { is_expected.not_to set_flash } + it { is_expected.to render_template(:new) } end end context 'not admin' do - it 'should redirect to root url if not an admin' do - sign_in FactoryGirl.create(:user) - get :update, - id: @requirement, - requirement: FactoryGirl.attributes_for(:requirement) - expect(response).to redirect_to(root_url) + before do + mock_user_sign_in + post :create, requirement: { contact_name: 'name' } end + it_behaves_like 'redirected request' end end - describe 'POST create' do + + describe 'PUT update' do context 'is admin' do - before(:each) do - sign_in FactoryGirl.create(:admin) - end + before(:each) { mock_user_sign_in(UserMock.new(:admin)) } context 'with valid attributes' do + let!(:req) { FactoryGirl.build_stubbed(:requirement) } before(:each) do - post :create, requirement: FactoryGirl.attributes_for(:requirement) + allow(Requirement).to receive(:find).with(req.id.to_s).and_return(req) + allow(req).to receive(:update_attributes).and_return(true) + put :update, id: req.id, requirement: { contact_name: 'Name' } end - it 'saves a new requirement' do - expect do - post :create, requirement: FactoryGirl.attributes_for(:requirement) - end.to change(Requirement, :count).by(1) + it 'should update the attributes of @requirement' do + expect(req).to have_received(:update_attributes) end - it { is_expected.to redirect_to(Requirement.last) } + it { is_expected.to redirect_to(req) } it { is_expected.to set_flash } end context 'with invalid attributes' do + let!(:req) { RequirementMock.new(traits: [:findable]) } before(:each) do - post :create, - requirement: FactoryGirl.attributes_for(:requirement, - contact_name: nil) - end - it 'fails to save a new requirment' do - expect do - post :create, - requirement: FactoryGirl.attributes_for(:requirement, - contact_name: nil) - end.not_to change(Requirement, :count) + allow(req).to receive(:update_attributes).and_return(false) + put :update, id: req.id, requirement: { contact_name: 'Name' } end + it { is_expected.to render_template(:edit) } it { is_expected.not_to set_flash } - it { is_expected.to render_template(:new) } end end context 'not admin' do - it 'should redirect to root url if not an admin' do - sign_in FactoryGirl.create(:user) - post :create, requirement: FactoryGirl.attributes_for(:requirement) - expect(response).to redirect_to(root_url) + before do + mock_user_sign_in + put :update, id: 1, requirement: { contact_name: 'Name' } end + it_behaves_like 'redirected request' end end + describe 'DELETE destroy' do context 'is admin' do + let!(:req) { RequirementMock.new(traits: [:findable]) } before(:each) do - sign_in FactoryGirl.create(:admin) - end - it 'assigns the selected requirement to @requirement' do - delete :destroy, id: @requirement - expect(assigns(:requirement)).to eq(@requirement) + mock_user_sign_in(UserMock.new(:admin)) + delete :destroy, id: req.id end - it 'removes @requirement from the database' do - expect do - delete :destroy, id: @requirement - end.to change(Requirement, :count).by(-1) + it 'destroys the requirement' do + expect(req).to have_received(:destroy).with(:force) end it 'should redirect to the requirements index page' do - delete :destroy, id: @requirement expect(response).to redirect_to requirements_url end end context 'not admin' do - it 'should redirect to root url if not an admin' do - sign_in FactoryGirl.create(:user) - delete :destroy, id: @requirement - expect(response).to redirect_to(root_url) + before do + mock_user_sign_in + delete :destroy, id: 1 end + it_behaves_like 'redirected request' end end end diff --git a/spec/support/controller_helpers.rb b/spec/support/controller_helpers.rb index 87e36a0f3..9795e17c7 100644 --- a/spec/support/controller_helpers.rb +++ b/spec/support/controller_helpers.rb @@ -1,14 +1,20 @@ -# some basic helpers to simulate devise controller methods in specs +require Rails.root.join('spec/support/mockers/user.rb') + module ControllerHelpers - def current_user - user_session_info = - response.request.env['rack.session']['warden.user.user.key'] - return unless user_session_info - user_id = user_session_info[0][0] - User.find(user_id) + def mock_user_sign_in(user = UserMock.new(traits: [:findable])) + pass_app_setup_check + allow(request.env['warden']).to receive(:authenticate!).and_return(user) + # necessary for permissions to work + allow(ApplicationController).to receive(:current_user).and_return(user) + allow(Ability).to receive(:new).and_return(Ability.new(user)) + allow_any_instance_of(described_class).to \ + receive(:current_user).and_return(user) end - def user_signed_in? - !current_user.nil? + private + + def pass_app_setup_check + allow(AppConfig).to receive(:first).and_return(true) unless AppConfig.first + allow(User).to receive(:count).and_return(1) unless User.first end end diff --git a/spec/support/mockers/category.rb b/spec/support/mockers/category.rb new file mode 100644 index 000000000..1bed58ea6 --- /dev/null +++ b/spec/support/mockers/category.rb @@ -0,0 +1,20 @@ +require Rails.root.join('spec/support/mockers/mocker.rb') +require Rails.root.join('spec/support/mockers/equipment_model.rb') + +class CategoryMock < Mocker + def self.klass + Category + end + + def self.klass_name + 'Category' + end + + private + + def with_equipment_models(models: nil, count: 1) + models ||= Array.new(count) { EquipmentModelMock.new } + parent_has_many(mocked_children: models, parent_sym: :category, + child_sym: :equipment_models) + end +end diff --git a/spec/support/mockers/equipment_item.rb b/spec/support/mockers/equipment_item.rb new file mode 100644 index 000000000..9819193a1 --- /dev/null +++ b/spec/support/mockers/equipment_item.rb @@ -0,0 +1,20 @@ +require Rails.root.join('spec/support/mockers/mocker.rb') +require Rails.root.join('spec/support/mockers/equipment_model.rb') + +class EquipmentItemMock < Mocker + def self.klass + EquipmentItem + end + + def self.klass_name + 'EquipmentItem' + end + + private + + def with_model(model: nil) + model ||= EquipmentModelMock.new + child_of_has_many(mocked_parent: model, parent_sym: :equipment_model, + child_sym: :equipment_items) + end +end diff --git a/spec/support/mockers/equipment_model.rb b/spec/support/mockers/equipment_model.rb new file mode 100644 index 000000000..fede4a578 --- /dev/null +++ b/spec/support/mockers/equipment_model.rb @@ -0,0 +1,31 @@ +require Rails.root.join('spec/support/mockers/mocker.rb') +require Rails.root.join('spec/support/mockers/category.rb') +require Rails.root.join('spec/support/mockers/equipment_item.rb') + +class EquipmentModelMock < Mocker + def self.klass + EquipmentModel + end + + def self.klass_name + 'EquipmentModel' + end + + private + + def with_item(item:) + with_items(items: [item]) + end + + def with_items(items: nil, count: 1) + items ||= Array.new(count) { EquipmentItemMock.new } + parent_has_many(mocked_children: items, parent_sym: :equipment_model, + child_sym: :equipment_items) + end + + def with_category(cat: nil) + cat ||= CategoryMock.new + child_of_has_many(mocked_parent: cat, parent_sym: :category, + child_sym: :equipment_models) + end +end diff --git a/spec/support/mockers/mocker.rb b/spec/support/mockers/mocker.rb new file mode 100644 index 000000000..a231b43af --- /dev/null +++ b/spec/support/mockers/mocker.rb @@ -0,0 +1,102 @@ +require 'rspec/mocks/standalone' + +# This class behaves as an extension of rspec-mocks' instance_spy. +# It is intended to be extended and used to make mocking models much simpler! +# +# To create a new subclass, the following methods must be overridden: +# - self.klass must return the class that the subclass is mocking +# - self.klass_name must return a string that matches the class being mocked +# +# Some examples using the EquipmentModelMock subclass: +# A mock that can be "found" with EquipmentModel#find: +# EquipmentModelMock.new(traits: [:findable]) +# A mock with a set of attributes: +# EquipmentModelMock.new(name: 'Camera', late_fee: 3) +# A mock with attributes and method stubs: +# EquipmentModelMock.new(name: 'Camera', model_restriced: false) +# A findable mock with attributes: +# EquipmentModelMock.new(traits: [:findable], name: 'Camera') +# +# A trait can be any method that exists on the mocker superclass or child class. +# To create an EquipmentModel that belongs to an existing category, camera: +# EquipmentModelMock.new(traits: [[:with_category, cat: camera]]) +# +# Use caution before adding methods -- any method defined here should be usable +# by all subclasses, with the exception of the association stub methods. + +class Mocker < RSpec::Mocks::InstanceVerifyingDouble + include RSpec::Mocks + + FIND_METHODS = [:find, :find_by_id] + + def initialize(traits: [], **attrs) + # from RSpec::Mocks::ExampleMethods + # combination of #declare_verifying_double and #declare_double + ref = ObjectReference.for(self.class.klass_name) + RSpec::Mocks.configuration.verifying_double_callbacks.each do |block| + block.call(ref) + end + attrs ||= {} + super(ref, attrs) + self.as_null_object + process_traits(traits) + end + + def process_traits(traits) + traits.each { |t| send(*t) } + end + + private + + def klass + Object + end + + def klass_name + 'Object' + end + + def spy + self + end + + # lets us use rspec-mock syntax in mockers + def receive(method_name, &block) + Matchers::Receive.new(method_name, block) + end + + def allow(target) + AllowanceTarget.new(target) + end + + # Traits + def findable + id = FactoryGirl.generate(:unique_id) + allow(spy).to receive(:id).and_return(id) + FIND_METHODS.each do |method| + allow(self.class.klass).to receive(method) + allow(self.class.klass).to receive(method).with(id).and_return(spy) + allow(self.class.klass).to receive(method).with(id.to_s).and_return(spy) + end + end + + # Generalized association stubs + def child_of_has_many(mocked_parent:, parent_sym:, child_sym:) + allow(spy).to receive(parent_sym).and_return(mocked_parent) + children = if mocked_parent.send(child_sym).is_a? Array + mocked_parent.send(child_sym) << spy + else + [spy] + end + allow(mocked_parent).to receive(child_sym).and_return(children) + end + + def parent_has_many(mocked_children:, parent_sym:, child_sym:) + if mocked_children.is_a? Array + mocked_children.each do |child| + allow(child).to receive(parent_sym).and_return(spy) + end + end + allow(spy).to receive(child_sym).and_return(mocked_children) + end +end diff --git a/spec/support/mockers/requirement.rb b/spec/support/mockers/requirement.rb new file mode 100644 index 000000000..96e382d4f --- /dev/null +++ b/spec/support/mockers/requirement.rb @@ -0,0 +1,11 @@ +require Rails.root.join('spec/support/mockers/mocker.rb') + +class RequirementMock < Mocker + def self.klass + Requirement + end + + def self.klass_name + 'Requirement' + end +end diff --git a/spec/support/mockers/reservation.rb b/spec/support/mockers/reservation.rb new file mode 100644 index 000000000..de4d13c72 --- /dev/null +++ b/spec/support/mockers/reservation.rb @@ -0,0 +1,18 @@ +require Rails.root.join('spec/support/mockers/mocker.rb') + +class ReservationMock < Mocker + def self.klass + Reservation + end + + def self.klass_name + 'Reservation' + end + + private + + def for_user(user:) + child_of_has_many(mocked_parent: user, parent_sym: :reserver, + child_sym: :reservations) + end +end diff --git a/spec/support/mockers/user.rb b/spec/support/mockers/user.rb new file mode 100644 index 000000000..61e2b4888 --- /dev/null +++ b/spec/support/mockers/user.rb @@ -0,0 +1,17 @@ +require Rails.root.join('spec/support/mockers/mocker.rb') + +class UserMock < Mocker + def initialize(role = :user, traits: [], **attrs) + attrs = FactoryGirl.attributes_for(role).merge attrs + traits = [:findable] if traits.empty? + super(traits: traits, **attrs) + end + + def self.klass + User + end + + def self.klass_name + 'User' + end +end diff --git a/spec/support/shared_examples/controller_examples.rb b/spec/support/shared_examples/controller_examples.rb new file mode 100644 index 000000000..7caff4e44 --- /dev/null +++ b/spec/support/shared_examples/controller_examples.rb @@ -0,0 +1,12 @@ +require 'spec_helper' + +shared_examples_for 'successful request' do |template| + it { is_expected.to respond_with(:success) } + it { is_expected.to render_template(template) } + it { is_expected.not_to set_flash } +end + +shared_examples_for 'redirected request' do + it { expect(response).to be_redirect } + it { is_expected.to set_flash } +end From 6a7266c984b95f833d187e8acc82f4698978bc96 Mon Sep 17 00:00:00 2001 From: Sydney Young Date: Tue, 9 Aug 2016 10:46:03 -0400 Subject: [PATCH 2/2] rubocop --- spec/support/controller_helpers.rb | 1 + spec/support/mockers/category.rb | 1 + spec/support/mockers/equipment_item.rb | 1 + spec/support/mockers/equipment_model.rb | 1 + spec/support/mockers/mocker.rb | 5 +++-- spec/support/mockers/requirement.rb | 1 + spec/support/mockers/reservation.rb | 1 + spec/support/mockers/user.rb | 1 + spec/support/shared_examples/controller_examples.rb | 1 + 9 files changed, 11 insertions(+), 2 deletions(-) diff --git a/spec/support/controller_helpers.rb b/spec/support/controller_helpers.rb index 9795e17c7..a90cf501c 100644 --- a/spec/support/controller_helpers.rb +++ b/spec/support/controller_helpers.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true require Rails.root.join('spec/support/mockers/user.rb') module ControllerHelpers diff --git a/spec/support/mockers/category.rb b/spec/support/mockers/category.rb index 1bed58ea6..9c4cc7521 100644 --- a/spec/support/mockers/category.rb +++ b/spec/support/mockers/category.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true require Rails.root.join('spec/support/mockers/mocker.rb') require Rails.root.join('spec/support/mockers/equipment_model.rb') diff --git a/spec/support/mockers/equipment_item.rb b/spec/support/mockers/equipment_item.rb index 9819193a1..87f69f37b 100644 --- a/spec/support/mockers/equipment_item.rb +++ b/spec/support/mockers/equipment_item.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true require Rails.root.join('spec/support/mockers/mocker.rb') require Rails.root.join('spec/support/mockers/equipment_model.rb') diff --git a/spec/support/mockers/equipment_model.rb b/spec/support/mockers/equipment_model.rb index fede4a578..398137a2d 100644 --- a/spec/support/mockers/equipment_model.rb +++ b/spec/support/mockers/equipment_model.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true require Rails.root.join('spec/support/mockers/mocker.rb') require Rails.root.join('spec/support/mockers/category.rb') require Rails.root.join('spec/support/mockers/equipment_item.rb') diff --git a/spec/support/mockers/mocker.rb b/spec/support/mockers/mocker.rb index a231b43af..8b95b22a8 100644 --- a/spec/support/mockers/mocker.rb +++ b/spec/support/mockers/mocker.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true require 'rspec/mocks/standalone' # This class behaves as an extension of rspec-mocks' instance_spy. @@ -27,7 +28,7 @@ class Mocker < RSpec::Mocks::InstanceVerifyingDouble include RSpec::Mocks - FIND_METHODS = [:find, :find_by_id] + FIND_METHODS = [:find, :find_by_id].freeze def initialize(traits: [], **attrs) # from RSpec::Mocks::ExampleMethods @@ -38,7 +39,7 @@ def initialize(traits: [], **attrs) end attrs ||= {} super(ref, attrs) - self.as_null_object + as_null_object process_traits(traits) end diff --git a/spec/support/mockers/requirement.rb b/spec/support/mockers/requirement.rb index 96e382d4f..680ed6643 100644 --- a/spec/support/mockers/requirement.rb +++ b/spec/support/mockers/requirement.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true require Rails.root.join('spec/support/mockers/mocker.rb') class RequirementMock < Mocker diff --git a/spec/support/mockers/reservation.rb b/spec/support/mockers/reservation.rb index de4d13c72..4c1f09914 100644 --- a/spec/support/mockers/reservation.rb +++ b/spec/support/mockers/reservation.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true require Rails.root.join('spec/support/mockers/mocker.rb') class ReservationMock < Mocker diff --git a/spec/support/mockers/user.rb b/spec/support/mockers/user.rb index 61e2b4888..1270ac456 100644 --- a/spec/support/mockers/user.rb +++ b/spec/support/mockers/user.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true require Rails.root.join('spec/support/mockers/mocker.rb') class UserMock < Mocker diff --git a/spec/support/shared_examples/controller_examples.rb b/spec/support/shared_examples/controller_examples.rb index 7caff4e44..3d645a7b7 100644 --- a/spec/support/shared_examples/controller_examples.rb +++ b/spec/support/shared_examples/controller_examples.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true require 'spec_helper' shared_examples_for 'successful request' do |template|