From 7cb884e44db412ed4cc19d9eb3e07d4b5b17f6b3 Mon Sep 17 00:00:00 2001 From: Matt Robinson Date: Tue, 15 Feb 2011 17:10:42 -0800 Subject: (#6346) Move the trap calls onto Signal so they're easier to stub Once you stub signal traps in tests, you can hit ctrl+c in the middle of a spec run and it will stop the run instead of puppet catching the SIGINT. I had trouble easily tracking down all the places to stub traps when the trap was being called as a private method on applications and daemons, but calling trap on Signal is equivalent since Kernel calls Signal.trap and Object mixes in Kernel to provide trap as a private method on all objects. A bigger solution would be to refactor everywhere we call trap into a method that's called consistently since right now we sprinkle SIGINT and SIGTERM trap handling over applications and daemons in inconsistent ways, returning different error codes and using different messages. I've captured this info in ticket #6345. Reviewed-by: Jacob Helwig --- spec/unit/application/agent_spec.rb | 6 +----- spec/unit/application/apply_spec.rb | 3 +-- spec/unit/application/filebucket_spec.rb | 2 +- spec/unit/application/queue_spec.rb | 6 +----- 4 files changed, 4 insertions(+), 13 deletions(-) (limited to 'spec/unit/application') diff --git a/spec/unit/application/agent_spec.rb b/spec/unit/application/agent_spec.rb index ff504eedf..9fc7879c9 100755 --- a/spec/unit/application/agent_spec.rb +++ b/spec/unit/application/agent_spec.rb @@ -50,12 +50,8 @@ describe Puppet::Application::Agent do end describe "in preinit" do - before :each do - @puppetd.stubs(:trap) - end - it "should catch INT" do - @puppetd.expects(:trap).with { |arg,block| arg == :INT } + Signal.expects(:trap).with { |arg,block| arg == :INT } @puppetd.preinit end diff --git a/spec/unit/application/apply_spec.rb b/spec/unit/application/apply_spec.rb index 4e1744206..ceba4a333 100755 --- a/spec/unit/application/apply_spec.rb +++ b/spec/unit/application/apply_spec.rb @@ -52,7 +52,6 @@ describe Puppet::Application::Apply do before :each do Puppet::Log.stubs(:newdestination) - Puppet.stubs(:trap) Puppet::Log.stubs(:level=) Puppet.stubs(:parse_config) Puppet::FileBucket::Dipper.stubs(:new) @@ -78,7 +77,7 @@ describe Puppet::Application::Apply do end it "should set INT trap" do - @apply.expects(:trap).with(:INT) + Signal.expects(:trap).with(:INT) @apply.setup end diff --git a/spec/unit/application/filebucket_spec.rb b/spec/unit/application/filebucket_spec.rb index e6272f179..95135c7eb 100644 --- a/spec/unit/application/filebucket_spec.rb +++ b/spec/unit/application/filebucket_spec.rb @@ -56,7 +56,7 @@ describe Puppet::Application::Filebucket do end it "should trap INT" do - @filebucket.expects(:trap).with(:INT) + Signal.expects(:trap).with(:INT) @filebucket.setup end diff --git a/spec/unit/application/queue_spec.rb b/spec/unit/application/queue_spec.rb index bd0d53ab1..f8ebbd0b4 100755 --- a/spec/unit/application/queue_spec.rb +++ b/spec/unit/application/queue_spec.rb @@ -29,12 +29,8 @@ describe Puppet::Application::Queue do end describe "in preinit" do - before :each do - @queue.stubs(:trap) - end - it "should catch INT" do - @queue.expects(:trap).with { |arg,block| arg == :INT } + Signal.expects(:trap).with { |arg,block| arg == :INT } @queue.preinit end -- cgit From de6a2052c2aeda1cd76ba828936a9d6f0ac7e907 Mon Sep 17 00:00:00 2001 From: Daniel Pittman Date: Tue, 22 Feb 2011 11:18:19 -0800 Subject: (#5552) Clean up subcommand handling inside puppet cert. We now have a regular, testable mechanism for handling the legacy '--' version of subcommands, as well as a modern bareword subcommand pattern. This makes it sensible to test command handling and avoid regressions. We identified a few quirks in the command line as part of this process. Pair-With: Jesse Wolfe Signed-off-by: Daniel Pittman --- spec/unit/application/cert_spec.rb | 58 +++++++++++++++++++++++++++++--------- 1 file changed, 44 insertions(+), 14 deletions(-) (limited to 'spec/unit/application') diff --git a/spec/unit/application/cert_spec.rb b/spec/unit/application/cert_spec.rb index 4663fc938..2f57d07a9 100755 --- a/spec/unit/application/cert_spec.rb +++ b/spec/unit/application/cert_spec.rb @@ -51,7 +51,7 @@ describe Puppet::Application::Cert do it "should set cert_mode to :destroy for --clean" do @cert_app.handle_clean(0) - @cert_app.cert_mode.should == :destroy + @cert_app.subcommand.should == :destroy end it "should set all to true for --all" do @@ -68,7 +68,7 @@ describe Puppet::Application::Cert do it "should set cert_mode to #{method} with option --#{method}" do @cert_app.send("handle_#{method}".to_sym, nil) - @cert_app.cert_mode.should == method + @cert_app.subcommand.should == method end end @@ -114,19 +114,19 @@ describe Puppet::Application::Cert do end it "should set the ca_location to :local if the cert_mode is generate" do - @cert_app.find_mode('--generate') + @cert_app.subcommand = 'generate' Puppet::SSL::Host.expects(:ca_location=).with(:local) @cert_app.setup end it "should set the ca_location to :local if the cert_mode is destroy" do - @cert_app.find_mode('--destroy') + @cert_app.subcommand = 'destroy' Puppet::SSL::Host.expects(:ca_location=).with(:local) @cert_app.setup end it "should set the ca_location to :only if the cert_mode is print" do - @cert_app.find_mode('--print') + @cert_app.subcommand = 'print' Puppet::SSL::Host.expects(:ca_location=).with(:only) @cert_app.setup end @@ -171,24 +171,54 @@ describe Puppet::Application::Cert do @cert_app.main end - it "should delegate to ca.apply with current set cert_mode" do - @cert_app.cert_mode = "currentmode" + it "should revoke cert if cert_mode is clean" do + @cert_app.subcommand = :destroy @cert_app.command_line.stubs(:args).returns(["host"]) - @ca.expects(:apply).with { |cert_mode,to| cert_mode == "currentmode" } + @ca.expects(:apply).with { |cert_mode,to| cert_mode == :revoke } + @ca.expects(:apply).with { |cert_mode,to| cert_mode == :destroy } @cert_app.main end + end - it "should revoke cert if cert_mode is clean" do - @cert_app.cert_mode = :destroy - @cert_app.command_line.stubs(:args).returns(["host"]) + describe "when identifying subcommands" do + before :each do + @cert_app.all = false + @ca = stub_everything 'ca' + @cert_app.ca = @ca + end - @ca.expects(:apply).with { |cert_mode,to| cert_mode == :revoke } - @ca.expects(:apply).with { |cert_mode,to| cert_mode == :destroy } + %w{list revoke generate sign print verify fingerprint}.each do |cmd| + short = cmd[0,1] + [cmd, "--#{cmd}", "-#{short}"].each do |option| + # In our command line '-v' was eaten by 'verbose', so we can't consume + # it here; this is a special case from our otherwise standard + # processing. --daniel 2011-02-22 + next if option == "-v" - @cert_app.main + it "should recognise '#{option}'" do + args = [option, "fun.example.com"] + + @cert_app.command_line.stubs(:args).returns(args) + @cert_app.parse_options + @cert_app.subcommand.should == cmd.to_sym + + args.should == ["fun.example.com"] + end + end end + %w{clean --clean -c}.each do |ugly| + it "should recognise the '#{ugly}' option as destroy" do + args = [ugly, "fun.example.com"] + + @cert_app.command_line.stubs(:args).returns(args) + @cert_app.parse_options + @cert_app.subcommand.should == :destroy + + args.should == ["fun.example.com"] + end + end end end -- cgit From 309b9320feef3e1a9459c7a26d10955b4d6b549c Mon Sep 17 00:00:00 2001 From: Daniel Pittman Date: Tue, 22 Feb 2011 11:25:03 -0800 Subject: (#5552) Display help when no subcommand is given. Previously, when the command line was empty we would try and invoke an empty method; this was less helpful than telling people what they could actually do, so we adapt our code to do precisely that. Paired-With: Jesse Wolfe Signed-off-by: Daniel Pittman --- spec/unit/application/cert_spec.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'spec/unit/application') diff --git a/spec/unit/application/cert_spec.rb b/spec/unit/application/cert_spec.rb index 2f57d07a9..b3257916b 100755 --- a/spec/unit/application/cert_spec.rb +++ b/spec/unit/application/cert_spec.rb @@ -189,6 +189,16 @@ describe Puppet::Application::Cert do @cert_app.ca = @ca end + it "should not fail when no command is given" do + # Make the help method silent for testing; this is a bit nasty, but we + # can't identify a cleaner method. Help welcome. --daniel 2011-02-22 + Puppet.features.stubs(:usage?).returns(false) + @cert_app.stubs(:puts) + + @cert_app.command_line.stubs(:args).returns([]) + expect { @cert_app.parse_options }.should raise_error SystemExit + end + %w{list revoke generate sign print verify fingerprint}.each do |cmd| short = cmd[0,1] [cmd, "--#{cmd}", "-#{short}"].each do |option| -- cgit From 23eb77d999acb73021547c5ef86adf609e202605 Mon Sep 17 00:00:00 2001 From: Jesse Wolfe Date: Fri, 25 Feb 2011 11:45:38 -0800 Subject: (#6322) --noop should not suppress error codes The noop option has been suppressing exit statuses. This is counterintuitive, as per discussion at http://projects.puppetlabs.com/issues/6322 This patch causes noop runs to return the same exit codes as real runs. Reviewed-By: Daniel Pittman --- spec/unit/application/agent_spec.rb | 4 ++-- spec/unit/application/apply_spec.rb | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) (limited to 'spec/unit/application') diff --git a/spec/unit/application/agent_spec.rb b/spec/unit/application/agent_spec.rb index 9fc7879c9..8f498d4ba 100755 --- a/spec/unit/application/agent_spec.rb +++ b/spec/unit/application/agent_spec.rb @@ -526,11 +526,11 @@ describe Puppet::Application::Agent do @puppetd.onetime end - it "should always exit with 0 if --noop" do + it "should exit with the report's computer exit status, even if --noop is set." do Puppet.stubs(:[]).with(:noop).returns(true) report = stub 'report', :exit_status => 666 @agent.stubs(:run).returns(report) - @puppetd.expects(:exit).with(0) + @puppetd.expects(:exit).with(666) @puppetd.onetime end diff --git a/spec/unit/application/apply_spec.rb b/spec/unit/application/apply_spec.rb index ceba4a333..d4f39abe0 100755 --- a/spec/unit/application/apply_spec.rb +++ b/spec/unit/application/apply_spec.rb @@ -166,6 +166,13 @@ describe Puppet::Application::Apply do @apply.expects(:exit).with(1) @apply.parseonly end + + it "should exit with exit code 1 if error, even if --noop is set" do + Puppet[:noop] = true + @collection.stubs(:perform_initial_import).raises(Puppet::ParseError) + @apply.expects(:exit).with(1) + @apply.parseonly + end end describe "the main command" do @@ -327,6 +334,15 @@ describe Puppet::Application::Apply do @apply.main end + it "should exit with report's computed exit status, even if --noop is set" do + Puppet.stubs(:[]).with(:noop).returns(true) + @apply.options.stubs(:[]).with(:detailed_exitcodes).returns(true) + Puppet::Transaction::Report.any_instance.stubs(:exit_status).returns(666) + @apply.expects(:exit).with(666) + + @apply.main + end + it "should always exit with 0 if option is disabled" do Puppet.stubs(:[]).with(:noop).returns(false) @apply.options.stubs(:[]).with(:detailed_exitcodes).returns(false) -- cgit