In the past any error in loading config or parsing application list would simply result in an empty applicaiton list being returned. In cases of a configuration error this masked the real cause and made debugging really hard.
We now print the exception received instead and exit with a exit code of 1 which should improve the situation marginally. Also improve the test coverage for the Applications class Signed-off-by: R.I.Pienaar <[email protected]> --- Local-branch: bug/master/7574 lib/mcollective/applications.rb | 5 +- spec/fixtures/application/test.rb | 7 ++ spec/unit/applications_spec.rb | 128 +++++++++++++++++++++++++++++++++++++ website/changelog.md | 1 + 4 files changed, 139 insertions(+), 2 deletions(-) create mode 100644 spec/fixtures/application/test.rb diff --git a/lib/mcollective/applications.rb b/lib/mcollective/applications.rb index 244bbec..ee2c694 100644 --- a/lib/mcollective/applications.rb +++ b/lib/mcollective/applications.rb @@ -37,8 +37,9 @@ module MCollective end applist - rescue - return [] + rescue Exception => e + STDERR.puts "Failed to generate application list: #{e.class}: #{e}" + exit 1 end # Filters a string of opts out using Shellwords diff --git a/spec/fixtures/application/test.rb b/spec/fixtures/application/test.rb new file mode 100644 index 0000000..f1ba8dd --- /dev/null +++ b/spec/fixtures/application/test.rb @@ -0,0 +1,7 @@ +module MCollective + class Application::Test < Application + def run + true + end + end +end diff --git a/spec/unit/applications_spec.rb b/spec/unit/applications_spec.rb index 6d7d81c..3a82bf3 100644 --- a/spec/unit/applications_spec.rb +++ b/spec/unit/applications_spec.rb @@ -4,6 +4,134 @@ require File.dirname(__FILE__) + '/../spec_helper' module MCollective describe Applications do + before do + tmpfile = Tempfile.new("mc_applications_spec") + path = tmpfile.path + tmpfile.close! + + @tmpdir = FileUtils.mkdir_p(path) + @tmpdir = @tmpdir[0] if @tmpdir.is_a?(Array) # ruby 1.9.2 + $LOAD_PATH << @tmpdir + + @appsdir = File.join([@tmpdir, "mcollective", "application"]) + FileUtils.mkdir_p(@appsdir) + + FileUtils.cp(File.join([File.dirname(__FILE__), "..", "fixtures", "application", "test.rb"]), @appsdir) + end + + after do + FileUtils.rm_r(@tmpdir) + end + + describe "[]" do + it "should load the config" do + Applications.expects(:load_config).once + PluginManager.expects("[]").once + Applications["test"] + end + + it "should return the correct stored application" do + app = mock("app") + app.stubs(:run) + + Applications.expects(:load_config).once + PluginManager.expects("[]").with("test_application").once.returns(app) + Applications["test"].should == app + end + end + + describe "#run" do + it "should load the configuration" do + app = mock("app") + app.stubs(:run) + + Applications.expects(:load_config).once + Applications.expects(:load_application).once + PluginManager.expects("[]").once.returns(app) + + Applications.run("test") + end + + it "should load the application" do + app = mock("app") + app.stubs(:run) + + Applications.expects(:load_config).once + Applications.expects(:load_application).with("test").once + PluginManager.expects("[]").once.returns(app) + + Applications.run("test") + end + + it "should invoke the application run method" do + app = mock("app") + app.stubs(:run).returns("hello world") + + Applications.expects(:load_config).once + Applications.expects(:load_application) + PluginManager.expects("[]").once.returns(app) + + Applications.run("test").should == "hello world" + end + end + + describe "#load_application" do + it "should return the existing application if already loaded" do + app = mock("app") + app.stubs(:run) + + PluginManager << {:type => "test_application", :class => app} + + Applications.expects("load_config").never + + Applications.load_application("test") + end + + it "should load the config" do + Applications.expects("load_config").returns(true).once + Applications.load_application("test") + end + + it "should load the correct class from disk" do + PluginManager.expects("loadclass").with("MCollective::Application::Test") + Applications.expects("load_config").returns(true).once + + Applications.load_application("test") + end + + it "should add the class to the plugin manager" do + Applications.expects("load_config").returns(true).once + + PluginManager.expects("<<").with({:type => "test_application", :class => "MCollective::Application::Test"}) + + Applications.load_application("test") + end + end + + describe "#list" do + it "should load the configuration" do + Applications.expects("load_config").returns(true).once + Config.any_instance.expects("libdir").returns(@tmpdir) + Applications.list + end + + it "should add found applications to the list" do + Applications.expects("load_config").returns(true).once + Config.any_instance.expects("libdir").returns(@tmpdir) + + Applications.list.should == ["test"] + end + + it "should print a friendly error and exit on failure" do + Applications.expects("load_config").returns(true).once + IO.any_instance.expects(:puts).with(regexp_matches(/Failed to generate application list/)).once + + expect { + Applications.list.should + }.to raise_error(SystemExit) + end + end + describe "#filter_extra_options" do it "should parse --config=x" do ["--config=x --foo=bar -f -f bar", "--foo=bar --config=x -f -f bar"].each do |t| diff --git a/website/changelog.md b/website/changelog.md index e307745..90a856b 100644 --- a/website/changelog.md +++ b/website/changelog.md @@ -11,6 +11,7 @@ title: Changelog |Date|Description|Ticket| |----|-----------|------| +|2011/05/20|Improve error reporting in the single application framework|7574| |2011/05/16|Allow _._ in fact names|7532| |2011/05/16|Fix compatability issues with RH4 init system|7448| |2011/05/15|Handle failures from remote nodes better in the inventory app|7524| -- 1.7.1 -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
