From 23657e0c78da01799775b80dd7d0f9dd0c19ffb2 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Fri, 14 Mar 2014 16:01:38 +0000 Subject: [PATCH 1/3] rename #expect_definitions to #stub_shellout and document This method was previously misnamed - we are not setting any expectations, but instead stubbing Mixlib::ShellOut so that its #stdout command returns given values which can then be tested against. Also rename the parameters for clarity, and add some explanatory comments. --- spec/helpers/cib_object.rb | 32 +++++++++++++++++++++---------- spec/helpers/runnable_resource.rb | 12 ++++++------ spec/providers/colocation_spec.rb | 2 +- spec/providers/group_spec.rb | 2 +- spec/providers/primitive_spec.rb | 6 +++--- 5 files changed, 33 insertions(+), 21 deletions(-) diff --git a/spec/helpers/cib_object.rb b/spec/helpers/cib_object.rb index a5de5c8..408e2dd 100644 --- a/spec/helpers/cib_object.rb +++ b/spec/helpers/cib_object.rb @@ -8,20 +8,32 @@ require File.expand_path('../../libraries/pacemaker/cib_object', module Chef::RSpec module Pacemaker module CIBObject - # "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 shellout_double(definition) + # Return a Mixlib::ShellOut double which mimics successful + # execution of a command, returning the given string on STDOUT. + def shellout_double(string) shellout = double(Mixlib::ShellOut) shellout.stub(:environment).and_return({}) shellout.stub(:run_command) shellout.stub(:error!) - expect(shellout).to receive(:stdout).and_return(definition) + expect(shellout).to receive(:stdout).and_return(string) shellout end - def expect_definitions(*definitions) - doubles = definitions.map { |d| shellout_double(d) } + # This stubs Mixlib::ShellOut.new with a sequence of doubles + # whose #stdout methods return a corresponding sequence of strings. + # This allows us to simulate the output of a series of shell + # commands being run via Mixlib::ShellOut. + # + # For example, "crm configure show" is executed by + # #load_current_resource, and again later on for the :create + # action, to see whether to create or modify. So the first + # double in the sequence would return an empty definition if we + # wanted to test creation of a new CIB object, or an existing + # definition if we wanted to test modification of an existing + # one. If the test needs subsequent doubles to return different + # values then stdout_strings can have more than one element. + def stub_shellout(*stdout_strings) + doubles = stdout_strings.map { |string| shellout_double(string) } Mixlib::ShellOut.stub(:new).and_return(*doubles) end end @@ -40,7 +52,7 @@ shared_examples "a CIB object" do end it "should be instantiated via Pacemaker::CIBObject.from_name" do - expect_definitions(fixture.definition_string) + stub_shellout(fixture.definition_string) obj = Pacemaker::CIBObject.from_name(fixture.name) expect_to_match_fixture(obj) end @@ -51,7 +63,7 @@ shared_examples "a CIB object" do end it "should barf if the loaded definition's type is not colocation" do - expect_definitions("clone foo blah blah") + stub_shellout("clone foo blah blah") expect { fixture.load_definition }.to \ raise_error(Pacemaker::CIBObject::TypeMismatch, "Expected #{object_type} type but loaded definition was type clone") @@ -62,7 +74,7 @@ shared_examples "action on non-existent resource" do |action, cmd, expected_erro include Chef::RSpec::Pacemaker::CIBObject it "should not attempt to #{action.to_s} a non-existent resource" do - expect_definitions("") + stub_shellout("") if expected_error expect { provider.run_action action }.to \ diff --git a/spec/helpers/runnable_resource.rb b/spec/helpers/runnable_resource.rb index f99156a..d454557 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_definitions(fixture.definition_string) + stub_shellout(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_definitions(fixture.definition_string) + stub_shellout(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_definitions(fixture.definition_string) + stub_shellout(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_definitions(config) + stub_shellout(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_definitions(fixture.definition_string) + stub_shellout(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_definitions(fixture.definition_string) + stub_shellout(fixture.definition_string) expect_running(true) provider.run_action :stop diff --git a/spec/providers/colocation_spec.rb b/spec/providers/colocation_spec.rb index 0ec3b62..4a0d3fd 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_definitions(fixture.definition_string) + stub_shellout(fixture.definition_string) provider.run_action :create diff --git a/spec/providers/group_spec.rb b/spec/providers/group_spec.rb index 2639fa6..6976876 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_definitions(fixture.definition_string) + stub_shellout(fixture.definition_string) provider.run_action :create diff --git a/spec/providers/primitive_spec.rb b/spec/providers/primitive_spec.rb index fdf73a2..f005698 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_definitions(fixture.definition_string) + stub_shellout(fixture.definition_string) provider.run_action :create @@ -102,7 +102,7 @@ describe "Chef::Provider::PacemakerPrimitive" do # 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) + stub_shellout("", 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_definitions(definition) + stub_shellout(definition) expected_error = \ "Existing #{fixture} has agent '#{existing_agent}' " \ From 74931711641f258c60ca91d79012df0cb8d3fb68 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Fri, 14 Mar 2014 16:19:35 +0000 Subject: [PATCH 2/3] add support for testing scenarios where commands fail --- spec/helpers/cib_object.rb | 44 ++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/spec/helpers/cib_object.rb b/spec/helpers/cib_object.rb index 408e2dd..79c5e5f 100644 --- a/spec/helpers/cib_object.rb +++ b/spec/helpers/cib_object.rb @@ -10,7 +10,7 @@ module Chef::RSpec module CIBObject # Return a Mixlib::ShellOut double which mimics successful # execution of a command, returning the given string on STDOUT. - def shellout_double(string) + def succeeding_shellout_double(string) shellout = double(Mixlib::ShellOut) shellout.stub(:environment).and_return({}) shellout.stub(:run_command) @@ -19,10 +19,38 @@ module Chef::RSpec shellout end + # Return a Mixlib::ShellOut double which mimics failed + # execution of a command, raising an exception when #error! is + # called. We expect #error! to be called, because if it isn't, + # that probably indicates the code isn't robust enough. This + # may need to be relaxed in the future. + def failing_shellout_double(stdout='', stderr='', exitstatus=1) + shellout = double(Mixlib::ShellOut) + shellout.stub(:environment).and_return({}) + shellout.stub(:run_command) + shellout.stub(:stdout).and_return(stdout) + shellout.stub(:stderr).and_return(stderr) + shellout.stub(:exitstatus).and_return(exitstatus) + exception = Mixlib::ShellOut::ShellCommandFailed.new( + "Expected process to exit with 0, " + + "but received '#{exitstatus}'" + ) + expect(shellout).to receive(:error!).and_raise(exception) + shellout + end + # This stubs Mixlib::ShellOut.new with a sequence of doubles - # whose #stdout methods return a corresponding sequence of strings. - # This allows us to simulate the output of a series of shell - # commands being run via Mixlib::ShellOut. + # with a corresponding sequence of behaviours. This allows us + # to simulate the output of a series of shell commands being run + # via Mixlib::ShellOut. Each double either mimics a successful + # command execution whose #stdout method returns the given + # string, or a failed execution with the given exit code and + # STDOUT/STDERR. + # + # results is an Array describing the sequence of behaviours; + # each element is either a string mimicking STDOUT from + # successful command execution, or a [stdout, stderr, exitcode] + # status mimicking command execution failure. # # For example, "crm configure show" is executed by # #load_current_resource, and again later on for the :create @@ -32,8 +60,12 @@ module Chef::RSpec # definition if we wanted to test modification of an existing # one. If the test needs subsequent doubles to return different # values then stdout_strings can have more than one element. - def stub_shellout(*stdout_strings) - doubles = stdout_strings.map { |string| shellout_double(string) } + def stub_shellout(*results) + doubles = results.map { |result| + result.is_a?(String) ? + succeeding_shellout_double(result) + : failing_shellout_double(*result) + } Mixlib::ShellOut.stub(:new).and_return(*doubles) end end From 4febff5dc70e10510045da9f46907a54ab3bd018 Mon Sep 17 00:00:00 2001 From: Adam Spiers Date: Fri, 14 Mar 2014 15:33:53 +0000 Subject: [PATCH 3/3] handle creation errors more robustly We have seen strange cases where creation of a new CIB object apparently failed: http://pastebin.suse.de/9346 Even though this is strange, we should certainly expect Pacemaker::CIBObject.from_name to return nil in a few cases, so we need to handle that properly. --- .../mixin/pacemaker/standard_cib_object.rb | 15 ++++++++----- spec/providers/primitive_spec.rb | 22 +++++++++++++++++++ 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/libraries/chef/mixin/pacemaker/standard_cib_object.rb b/libraries/chef/mixin/pacemaker/standard_cib_object.rb index 45fd0c6..0490d9b 100644 --- a/libraries/chef/mixin/pacemaker/standard_cib_object.rb +++ b/libraries/chef/mixin/pacemaker/standard_cib_object.rb @@ -46,13 +46,16 @@ 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}" - else - ::Chef::Log.error "Failed to configure #{cib_object}" + created_cib_object = Pacemaker::CIBObject.from_name(new_resource.name) + + raise "Failed to create #{cib_object}" if created_cib_object.nil? + unless created_cib_object.exists? + # This case seems pretty unlikely + raise "Definition missing for #{created_cib_object} after creation" end + + new_resource.updated_by_last_action(true) + ::Chef::Log.info "Successfully configured #{created_cib_object}" end def standard_delete_resource diff --git a/spec/providers/primitive_spec.rb b/spec/providers/primitive_spec.rb index f005698..b6cd746 100644 --- a/spec/providers/primitive_spec.rb +++ b/spec/providers/primitive_spec.rb @@ -110,6 +110,28 @@ describe "Chef::Provider::PacemakerPrimitive" do expect(@resource).to be_updated end + it "should barf if crm fails to create the primitive" do + stub_shellout("", ["crm configure failed", "oh noes", 3]) + + expect { provider.run_action :create }.to \ + raise_error(RuntimeError, "Failed to create #{fixture}") + + expect(@chef_run).to run_execute(fixture.crm_configure_command) + expect(@resource).not_to be_updated + end + + # This scenario seems rather artificial and unlikely, but it doesn't + # do any harm to test it. + it "should barf if crm creates a primitive with empty definition" do + stub_shellout("", "") + + expect { provider.run_action :create }.to \ + raise_error(RuntimeError, "Failed to create #{fixture}") + + expect(@chef_run).to run_execute(fixture.crm_configure_command) + expect(@resource).not_to be_updated + end + 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)