This revised patchset adds Rspec tests for the templates (very basic). @jayg: I've changed the local variable to an instance variable in the destroy action since I'm returning that data back to the client so it was needed. The mapping from /api/hardware_profiles to /hardware_profiles is temporary. It is needed to test the cli functionality. It can be removed later
--- .../hardware_profiles/_hardware_profile.xml.haml | 7 +++++++ src/app/views/hardware_profiles/create.xml.haml | 2 ++ src/app/views/hardware_profiles/destroy.xml.haml | 2 ++ src/app/views/hardware_profiles/index.xml.haml | 4 ++++ src/app/views/hardware_profiles/show.xml.haml | 2 ++ 5 files changed, 17 insertions(+), 0 deletions(-) create mode 100644 src/app/views/hardware_profiles/_hardware_profile.xml.haml create mode 100644 src/app/views/hardware_profiles/create.xml.haml create mode 100644 src/app/views/hardware_profiles/destroy.xml.haml create mode 100644 src/app/views/hardware_profiles/index.xml.haml create mode 100644 src/app/views/hardware_profiles/show.xml.haml
diff --git a/src/app/views/hardware_profiles/_hardware_profile.xml.haml b/src/app/views/hardware_profiles/_hardware_profile.xml.haml new file mode 100644 index 0000000..5cd1e1b --- /dev/null +++ b/src/app/views/hardware_profiles/_hardware_profile.xml.haml @@ -0,0 +1,7 @@ +!!!XML +%hardware_profile{:id => hwp.id, :href => hardware_profile_url(hwp.id)} + %name= hwp.name + %memory{:id => hwp.memory.id, :unit => hwp.memory.unit}= hwp.memory.value.nil? ? "nil" : hwp.memory.value + %storage{:id => hwp.storage.id, :unit => hwp.storage.unit}= hwp.storage.value.nil? ? "nil" : hwp.storage.value + %cpu{:id => hwp.cpu.id, :unit => hwp.cpu.unit}= hwp.cpu.value.nil? ? "nil" : hwp.cpu.value + %architecture{:id => hwp.architecture.id, :unit => hwp.architecture.unit}= hwp.architecture.value.nil? ? "nil" : hwp.architecture.value diff --git a/src/app/views/hardware_profiles/create.xml.haml b/src/app/views/hardware_profiles/create.xml.haml new file mode 100644 index 0000000..b0ae305 --- /dev/null +++ b/src/app/views/hardware_profiles/create.xml.haml @@ -0,0 +1,2 @@ +!!! XML += render :partial => "hardware_profile", :locals => {:hwp => @hardware_profile} diff --git a/src/app/views/hardware_profiles/destroy.xml.haml b/src/app/views/hardware_profiles/destroy.xml.haml new file mode 100644 index 0000000..b9370dc --- /dev/null +++ b/src/app/views/hardware_profiles/destroy.xml.haml @@ -0,0 +1,2 @@ +!!!XML += render :partial => "hardware_profile", :locals => {:hwp => @hardware_profile} diff --git a/src/app/views/hardware_profiles/index.xml.haml b/src/app/views/hardware_profiles/index.xml.haml new file mode 100644 index 0000000..06bd3ca --- /dev/null +++ b/src/app/views/hardware_profiles/index.xml.haml @@ -0,0 +1,4 @@ +!!!XML +%hardware_profiles + - @hardware_profiles.each do |hwp| + = render :partial => 'hardware_profile', :locals => {:hwp => hwp} diff --git a/src/app/views/hardware_profiles/show.xml.haml b/src/app/views/hardware_profiles/show.xml.haml new file mode 100644 index 0000000..b9370dc --- /dev/null +++ b/src/app/views/hardware_profiles/show.xml.haml @@ -0,0 +1,2 @@ +!!!XML += render :partial => "hardware_profile", :locals => {:hwp => @hardware_profile}
--- .../controllers/hardware_profiles_controller.rb | 22 ++++++++++++++----- 1 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/src/app/controllers/hardware_profiles_controller.rb b/src/app/controllers/hardware_profiles_controller.rb index 13e6887..52532dc 100644 --- a/src/app/controllers/hardware_profiles_controller.rb +++ b/src/app/controllers/hardware_profiles_controller.rb @@ -27,6 +27,7 @@ class HardwareProfilesController < ApplicationController @params = params set_admin_content_tabs 'hardware_profiles' respond_to do |format| + format.xml format.html format.js do if params[:hardware_profile] @@ -58,6 +59,7 @@ class HardwareProfilesController < ApplicationController save_breadcrumb(hardware_profile_path(@hardware_profile), @hardware_profile.name)
respond_to do |format| + format.xml format.html format.js do if params.delete :details_pane @@ -89,7 +91,10 @@ class HardwareProfilesController < ApplicationController end
if @hardware_profile.save - redirect_to hardware_profiles_path + respond_to do |format| + format.xml + format.html {redirect_to hardware_profiles_path} + end else render :action => 'new' end @@ -104,15 +109,20 @@ class HardwareProfilesController < ApplicationController end
def destroy - hardware_profile = HardwareProfile.find(params[:id]) - require_privilege(Privilege::MODIFY, hardware_profile) - if hardware_profile.provider_hardware_profile? + @hardware_profile = HardwareProfile.find(params[:id]) + require_privilege(Privilege::MODIFY, @hardware_profile) + if @hardware_profile.provider_hardware_profile? flash[:warning] = t "hardware_profiles.flash.warning.cannot_delete_backend_hwp" - redirect_to hardware_profile_path(hardware_profile) + redirect_to hardware_profile_path(@hardware_profile) return end - if hardware_profile.destroy + if @hardware_profile.destroy flash[:notice] = t "hardware_profiles.flash.notice.deleted" + respond_to do |format| + format.xml + format.html { redirect_to hardware_profiles_path } + end + return else flash[:error] = t "hardware_profiles.flash.error.not_deleted" end
--- .../hardware_profiles_controller_spec.rb | 328 +++++++++++++++----- 1 files changed, 242 insertions(+), 86 deletions(-)
diff --git a/src/spec/controllers/hardware_profiles_controller_spec.rb b/src/spec/controllers/hardware_profiles_controller_spec.rb index 00bd3d9..0e76ba0 100644 --- a/src/spec/controllers/hardware_profiles_controller_spec.rb +++ b/src/spec/controllers/hardware_profiles_controller_spec.rb @@ -18,122 +18,278 @@ require 'spec_helper'
describe HardwareProfilesController do
- fixtures :all - before(:each) do - @admin_permission = FactoryGirl.create :admin_permission - @admin = @admin_permission.user + render_views + + shared_examples_for "having XML with hwps" do + # TODO: implement more attributes checks + subject { Nokogiri::XML(response.body) } + context "list of hardware_profiles" do + let(:xml_hwps) { subject.xpath('//hardware_profiles/hardware_profile') } + context "number of hardware_profiles" do + it { xml_hwps.size.should be_eql(number_of_hardware_profiles) } + end + it "should have correct hardware_profiles" do + hardware_profiles.each do |hardware_profile| + xml_hwp = xml_hwps.xpath("//hardware_profile[@id="#{hardware_profile.id}"]") + xml_hwp.xpath('name').text.should be_eql(hardware_profile.name.to_s) + xml_hwp.xpath('@href').text.should be_eql(hardware_profile_url(hardware_profile)) + end + end + end end
- describe "Authorization" do + context "UI" do + fixtures :all + before(:each) do + @admin_permission = FactoryGirl.create :admin_permission + @admin = @admin_permission.user + end
- context "Admin" do - it "should provide ui to view all hardware profiles" do - mock_warden(@admin) - @request.accept = "text/html" - get :index - response.should be_success - assigns[:hardware_profiles].size.should == HardwareProfile.count - response.should render_template("index") - end + describe "Authorization" do + + context "Admin" do + it "should provide ui to view all hardware profiles" do + mock_warden(@admin) + @request.accept = "text/html" + get :index + response.should be_success + assigns[:hardware_profiles].size.should == HardwareProfile.count + response.should render_template("index") + end
- it "should be able to create hardware profiles" do - mock_warden(@admin) - lambda do - post :create, :commit => 'Save', :hardware_profile => { - :name => 'test', + it "should be able to create hardware profiles" do + mock_warden(@admin) + lambda do + post :create, :commit => 'Save', :hardware_profile => { + :name => 'test', + :memory_attributes => FactoryGirl.attributes_for(:mock_hwp1_memory), + :cpu_attributes => FactoryGirl.attributes_for(:mock_hwp1_cpu), + :storage_attributes => FactoryGirl.attributes_for(:mock_hwp1_storage), + :architecture_attributes => FactoryGirl.attributes_for(:mock_hwp2_memory), + } + end.should change(HardwareProfile, :count).by(1) + HardwareProfile.find_by_name('test').should_not be_nil + response.should redirect_to(hardware_profiles_path) + end + + it "should be able to edit hardware profiles" do + hardware_profile = Factory.create :hardware_profile + mock_warden(@admin) + put :update, :id => hardware_profile.id, :hardware_profile => { + :name => 'updated hwp', :memory_attributes => FactoryGirl.attributes_for(:mock_hwp1_memory), :cpu_attributes => FactoryGirl.attributes_for(:mock_hwp1_cpu), :storage_attributes => FactoryGirl.attributes_for(:mock_hwp1_storage), :architecture_attributes => FactoryGirl.attributes_for(:mock_hwp2_memory), } - end.should change(HardwareProfile, :count).by(1) - HardwareProfile.find_by_name('test').should_not be_nil - response.should redirect_to(hardware_profiles_path) - end + HardwareProfile.find_by_name('updated hwp').should_not be_nil + response.should redirect_to(hardware_profiles_path) + end
- it "should be able to edit hardware profiles" do - hardware_profile = Factory.create :hardware_profile - mock_warden(@admin) - put :update, :id => hardware_profile.id, :hardware_profile => { - :name => 'updated hwp', - :memory_attributes => FactoryGirl.attributes_for(:mock_hwp1_memory), - :cpu_attributes => FactoryGirl.attributes_for(:mock_hwp1_cpu), - :storage_attributes => FactoryGirl.attributes_for(:mock_hwp1_storage), - :architecture_attributes => FactoryGirl.attributes_for(:mock_hwp2_memory), - } - HardwareProfile.find_by_name('updated hwp').should_not be_nil - response.should redirect_to(hardware_profiles_path) - end + it "should be able to delete hardware profiles" do + hardware_profile = Factory.create :hardware_profile + mock_warden(@admin)
- it "should be able to delete hardware profiles" do - hardware_profile = Factory.create :hardware_profile - mock_warden(@admin) + HardwareProfile.exists?(hardware_profile.id).should be_true + delete :destroy, :id => hardware_profile.id + HardwareProfile.exists?(hardware_profile.id).should be_false + response.should redirect_to(hardware_profiles_path) + end
- HardwareProfile.exists?(hardware_profile.id).should be_true - delete :destroy, :id => hardware_profile.id - HardwareProfile.exists?(hardware_profile.id).should be_false - response.should redirect_to(hardware_profiles_path) end
- end
+ context "Unauthorized user" do + before(:each) do + @user_permission = FactoryGirl.create :pool_user_permission + @user = @user_permission.user + end
- context "Unauthorized user" do - before(:each) do - @user_permission = FactoryGirl.create :pool_user_permission - @user = @user_permission.user - end + it "should not list hw profiles which I'm not allowed to see" do + hardware_profile = Factory.create :hardware_profile + mock_warden(@user) + @request.accept = "text/html" + get :index + response.should be_success + assigns[:hardware_profiles].find {|p| p.name == hardware_profile.name}.should be_nil + response.should render_template("index") + end
- it "should not list hw profiles which I'm not allowed to see" do - hardware_profile = Factory.create :hardware_profile - mock_warden(@user) - @request.accept = "text/html" - get :index - response.should be_success - assigns[:hardware_profiles].find {|p| p.name == hardware_profile.name}.should be_nil - response.should render_template("index") - end + it "should not be able to create hardware profiles" do + mock_warden(@user) + lambda do + post :create, :commit => 'Save', :hardware_profile => { + :name => 'test', + :memory_attributes => FactoryGirl.attributes_for(:mock_hwp1_memory), + :cpu_attributes => FactoryGirl.attributes_for(:mock_hwp1_cpu), + :storage_attributes => FactoryGirl.attributes_for(:mock_hwp1_storage), + :architecture_attributes => FactoryGirl.attributes_for(:mock_hwp2_memory), + } + end.should_not change(HardwareProfile, :count) + HardwareProfile.find_by_name('test').should be_nil + response.should render_template('layouts/error') + end
- it "should not be able to create hardware profiles" do - mock_warden(@user) - lambda do - post :create, :commit => 'Save', :hardware_profile => { - :name => 'test', + it "should not be able to edit hardware profiles" do + hardware_profile = Factory.create :hardware_profile + mock_warden(@user) + put :update, :id => hardware_profile.id, :hardware_profile => { + :name => 'updated hwp', :memory_attributes => FactoryGirl.attributes_for(:mock_hwp1_memory), :cpu_attributes => FactoryGirl.attributes_for(:mock_hwp1_cpu), :storage_attributes => FactoryGirl.attributes_for(:mock_hwp1_storage), :architecture_attributes => FactoryGirl.attributes_for(:mock_hwp2_memory), } - end.should_not change(HardwareProfile, :count) - HardwareProfile.find_by_name('test').should be_nil - response.should render_template('layouts/error') + HardwareProfile.find_by_name('updated hwp').should be_nil + response.should render_template('layouts/error') + end + + + it "should not be able to delete hardware profiles" do + hardware_profile = Factory.create :hardware_profile + mock_warden(@user) + + HardwareProfile.exists?(hardware_profile.id).should be_true + delete :destroy, :id => hardware_profile.id + HardwareProfile.exists?(hardware_profile.id).should be_true + response.should render_template('layouts/error') + end end
- it "should not be able to edit hardware profiles" do - hardware_profile = Factory.create :hardware_profile - mock_warden(@user) - put :update, :id => hardware_profile.id, :hardware_profile => { - :name => 'updated hwp', - :memory_attributes => FactoryGirl.attributes_for(:mock_hwp1_memory), - :cpu_attributes => FactoryGirl.attributes_for(:mock_hwp1_cpu), - :storage_attributes => FactoryGirl.attributes_for(:mock_hwp1_storage), - :architecture_attributes => FactoryGirl.attributes_for(:mock_hwp2_memory), - } - HardwareProfile.find_by_name('updated hwp').should be_nil - response.should render_template('layouts/error') + end + end + + context "API" do + context "when requesting XML" do + + before(:each) do + accept_xml end
+ context "when using admin credentials" do + before(:each) do + @admin_permission = FactoryGirl.create :admin_permission + @admin = @admin_permission.user + mock_warden(@admin) + end + + describe "#show" do + context "when requested hardware profile exists" do + + before(:each) do + get :show, :id => 1 + end + + it_behaves_like "http OK" + it_behaves_like "responding with XML"
- it "should not be able to delete hardware profiles" do - hardware_profile = Factory.create :hardware_profile - mock_warden(@user) + context "XML body" do + # TODO: implement more attributes checks + subject { Nokogiri::XML(response.body) } + it { + subject.xpath("//hardware_profile[@id]").text.should be_eql(1.to_s) + }
- HardwareProfile.exists?(hardware_profile.id).should be_true - delete :destroy, :id => hardware_profile.id - HardwareProfile.exists?(hardware_profile.id).should be_true - response.should render_template('layouts/error') + end + end + + context "when requested hardware profile does not exist" do + + before(:each) do + h = HardwareProfile.find_by_id(7) + h.delete if h + get :show, :id => 7 + end + + it_behaves_like "http Not Found" + it_behaves_like "responding with XML" + + context "XML body" do + + subject { Nokogiri::XML(response.body) } + + it { + subject.xpath('//error').size.should be_eql(1) + subject.xpath('//error/code').text.should be_eql('RecordNotFound') + subject.xpath('//error/message').text.should be_eql("Couldn't find HardwareProfile with ID=7") + } + end + end + end + + describe "#create" do + before(:each) do + post :create, :commit => 'Save', :hardware_profile => { + :name => 'test', + :memory_attributes => FactoryGirl.attributes_for(:mock_hwp1_memory), + :cpu_attributes => FactoryGirl.attributes_for(:mock_hwp1_cpu), + :storage_attributes => FactoryGirl.attributes_for(:mock_hwp1_storage), + :architecture_attributes => FactoryGirl.attributes_for(:mock_hwp2_memory), + } + end + + context "with correct parameters" do + let(:hwp) { FactoryGirl.build(:hardware_profile) } + + it_behaves_like "http OK" + it_behaves_like "responding with XML" + end + + context "with incorrect parameters" do + let(:hwp) { FactoryGirl.build(:invalid_hwp) } + + it_behaves_like "http Bad Request" + it_behaves_like "responding with XML" + + context "XML body" do + subject { Nokogiri::XML(response.body) } + it "should have some errors" do + subject.xpath('//errors').size.should be_eql(1) + subject.xpath('//errors/error').size.should <= 1 + end + end + end + end + + describe "#destroy" do + + let(:hwp) { Factory.create(:hardware_profile) } + + context "existing hwp" do + + before(:each) do + delete :destroy, :id => hwp.id + end + + it_behaves_like "http OK" + it_behaves_like "responding with XML" + + it { expect { hwp.reload }.to raise_error(ActiveRecord::RecordNotFound) } + + end + + context "non existing hwp" do + + before(:each) do + hwp.delete + delete :destroy, :id => hwp.id + end + + it_behaves_like "http Not Found" + it_behaves_like "responding with XML" + + context "XML body" do + subject { Nokogiri::XML(response.body) } + + it { + subject.xpath('//error').size.should be_eql(1) + subject.xpath('//error/code').text.should be_eql('RecordNotFound') + subject.xpath('//error/message').text.should be_eql("Couldn't find HardwareProfile with ID=#{hwp.id}") + } + end + end + end end end - end end
On 17/07/12 22:39 +0530, Samridh wrote:
.../hardware_profiles_controller_spec.rb | 328 +++++++++++++++----- 1 files changed, 242 insertions(+), 86 deletions(-)
I really hate to have to nack this again, but there are some problems here, namely there are a bunch of tests that pass in magic numbers for IDs. Instead, the test should create the object via a factory, and use the id from that object, which will be dynamic. Alternatively, mock_models could be used, but as we have not refactored the tests to do much of that yet, I think a factory would be reasonable. Also, when I run the new specs, I get:
rspec ./spec/support/shared_examples_for_api.rb:20 # HardwareProfilesController API when requesting XML when using admin credentials#show when requested hardware profile exists behaves like http OK response status code
rspec ./spec/controllers/hardware_profiles_controller_spec.rb:189 # HardwareProfilesController API when requesting XML when using admin credentials#show when requested hardware profile exists XML body
rspec ./spec/support/shared_examples_for_api.rb:27 # HardwareProfilesController API when requesting XML when using admin credentials#create with incorrect parameters behaves like http Bad Request response status code
rspec ./spec/controllers/hardware_profiles_controller_spec.rb:246 # HardwareProfilesController API when requesting XML when using admin credentials#create with incorrect parameters XML body should have some errors
rspec ./spec/support/shared_examples_for_api.rb:20 # HardwareProfilesController API when requesting XML when using admin credentials#destroy existing hwp behaves like http OK response status code
The shared examples are likely just results of the 2 actual test failures, but I am wondering how these tests passed for you and not for me? Aside from this, the patchset looks pretty good overall.
-j
--- src/config/routes.rb | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/config/routes.rb b/src/config/routes.rb index e7f26c5..88dc24a 100644 --- a/src/config/routes.rb +++ b/src/config/routes.rb @@ -285,6 +285,7 @@ Conductor::Application.routes.draw do end resources :provider_accounts, :only => [:index, :show] resources :provider_types, :only => [:index, :show] + resources :hardware_profiles, :only => [:index, :show, :destroy, :create] end
#match 'matching_profiles', :to => '/hardware_profiles/matching_profiles/:hardware_profile_id/provider/:provider_id', :controller => 'hardware_profiles', :action => 'matching_profiles', :conditions => { :method => :get }, :as =>'matching_profiles'
aeolus-devel@lists.fedorahosted.org
-
Jason Guiditta -
Samridh