From 162de47f9507a3e8b1b3da1747fe184b66456407 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Fri, 7 Feb 2014 01:09:55 +0000 Subject: [PATCH] fix buggy verification of primitive resource creation Newly created primitive resources weren't being marked as updated in real runs, because exists? was being run on an old object. Furthermore, the tests didn't catch this because #exists? on any instance of Pacemaker::Resource::Primitive was being mocked to return true. The fix requires re-invoking Pacemaker::CIBObject.from_name after attempting to create the new primitive, so that cib_object.exists? accurately reflects whether the creation succeeded. However, testing this required mocking Mixlib::Shellout#stdout to first return "" and then secondly a definition string for the created resource, and unfortunately this exposed a nasty bug in rspec-mocks: https://github.com/rspec/rspec-mocks/issues/559 So we revamp the mocking of Mixlib::Shellout#stdout to use doubles instead of #expect_any_instance_of. --- .../mixin/pacemaker/standard_cib_object.rb | 1 + spec/helpers/cib_object.rb | 33 ++++++++----------- spec/helpers/runnable_resource.rb | 12 +++---- .../pacemaker/resource/primitive_spec.rb | 4 --- spec/providers/colocation_spec.rb | 2 +- spec/providers/group_spec.rb | 2 +- spec/providers/primitive_spec.rb | 12 +++---- 7 files changed, 28 insertions(+), 38 deletions(-) diff --git a/libraries/chef/mixin/pacemaker/standard_cib_object.rb b/libraries/chef/mixin/pacemaker/standard_cib_object.rb index 6c4a383..592171a 100644 --- a/libraries/chef/mixin/pacemaker/standard_cib_object.rb +++ b/libraries/chef/mixin/pacemaker/standard_cib_object.rb @@ -45,6 +45,7 @@ class Chef action :nothing end.run_action(:run) + cib_object = Pacemaker::CIBObject.from_name(new_resource.name) if cib_object.exists? new_resource.updated_by_last_action(true) ::Chef::Log.info "Successfully configured #{cib_object}" diff --git a/spec/helpers/cib_object.rb b/spec/helpers/cib_object.rb index 0d3bb50..39c9c15 100644 --- a/spec/helpers/cib_object.rb +++ b/spec/helpers/cib_object.rb @@ -15,11 +15,7 @@ shared_examples "a CIB object" do end it "should be instantiated via Pacemaker::CIBObject.from_name" do - Mixlib::ShellOut.any_instance.stub(:error!) - expect_any_instance_of(Mixlib::ShellOut) \ - .to receive(:stdout) \ - .and_return(fixture.definition_string) - + expect_definitions(fixture.definition_string) obj = Pacemaker::CIBObject.from_name(fixture.name) expect_to_match_fixture(obj) end @@ -30,10 +26,7 @@ shared_examples "a CIB object" do end it "should barf if the loaded definition's type is not colocation" do - Mixlib::ShellOut.any_instance.stub(:error!) - expect_any_instance_of(Mixlib::ShellOut) \ - .to receive(:stdout) \ - .and_return("clone foo blah blah") + expect_definitions("clone foo blah blah") expect { fixture.load_definition }.to \ raise_error(Pacemaker::CIBObject::TypeMismatch, "Expected #{object_type} type but loaded definition was type clone") @@ -42,7 +35,7 @@ end shared_examples "action on non-existent resource" do |action, cmd, expected_error| it "should not attempt to #{action.to_s} a non-existent resource" do - expect_definition("") + expect_definitions("") if expected_error expect { provider.run_action action }.to \ @@ -62,18 +55,18 @@ module Chef::RSpec # "crm configure show" is executed by load_current_resource, and # again later on for the :create action, to see whether to create or # modify. - def expect_definition(definition) - Mixlib::ShellOut.any_instance.stub(:run_command) - Mixlib::ShellOut.any_instance.stub(:error!) - expect_any_instance_of(Mixlib::ShellOut) \ - .to receive(:stdout) \ - .and_return(definition) + def shellout_double(definition) + shellout = double(Mixlib::ShellOut) + shellout.stub(:environment).and_return({}) + shellout.stub(:run_command) + shellout.stub(:error!) + expect(shellout).to receive(:stdout).and_return(definition) + shellout end - def expect_exists(exists) - expect_any_instance_of(cib_object_class) \ - .to receive(:exists?) \ - .and_return(exists) + def expect_definitions(*definitions) + doubles = definitions.map { |d| shellout_double(d) } + Mixlib::ShellOut.stub(:new).and_return(*doubles) end end end diff --git a/spec/helpers/runnable_resource.rb b/spec/helpers/runnable_resource.rb index f140111..f99156a 100644 --- a/spec/helpers/runnable_resource.rb +++ b/spec/helpers/runnable_resource.rb @@ -21,7 +21,7 @@ shared_examples "a runnable resource" do |fixture| :delete, "crm configure delete #{fixture.name}", nil it "should not delete a running resource" do - expect_definition(fixture.definition_string) + expect_definitions(fixture.definition_string) expect_running(true) expected_error = "Cannot delete running #{fixture}" @@ -34,7 +34,7 @@ shared_examples "a runnable resource" do |fixture| end it "should delete a non-running resource" do - expect_definition(fixture.definition_string) + expect_definitions(fixture.definition_string) expect_running(false) provider.run_action :delete @@ -52,7 +52,7 @@ shared_examples "a runnable resource" do |fixture| "Cannot start non-existent #{fixture}" it "should do nothing to a started resource" do - expect_definition(fixture.definition_string) + expect_definitions(fixture.definition_string) expect_running(true) provider.run_action :start @@ -64,7 +64,7 @@ shared_examples "a runnable resource" do |fixture| it "should start a stopped resource" do config = fixture.definition_string.sub("Started", "Stopped") - expect_definition(config) + expect_definitions(config) expect_running(false) provider.run_action :start @@ -82,7 +82,7 @@ shared_examples "a runnable resource" do |fixture| "Cannot stop non-existent #{fixture}" it "should do nothing to a stopped resource" do - expect_definition(fixture.definition_string) + expect_definitions(fixture.definition_string) expect_running(false) provider.run_action :stop @@ -93,7 +93,7 @@ shared_examples "a runnable resource" do |fixture| end it "should stop a started resource" do - expect_definition(fixture.definition_string) + expect_definitions(fixture.definition_string) expect_running(true) provider.run_action :stop diff --git a/spec/libraries/pacemaker/resource/primitive_spec.rb b/spec/libraries/pacemaker/resource/primitive_spec.rb index 8556472..ffd33df 100644 --- a/spec/libraries/pacemaker/resource/primitive_spec.rb +++ b/spec/libraries/pacemaker/resource/primitive_spec.rb @@ -14,10 +14,6 @@ describe Pacemaker::Resource::Primitive do Chef::RSpec::Pacemaker::Config::KEYSTONE_PRIMITIVE_DEFINITION } - before(:each) do - Mixlib::ShellOut.any_instance.stub(:run_command) - end - def object_type 'primitive' end diff --git a/spec/providers/colocation_spec.rb b/spec/providers/colocation_spec.rb index 80217d6..0ec3b62 100644 --- a/spec/providers/colocation_spec.rb +++ b/spec/providers/colocation_spec.rb @@ -35,7 +35,7 @@ describe "Chef::Provider::PacemakerColocation" do def test_modify(expected_cmds) yield - expect_definition(fixture.definition_string) + expect_definitions(fixture.definition_string) provider.run_action :create diff --git a/spec/providers/group_spec.rb b/spec/providers/group_spec.rb index fbc28b6..2639fa6 100644 --- a/spec/providers/group_spec.rb +++ b/spec/providers/group_spec.rb @@ -36,7 +36,7 @@ describe "Chef::Provider::PacemakerGroup" do def test_modify(expected_cmds) yield - expect_definition(fixture.definition_string) + expect_definitions(fixture.definition_string) provider.run_action :create diff --git a/spec/providers/primitive_spec.rb b/spec/providers/primitive_spec.rb index 7316abb..fdf73a2 100644 --- a/spec/providers/primitive_spec.rb +++ b/spec/providers/primitive_spec.rb @@ -38,7 +38,7 @@ describe "Chef::Provider::PacemakerPrimitive" do def test_modify(expected_cmds) yield - expect_definition(fixture.definition_string) + expect_definitions(fixture.definition_string) provider.run_action :create @@ -99,10 +99,10 @@ describe "Chef::Provider::PacemakerPrimitive" do end it "should create a primitive if it doesn't already exist" do - expect_definition("") - # Later, the :create action calls cib_object_class#exists? to check - # that creation succeeded. - expect_exists(true) + # The first time, Mixlib::ShellOut needs to return an empty definition. + # Then the resource gets created so the second time it needs to return + # the definition used for creation. + expect_definitions("", fixture.definition_string) provider.run_action :create @@ -113,7 +113,7 @@ describe "Chef::Provider::PacemakerPrimitive" do it "should barf if the primitive is already defined with the wrong agent" do existing_agent = "ocf:openstack:something-else" definition = fixture.definition_string.sub(fixture.agent, existing_agent) - expect_definition(definition) + expect_definitions(definition) expected_error = \ "Existing #{fixture} has agent '#{existing_agent}' " \