Merge branch 'feature/next/6962-self-documenting-strings-actions-and-options' into next

28 files changed, 453 insertions, 116 deletions

diff --git a/lib/puppet/application.rb b/lib/puppet/application.rb
index 4c5a5a967..f3a749786 100644
--- a/lib/puppet/application.rb
+++ b/lib/puppet/application.rb
@@ -359,14 +359,10 @@ class Application
       end
     end
 
-    # scan command line.
-    begin
-      option_parser.parse!(self.command_line.args)
-    rescue OptionParser::ParseError => detail
-      $stderr.puts detail
-      $stderr.puts "Try 'puppet #{command_line.subcommand_name} --help'"
-      exit(1)
-    end
+    # Scan command line.  We just hand any exceptions to our upper levels,
+    # rather than printing help and exiting, so that we can meaningfully
+    # respond with context-sensitive help if we want to. --daniel 2011-04-12
+    option_parser.parse!(self.command_line.args)
   end
 
   def handlearg(opt, arg)
diff --git a/lib/puppet/application/cert.rb b/lib/puppet/application/cert.rb
index cbd6fd610..c08775380 100644
--- a/lib/puppet/application/cert.rb
+++ b/lib/puppet/application/cert.rb
@@ -48,7 +48,7 @@ class Puppet::Application::Cert < Puppet::Application
   end
 
   def help
-    puts <<-HELP
+    <<-HELP
 
 puppet-cert(8) -- Manage certificates and requests
 ========
@@ -166,7 +166,6 @@ COPYRIGHT
 Copyright (c) 2011 Puppet Labs, LLC Licensed under the Apache 2.0 License
 
     HELP
-    exit
   end
 
   def main
diff --git a/lib/puppet/application/faces_base.rb b/lib/puppet/application/faces_base.rb
index 288b50048..f1b77f285 100644
--- a/lib/puppet/application/faces_base.rb
+++ b/lib/puppet/application/faces_base.rb
@@ -1,5 +1,6 @@
 require 'puppet/application'
 require 'puppet/faces'
+require 'optparse'
 
 class Puppet::Application::FacesBase < Puppet::Application
   should_parse_config
@@ -50,11 +51,13 @@ class Puppet::Application::FacesBase < Puppet::Application
       $stderr.puts "Cancelling Face"
       exit(0)
     end
+  end
 
+  def parse_options
     # We need to parse enough of the command line out early, to identify what
     # the action is, so that we can obtain the full set of options to parse.
 
-    # TODO: These should be configurable versions, through a global
+    # REVISIT: These should be configurable versions, through a global
     # '--version' option, but we don't implement that yet... --daniel 2011-03-29
     @type   = self.class.name.to_s.sub(/.+:/, '').downcase.to_sym
     @face   = Puppet::Faces[@type, :current]
@@ -88,25 +91,30 @@ class Puppet::Application::FacesBase < Puppet::Application
             index += 1          # ...so skip the argument.
           end
         else
-          raise ArgumentError, "Unknown option #{item.sub(/=.*$/, '').inspect}"
+          raise OptionParser::InvalidOption.new(item.sub(/=.*$/, ''))
         end
       else
         action = @face.get_action(item.to_sym)
         if action.nil? then
-          raise ArgumentError, "#{@face} does not have an #{item.inspect} action!"
+          raise OptionParser::InvalidArgument.new("#{@face} does not have an #{item} action")
         end
         @action = action
       end
     end
 
-    @action or raise ArgumentError, "No action given on the command line!"
+    unless @action
+      raise OptionParser::MissingArgument.new("No action given on the command line")
+    end
 
-    # Finally, we can interact with the default option code to build behaviour
+    # Now we can interact with the default option code to build behaviour
     # around the full set of options we now know we support.
     @action.options.each do |option|
       option = @action.get_option(option) # make it the object.
       self.class.option(*option.optparse) # ...and make the CLI parse it.
     end
+
+    # ...and invoke our parent to parse all the command line options.
+    super
   end
 
   def find_global_settings_argument(item)
diff --git a/lib/puppet/application/help.rb b/lib/puppet/application/help.rb
new file mode 100644
index 000000000..fd8818db0
--- /dev/null
+++ b/lib/puppet/application/help.rb
@@ -0,0 +1,8 @@
+# -*- coding: utf-8 -*-
+require 'puppet/application/faces_base'
+
+class Puppet::Application::Help < Puppet::Application::FacesBase
+  # Meh.  Disable the default behaviour, which is to inspect the
+  # string and return that – not so helpful. --daniel 2011-04-11
+  def render(result) result end
+end
diff --git a/lib/puppet/faces/help.rb b/lib/puppet/faces/help.rb
new file mode 100644
index 000000000..1d8abe20e
--- /dev/null
+++ b/lib/puppet/faces/help.rb
@@ -0,0 +1,104 @@
+require 'puppet/faces'
+require 'puppet/util/command_line'
+require 'pathname'
+require 'erb'
+
+Puppet::Faces.define(:help, '0.0.1') do
+  summary "Displays help about puppet subcommands"
+
+  action(:help) do
+    summary "Display help about faces and their actions."
+
+    option "--version VERSION" do
+      desc "Which version of the interface to show help for"
+    end
+
+    when_invoked do |*args|
+      # Check our invocation, because we want varargs and can't do defaults
+      # yet.  REVISIT: when we do option defaults, and positional options, we
+      # should rewrite this to use those. --daniel 2011-04-04
+      options = args.pop
+      if options.nil? or args.length > 2 then
+        raise ArgumentError, "help only takes two (optional) arguments, a face name, and an action"
+      end
+
+      version = :current
+      if options.has_key? :version then
+        if options[:version].to_s !~ /^current$/i then
+          version = options[:version]
+        else
+          if args.length == 0 then
+            raise ArgumentError, "version only makes sense when a face is given"
+          end
+        end
+      end
+
+      # Name those parameters...
+      facename, actionname = args
+
+      if facename then
+        if legacy_applications.include? facename then
+          actionname and raise ArgumentError, "Legacy subcommands don't take actions"
+          return Puppet::Application[facename].help
+        else
+          face = Puppet::Faces[facename.to_sym, version]
+          actionname and action = face.get_action(actionname.to_sym)
+        end
+      end
+
+      case args.length
+      when 0 then
+        template = erb 'global.erb'
+      when 1 then
+        face or fail ArgumentError, "Unable to load face #{facename}"
+        template = erb 'face.erb'
+      when 2 then
+        face or fail ArgumentError, "Unable to load face #{facename}"
+        action or fail ArgumentError, "Unable to load action #{actionname} from #{face}"
+        template = erb 'action.erb'
+      else
+        fail ArgumentError, "Too many arguments to help action"
+      end
+
+      # Run the ERB template in our current binding, including all the local
+      # variables we established just above. --daniel 2011-04-11
+      return template.result(binding)
+    end
+  end
+
+  def erb(name)
+    template = (Pathname(__FILE__).dirname + "help" + name)
+    erb = ERB.new(template.read, nil, '%')
+    erb.filename = template.to_s
+    return erb
+  end
+
+  def legacy_applications
+    # The list of applications, less those that are duplicated as a face.
+    Puppet::Util::CommandLine.available_subcommands.reject do |appname|
+      Puppet::Faces.face? appname.to_sym, :current or
+        # ...this is a nasty way to exclude non-applications. :(
+        %w{faces_base indirection_base}.include? appname
+    end.sort
+  end
+
+  def horribly_extract_summary_from(appname)
+    begin
+      require "puppet/application/#{appname}"
+      help = Puppet::Application[appname].help.split("\n")
+      # Now we find the line with our summary, extract it, and return it.  This
+      # depends on the implementation coincidence of how our pages are
+      # formatted.  If we can't match the pattern we expect we return the empty
+      # string to ensure we don't blow up in the summary. --daniel 2011-04-11
+      while line = help.shift do
+        if md = /^puppet-#{appname}\([^\)]+\) -- (.*)$/.match(line) then
+          return md[1]
+        end
+      end
+    rescue Exception
+      # Damn, but I hate this: we just ignore errors here, no matter what
+      # class they are.  Meh.
+    end
+    return ''
+  end
+end
diff --git a/lib/puppet/faces/help/action.erb b/lib/puppet/faces/help/action.erb
new file mode 100644
index 000000000..eaf131464
--- /dev/null
+++ b/lib/puppet/faces/help/action.erb
@@ -0,0 +1,3 @@
+Use: puppet <%= face.name %> [options] <%= action.name %> [options]
+
+Summary: <%= action.summary %>
diff --git a/lib/puppet/faces/help/face.erb b/lib/puppet/faces/help/face.erb
new file mode 100644
index 000000000..efe5fd809
--- /dev/null
+++ b/lib/puppet/faces/help/face.erb
@@ -0,0 +1,7 @@
+Use: puppet <%= face.name %> [options] <action> [options]
+
+Available actions:
+% face.actions.each do |actionname|
+%   action = face.get_action(actionname)
+  <%= action.name.to_s.ljust(16) %>  <%= action.summary %>
+% end
diff --git a/lib/puppet/faces/help/global.erb b/lib/puppet/faces/help/global.erb
new file mode 100644
index 000000000..e123367a2
--- /dev/null
+++ b/lib/puppet/faces/help/global.erb
@@ -0,0 +1,20 @@
+puppet <subcommand> [options] <action> [options]
+
+Available subcommands, from Puppet Faces:
+% Puppet::Faces.faces.sort.each do |name|
+%   face = Puppet::Faces[name, :current]
+  <%= face.name.to_s.ljust(16) %>  <%= face.summary %>
+% end
+
+% unless legacy_applications.empty? then # great victory when this is true!
+Available applications, soon to be ported to Faces:
+%   legacy_applications.each do |appname|
+%     summary = horribly_extract_summary_from appname
+  <%= appname.to_s.ljust(16) %>  <%= summary %>
+%   end
+% end
+
+See 'puppet help <subcommand> <action>' for help on a specific subcommand action.
+See 'puppet help <subcommand>' for help on a specific subcommand.
+See 'puppet man  <subcommand>' for the full man page.
+Puppet v<%= Puppet::PUPPETVERSION %>
diff --git a/lib/puppet/interface.rb b/lib/puppet/interface.rb
index 07e27efa8..27b3584b9 100644
--- a/lib/puppet/interface.rb
+++ b/lib/puppet/interface.rb
@@ -68,8 +68,13 @@ class Puppet::Interface
     self.default_format = format.to_sym
   end
 
-  attr_accessor :type, :verb, :version, :arguments
-  attr_reader :name
+  attr_accessor :summary
+  def summary(value = nil)
+    @summary = value unless value.nil?
+    @summary
+  end
+
+  attr_reader :name, :version
 
   def initialize(name, version, &block)
     unless Puppet::Interface::FaceCollection.validate_version(version)
diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index e4a37a1f7..302e61901 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -14,6 +14,7 @@ class Puppet::Interface::Action
   attr_reader :name
   def to_s() "#{@face}##{@name}" end
 
+  attr_accessor :summary
 
   # Initially, this was defined to allow the @action.invoke pattern, which is
   # a very natural way to invoke behaviour given our introspection
diff --git a/lib/puppet/interface/action_builder.rb b/lib/puppet/interface/action_builder.rb
index b08c3d023..34bb3fa44 100644
--- a/lib/puppet/interface/action_builder.rb
+++ b/lib/puppet/interface/action_builder.rb
@@ -28,4 +28,8 @@ class Puppet::Interface::ActionBuilder
     option = Puppet::Interface::OptionBuilder.build(@action, *declaration, &block)
     @action.add_option(option)
   end
+
+  def summary(text)
+    @action.summary = text
+  end
 end
diff --git a/lib/puppet/interface/face_collection.rb b/lib/puppet/interface/face_collection.rb
index 84296582c..e4eb22fa3 100644
--- a/lib/puppet/interface/face_collection.rb
+++ b/lib/puppet/interface/face_collection.rb
@@ -53,7 +53,11 @@ module Puppet::Interface::FaceCollection
 
   def self.face?(name, version)
     name = underscorize(name)
-    return true if @faces[name].has_key?(version)
+
+    # Note: be careful not to accidentally create the top level key, either,
+    # because it will result in confusion when people try to enumerate the
+    # list of valid faces later. --daniel 2011-04-11
+    return true if @faces.has_key?(name) and @faces[name].has_key?(version)
 
     # We always load the current version file; the common case is that we have
     # the expected version and any compatibility versions in the same file,
@@ -106,7 +110,11 @@ module Puppet::Interface::FaceCollection
     # but we don't need that right now.
     #
     # So, this comment is a place-holder for that.  --daniel 2011-04-06
-    return !! @faces[name].has_key?(version)
+    #
+    # Note: be careful not to accidentally create the top level key, either,
+    # because it will result in confusion when people try to enumerate the
+    # list of valid faces later. --daniel 2011-04-11
+    return !! (@faces.has_key?(name) and @faces[name].has_key?(version))
   end
 
   def self.register(face)
diff --git a/lib/puppet/util/command_line.rb b/lib/puppet/util/command_line.rb
index 52b5f81ef..fa462ee2d 100644
--- a/lib/puppet/util/command_line.rb
+++ b/lib/puppet/util/command_line.rb
@@ -17,12 +17,12 @@ module Puppet
         'master'     => 'puppetmasterd'
       )
 
-      def initialize( zero = $0, argv = ARGV, stdin = STDIN )
+      def initialize(zero = $0, argv = ARGV, stdin = STDIN)
         @zero  = zero
         @argv  = argv.dup
         @stdin = stdin
 
-        @subcommand_name, @args = subcommand_and_args( @zero, @argv, @stdin )
+        @subcommand_name, @args = subcommand_and_args(@zero, @argv, @stdin)
         Puppet::Plugins.on_commandline_initialization(:command_line_object => self)
       end
 
@@ -33,19 +33,20 @@ module Puppet
         File.join('puppet', 'application')
       end
 
-      def available_subcommands
-        absolute_appdirs = $LOAD_PATH.collect do |x| 
+      def self.available_subcommands
+        absolute_appdirs = $LOAD_PATH.collect do |x|
           File.join(x,'puppet','application')
         end.select{ |x| File.directory?(x) }
         absolute_appdirs.inject([]) do |commands, dir|
           commands + Dir[File.join(dir, '*.rb')].map{|fn| File.basename(fn, '.rb')}
         end.uniq
       end
-
-      def usage_message
-        usage = "Usage: puppet command <space separated arguments>"
-        available = "Available commands are: #{available_subcommands.sort.join(', ')}"
-        [usage, available].join("\n")
+      # available_subcommands was previously an instance method, not a class
+      # method, and we have an unknown number of user-implemented applications
+      # that depend on that behaviour.  Forwarding allows us to preserve a
+      # backward compatible API. --daniel 2011-04-11
+      def available_subcommands
+        self.class.available_subcommands
       end
 
       def require_application(application)
@@ -53,15 +54,24 @@ module Puppet
       end
 
       def execute
-        if subcommand_name.nil?
-          puts usage_message
-        elsif available_subcommands.include?(subcommand_name) #subcommand
+        if subcommand_name and available_subcommands.include?(subcommand_name) then
           require_application subcommand_name
           app = Puppet::Application.find(subcommand_name).new(self)
           Puppet::Plugins.on_application_initialization(:appliation_object => self)
           app.run
+        elsif execute_external_subcommand then
+          # Logically, we shouldn't get here, but we do, so whatever.  We just
+          # return to the caller.  How strange we are. --daniel 2011-04-11
         else
-          abort "Error: Unknown command #{subcommand_name}.\n#{usage_message}" unless execute_external_subcommand
+          unless subcommand_name.nil? then
+            puts "Error: Unknown Puppet subcommand #{subcommand_name}.\n"
+          end
+
+          # Doing this at the top of the file is natural, but causes puppet.rb
+          # to load too early, which causes things to break.  This is a nasty
+          # thing, found in #7065. --daniel 2011-04-11
+          require 'puppet/faces/help'
+          puts Puppet::Faces[:help, :current].help
         end
       end
 
@@ -69,10 +79,10 @@ module Puppet
         external_command = "puppet-#{subcommand_name}"
 
         require 'puppet/util'
-        path_to_subcommand = Puppet::Util.which( external_command )
+        path_to_subcommand = Puppet::Util.which(external_command)
         return false unless path_to_subcommand
 
-        system( path_to_subcommand, *args )
+        system(path_to_subcommand, *args)
         true
       end
 
@@ -82,7 +92,7 @@ module Puppet
 
       private
 
-      def subcommand_and_args( zero, argv, stdin )
+      def subcommand_and_args(zero, argv, stdin)
         zero = File.basename(zero, '.rb')
 
         if zero == 'puppet'
diff --git a/spec/lib/puppet/faces/basetest.rb b/spec/lib/puppet/faces/basetest.rb
new file mode 100644
index 000000000..d20c52b97
--- /dev/null
+++ b/spec/lib/puppet/faces/basetest.rb
@@ -0,0 +1 @@
+Puppet::Faces.define(:basetest, '0.0.1')
diff --git a/spec/lib/puppet/faces/huzzah.rb b/spec/lib/puppet/faces/huzzah.rb
index 735004475..e86730250 100644
--- a/spec/lib/puppet/faces/huzzah.rb
+++ b/spec/lib/puppet/faces/huzzah.rb
@@ -1,4 +1,5 @@
 require 'puppet/faces'
 Puppet::Faces.define(:huzzah, '2.0.1') do
+  summary "life is a thing for celebration"
   action :bar do "is where beer comes from" end
 end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index d28cb2504..1187c1caf 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -7,6 +7,7 @@ ARGV.clear
 require 'puppet'
 require 'mocha'
 gem 'rspec', '>=2.0.0'
+require 'rspec/expectations'
 
 # So everyone else doesn't have to include this base constant.
 module PuppetSpec
@@ -65,3 +66,9 @@ RSpec.configure do |config|
     GC.enable
   end
 end
+
+RSpec::Matchers.define :have_matching_element do |expected|
+  match do |actual|
+    actual.any? { |item| item =~ expected }
+  end
+end
diff --git a/spec/unit/application/cert_spec.rb b/spec/unit/application/cert_spec.rb
index 5b25ab7b8..a1b5eb19a 100755
--- a/spec/unit/application/cert_spec.rb
+++ b/spec/unit/application/cert_spec.rb
@@ -1,7 +1,5 @@
-#!/usr/bin/env ruby
-
+#!/usr/bin/env rspec
 require 'spec_helper'
-
 require 'puppet/application/cert'
 
 describe Puppet::Application::Cert do
@@ -189,16 +187,6 @@ describe Puppet::Application::Cert do
       @cert_app.ca = @ca
     end
 
-    it "should SystemExit after printing help message" 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|
diff --git a/spec/unit/application/certificate_spec.rb b/spec/unit/application/certificate_spec.rb
index 6153d9538..27d6ac81b 100755
--- a/spec/unit/application/certificate_spec.rb
+++ b/spec/unit/application/certificate_spec.rb
@@ -6,12 +6,15 @@ describe Puppet::Application::Certificate do
     # so is this actually a valuable test? --daniel 2011-04-07
     subject.command_line.stubs(:args).returns %w{list}
     subject.preinit
+    subject.parse_options
     subject.should respond_to(:handle_ca_location)
   end
 
   it "should accept the ca-location option" do
     subject.command_line.stubs(:args).returns %w{--ca-location local list}
-    subject.preinit and subject.parse_options and subject.setup
+    subject.preinit
+    subject.parse_options
+    subject.setup
     subject.arguments.should == [{ :ca_location => "local" }]
   end
 end
diff --git a/spec/unit/application/faces_base_spec.rb b/spec/unit/application/faces_base_spec.rb
index b7a11ad56..18bd30295 100755
--- a/spec/unit/application/faces_base_spec.rb
+++ b/spec/unit/application/faces_base_spec.rb
@@ -1,4 +1,4 @@
-#!/usr/bin/env ruby
+#!/usr/bin/env rspec
 
 require 'spec_helper'
 require 'puppet/application/faces_base'
@@ -9,13 +9,6 @@ end
 
 describe Puppet::Application::FacesBase do
   before :all do
-    @dir = Dir.mktmpdir
-    $LOAD_PATH.push(@dir)
-    FileUtils.mkdir_p(File.join @dir, 'puppet', 'faces')
-    File.open(File.join(@dir, 'puppet', 'faces', 'basetest.rb'), 'w') do |f|
-      f.puts "Puppet::Faces.define(:basetest, '0.0.1')"
-    end
-
     Puppet::Faces.define(:basetest, '0.0.1') do
       option("--[no-]boolean")
       option("--mandatory MANDATORY")
@@ -28,16 +21,9 @@ describe Puppet::Application::FacesBase do
     end
   end
 
-  after :all do
-    FileUtils.remove_entry_secure @dir
-    $LOAD_PATH.pop
-  end
-
   let :app do
     app = Puppet::Application::FacesBase::Basetest.new
-    app.stubs(:exit)
-    app.stubs(:puts)
-    app.command_line.stubs(:subcommand_name).returns 'subcommand'
+    app.command_line.stubs(:subcommand_name).returns('subcommand')
     Puppet::Util::Log.stubs(:newdestination)
     app
   end
@@ -51,7 +37,7 @@ describe Puppet::Application::FacesBase do
     end
   end
 
-  describe "#preinit" do
+  describe "#parse_options" do
     before :each do
       app.command_line.stubs(:args).returns %w{}
     end
@@ -68,6 +54,7 @@ describe Puppet::Application::FacesBase do
           Signal.stubs(:trap)
           app.command_line.stubs(:args).returns %w{foo}
           app.preinit
+          app.parse_options
         end
 
         it "should set the faces based on the type" do
@@ -85,54 +72,54 @@ describe Puppet::Application::FacesBase do
       end
 
       it "should fail if no action is given" do
-        expect { app.preinit }.
-          should raise_error ArgumentError, /No action given/
+        expect { app.preinit; app.parse_options }.
+          to raise_error OptionParser::MissingArgument, /No action given/
       end
 
       it "should report a sensible error when options with = fail" do
         app.command_line.stubs(:args).returns %w{--action=bar foo}
-        expect { app.preinit }.
-          should raise_error ArgumentError, /Unknown option "--action"/
+        expect { app.preinit; app.parse_options }.
+          to raise_error OptionParser::InvalidOption, /invalid option: --action/
       end
 
       it "should fail if an action option is before the action" do
         app.command_line.stubs(:args).returns %w{--action foo}
-        expect { app.preinit }.
-          should raise_error ArgumentError, /Unknown option "--action"/
+        expect { app.preinit; app.parse_options }.
+          to raise_error OptionParser::InvalidOption, /invalid option: --action/
       end
 
       it "should fail if an unknown option is before the action" do
         app.command_line.stubs(:args).returns %w{--bar foo}
-        expect { app.preinit }.
-          should raise_error ArgumentError, /Unknown option "--bar"/
+        expect { app.preinit; app.parse_options }.
+          to raise_error OptionParser::InvalidOption, /invalid option: --bar/
       end
 
-      it "should not fail if an unknown option is after the action" do
+      it "should fail if an unknown option is after the action" do
         app.command_line.stubs(:args).returns %w{foo --bar}
-        app.preinit
-        app.action.name.should == :foo
-        app.face.should_not be_option :bar
-        app.action.should_not be_option :bar
+        expect { app.preinit; app.parse_options }.
+          to raise_error OptionParser::InvalidOption, /invalid option: --bar/
       end
 
       it "should accept --bar as an argument to a mandatory option after action" do
         app.command_line.stubs(:args).returns %w{foo --mandatory --bar}
-        app.preinit and app.parse_options
+        app.preinit
+        app.parse_options
         app.action.name.should == :foo
         app.options.should == { :mandatory => "--bar" }
       end
 
       it "should accept --bar as an argument to a mandatory option before action" do
         app.command_line.stubs(:args).returns %w{--mandatory --bar foo}
-        app.preinit and app.parse_options
+        app.preinit
+        app.parse_options
         app.action.name.should == :foo
         app.options.should == { :mandatory => "--bar" }
       end
 
       it "should not skip when --foo=bar is given" do
         app.command_line.stubs(:args).returns %w{--mandatory=bar --bar foo}
-        expect { app.preinit }.
-          should raise_error ArgumentError, /Unknown option "--bar"/
+        expect { app.preinit; app.parse_options }.
+          to raise_error OptionParser::InvalidOption, /invalid option: --bar/
       end
 
       { "boolean options before" => %w{--trace foo},
@@ -140,7 +127,8 @@ describe Puppet::Application::FacesBase do
       }.each do |name, args|
         it "should accept global boolean settings #{name} the action" do
           app.command_line.stubs(:args).returns args
-          app.preinit && app.parse_options
+          app.preinit
+          app.parse_options
           Puppet[:trace].should be_true
         end
       end
@@ -150,7 +138,8 @@ describe Puppet::Application::FacesBase do
       }.each do |name, args|
         it "should accept global settings with arguments #{name} the action" do
           app.command_line.stubs(:args).returns args
-          app.preinit && app.parse_options
+          app.preinit
+          app.parse_options
           Puppet[:syslogfacility].should == "user1"
         end
       end
@@ -160,19 +149,25 @@ describe Puppet::Application::FacesBase do
   describe "#setup" do
     it "should remove the action name from the arguments" do
       app.command_line.stubs(:args).returns %w{--mandatory --bar foo}
-      app.preinit and app.parse_options and app.setup
+      app.preinit
+      app.parse_options
+      app.setup
       app.arguments.should == [{ :mandatory => "--bar" }]
     end
 
     it "should pass positional arguments" do
       app.command_line.stubs(:args).returns %w{--mandatory --bar foo bar baz quux}
-      app.preinit and app.parse_options and app.setup
+      app.preinit
+      app.parse_options
+      app.setup
       app.arguments.should == ['bar', 'baz', 'quux', { :mandatory => "--bar" }]
     end
   end
 
   describe "#main" do
-    before do
+    before :each do
+      app.expects(:exit).with(0)
+
       app.face      = Puppet::Faces[:basetest, '0.0.1']
       app.action    = app.face.get_action(:foo)
       app.format    = :pson
@@ -186,6 +181,7 @@ describe Puppet::Application::FacesBase do
 
     it "should use its render method to render any result" do
       app.expects(:render).with(app.arguments.length + 1)
+      app.stubs(:puts)          # meh.  Don't print nil, thanks. --daniel 2011-04-12
       app.main
     end
   end
diff --git a/spec/unit/application/indirection_base_spec.rb b/spec/unit/application/indirection_base_spec.rb
index 10ebe8e3d..98eb3a118 100755
--- a/spec/unit/application/indirection_base_spec.rb
+++ b/spec/unit/application/indirection_base_spec.rb
@@ -13,7 +13,7 @@ face = Puppet::Faces::Indirector.define(:testindirection, '0.0.1') do
 end
 # REVISIT: This horror is required because we don't allow anything to be
 # :current except for if it lives on, and is loaded from, disk. --daniel 2011-03-29
-face.version = :current
+face.instance_variable_set('@version', :current)
 Puppet::Faces.register(face)
 ########################################################################
 
diff --git a/spec/unit/application_spec.rb b/spec/unit/application_spec.rb
index 740b76f62..a1a46c814 100755
--- a/spec/unit/application_spec.rb
+++ b/spec/unit/application_spec.rb
@@ -357,16 +357,6 @@ describe Puppet::Application do
       end
 
     end
-
-    it "should exit if OptionParser raises an error" do
-      $stderr.stubs(:puts)
-      OptionParser.any_instance.stubs(:parse!).raises(OptionParser::ParseError.new("blah blah"))
-
-      @app.expects(:exit)
-
-      lambda { @app.parse_options }.should_not raise_error
-    end
-
   end
 
   describe "when calling default setup" do
diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
new file mode 100644
index 000000000..cd74a5bf1
--- /dev/null
+++ b/spec/unit/faces/help_spec.rb
@@ -0,0 +1,112 @@
+require 'spec_helper'
+require 'puppet/faces/help'
+
+describe Puppet::Faces[:help, '0.0.1'] do
+  it "should have a help action" do
+    subject.should be_action :help
+  end
+
+  it "should have a default action of help" do
+    pending "REVISIT: we don't support default actions yet"
+  end
+
+  it "should accept a call with no arguments" do
+    expect { subject.help() }.should_not raise_error
+  end
+
+  it "should accept a face name" do
+    expect { subject.help(:help) }.should_not raise_error
+  end
+
+  it "should accept a face and action name" do
+    expect { subject.help(:help, :help) }.should_not raise_error
+  end
+
+  it "should fail if more than a face and action are given" do
+    expect { subject.help(:help, :help, :for_the_love_of_god) }.
+      should raise_error ArgumentError
+  end
+
+  it "should treat :current and 'current' identically" do
+    subject.help(:help, :version => :current).should ==
+      subject.help(:help, :version => 'current')
+  end
+
+  it "should complain when the request version of a face is missing" do
+    expect { subject.help(:huzzah, :bar, :version => '17.0.0') }.
+      should raise_error Puppet::Error
+  end
+
+  it "should find a face by version" do
+    face = Puppet::Faces[:huzzah, :current]
+    subject.help(:huzzah, :version => face.version).
+      should == subject.help(:huzzah, :version => :current)
+  end
+
+  context "when listing subcommands" do
+    subject { Puppet::Faces[:help, :current].help }
+
+    # Check a precondition for the next block; if this fails you have
+    # something odd in your set of faces, and we skip testing things that
+    # matter. --daniel 2011-04-10
+    it "should have at least one face with a summary" do
+      Puppet::Faces.faces.should be_any do |name|
+        Puppet::Faces[name, :current].summary
+      end
+    end
+
+    Puppet::Faces.faces.each do |name|
+      face = Puppet::Faces[name, :current]
+      summary = face.summary
+
+      it { should =~ %r{ #{name} } }
+      it { should =~ %r{ #{name} +#{summary}} } if summary
+    end
+
+    Puppet::Faces[:help, :current].legacy_applications.each do |appname|
+      it { should =~ %r{ #{appname} } }
+
+      summary = Puppet::Faces[:help, :current].horribly_extract_summary_from(appname)
+      summary and it { should =~ %r{ #{summary}\b} }
+    end
+  end
+
+  context "#legacy_applications" do
+    subject { Puppet::Faces[:help, :current].legacy_applications }
+
+    # If we don't, these tests are ... less than useful, because they assume
+    # it.  When this breaks you should consider ditching the entire feature
+    # and tests, but if not work out how to fake one. --daniel 2011-04-11
+    it { should have_at_least(1).item }
+
+    # Meh.  This is nasty, but we can't control the other list; the specific
+    # bug that caused these to be listed is annoyingly subtle and has a nasty
+    # fix, so better to have a "fail if you do something daft" trigger in
+    # place here, I think. --daniel 2011-04-11
+    %w{faces_base indirection_base}.each do |name|
+      it { should_not include name }
+    end
+  end
+
+  context "help for legacy applications" do
+    subject { Puppet::Faces[:help, :current] }
+    let :appname do subject.legacy_applications.first end
+
+    # This test is purposely generic, so that as we eliminate legacy commands
+    # we don't get into a loop where we either test a face-based replacement
+    # and fail to notice breakage, or where we have to constantly rewrite this
+    # test and all. --daniel 2011-04-11
+    it "should return the legacy help when given the subcommand" do
+      help = subject.help(appname)
+      help.should =~ /puppet-#{appname}/
+      %w{SYNOPSIS USAGE DESCRIPTION OPTIONS COPYRIGHT}.each do |heading|
+        help.should =~ /^#{heading}$/
+      end
+    end
+
+    it "should fail when asked for an action on a legacy command" do
+      expect { subject.help(appname, :whatever) }.
+        to raise_error ArgumentError, /Legacy subcommands don't take actions/
+    end
+  end
+end
diff --git a/spec/unit/faces_spec.rb b/spec/unit/faces_spec.rb
new file mode 100644
index 000000000..b6c49d917
--- /dev/null
+++ b/spec/unit/faces_spec.rb
@@ -0,0 +1 @@
+# You should look at interface_spec.rb
diff --git a/spec/unit/interface/action_builder_spec.rb b/spec/unit/interface/action_builder_spec.rb
index 7d2710942..666575605 100755
--- a/spec/unit/interface/action_builder_spec.rb
+++ b/spec/unit/interface/action_builder_spec.rb
@@ -55,5 +55,16 @@ describe Puppet::Interface::ActionBuilder do
         action.should be_option :bar
       end
     end
+
+    context "inline documentation" do
+      let :face do Puppet::Interface.new(:inline_action_docs, '0.0.1') end
+
+      it "should set the summary" do
+        action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+          summary "this is some text"
+        end
+        action.summary.should == "this is some text"
+      end
+    end
   end
 end
diff --git a/spec/unit/interface/face_collection_spec.rb b/spec/unit/interface/face_collection_spec.rb
index b83bd50d3..752871035 100755
--- a/spec/unit/interface/face_collection_spec.rb
+++ b/spec/unit/interface/face_collection_spec.rb
@@ -8,9 +8,14 @@ describe Puppet::Interface::FaceCollection do
   # To avoid cross-pollution we have to save and restore both the hash
   # containing all the interface data, and the array used by require.  Restoring
   # both means that we don't leak side-effects across the code. --daniel 2011-04-06
+  #
+  # Worse luck, we *also* need to flush $" of anything defining a face,
+  # because otherwise we can cross-pollute from other test files and end up
+  # with no faces loaded, but the require value set true. --daniel 2011-04-10
   before :each do
     @original_faces    = subject.instance_variable_get("@faces").dup
     @original_required = $".dup
+    $".delete_if do |path| path =~ %r{/faces/.*\.rb$} end
     subject.instance_variable_get("@faces").clear
   end
 
@@ -75,18 +80,14 @@ describe Puppet::Interface::FaceCollection do
     end
 
     it "should attempt to load the default faces for the specified version :current" do
-      subject.expects(:require).never # except...
       subject.expects(:require).with('puppet/faces/fozzie')
       subject['fozzie', :current]
     end
   end
 
   describe "::face?" do
-    before :each do
-      subject.instance_variable_get("@faces")[:foo]['0.0.1'] = 10
-    end
-
     it "should return true if the faces specified is registered" do
+      subject.instance_variable_get("@faces")[:foo]['0.0.1'] = 10
       subject.face?("foo", '0.0.1').should == true
     end
 
@@ -137,6 +138,11 @@ describe Puppet::Interface::FaceCollection do
         subject.face?(:huzzah, :current).should be_true
       end
     end
+
+    it "should not cause an invalid face to be enumerated later" do
+      subject.face?(:there_is_no_face, :current).should be_false
+      subject.faces.should_not include :there_is_no_face
+    end
   end
 
   describe "::register" do
diff --git a/spec/unit/interface_spec.rb b/spec/unit/interface_spec.rb
index ea11b21ba..7e6b7de77 100755
--- a/spec/unit/interface_spec.rb
+++ b/spec/unit/interface_spec.rb
@@ -1,3 +1,4 @@
+require 'spec_helper'
 require 'puppet/faces'
 require 'puppet/interface'
 
@@ -16,6 +17,20 @@ describe Puppet::Interface do
     Puppet::Interface::FaceCollection.instance_variable_set("@faces", @faces)
   end
 
+  describe "#[]" do
+    it "should fail when no version is requested" do
+      expect { subject[:huzzah] }.should raise_error ArgumentError
+    end
+
+    it "should raise an exception when the requested version is unavailable" do
+      expect { subject[:huzzah, '17.0.0'] }.should raise_error, Puppet::Error
+    end
+
+    it "should raise an exception when the requested face doesn't exist" do
+      expect { subject[:burrble_toot, :current] }.should raise_error, Puppet::Error
+    end
+  end
+
   describe "#define" do
     it "should register the face" do
       face = subject.define(:face_test_register, '0.0.1')
@@ -28,13 +43,36 @@ describe Puppet::Interface do
     end
 
     it "should require a version number" do
-      expect { subject.define(:no_version) }.should raise_error ArgumentError
+      expect { subject.define(:no_version) }.to raise_error ArgumentError
+    end
+
+    it "should support summary builder and accessor methods" do
+      subject.new(:foo, '1.0.0').should respond_to(:summary).with(0).arguments
+      subject.new(:foo, '1.0.0').should respond_to(:summary=).with(1).arguments
+    end
+
+    it "should set the summary text" do
+      text = "hello, freddy, my little pal"
+      subject.define(:face_test_summary, '1.0.0') do
+        summary text
+      end
+      subject[:face_test_summary, '1.0.0'].summary.should == text
+    end
+
+    it "should support mutating the summary" do
+      text = "hello, freddy, my little pal"
+      subject.define(:face_test_summary, '1.0.0') do
+        summary text
+      end
+      subject[:face_test_summary, '1.0.0'].summary.should == text
+      subject[:face_test_summary, '1.0.0'].summary = text + text
+      subject[:face_test_summary, '1.0.0'].summary.should == text + text
     end
   end
 
   describe "#initialize" do
     it "should require a version number" do
-      expect { subject.new(:no_version) }.should raise_error ArgumentError
+      expect { subject.new(:no_version) }.to raise_error ArgumentError
     end
 
     it "should require a valid version number" do
diff --git a/spec/unit/util/command_line_spec.rb b/spec/unit/util/command_line_spec.rb
index ca5f0540f..6cf90475b 100755
--- a/spec/unit/util/command_line_spec.rb
+++ b/spec/unit/util/command_line_spec.rb
@@ -99,10 +99,11 @@ describe Puppet::Util::CommandLine do
         Puppet::Util.expects(:which).with('puppet-whatever').returns(nil)
         commandline.expects(:system).never
 
-        commandline.expects(:usage_message).returns("the usage message")
-        commandline.expects(:abort).with{|x| x =~ /the usage message/}.raises("stubbed abort")
+        text = Puppet::Faces[:help, :current].help
+        commandline.expects(:puts).with { |x| x =~ /Unknown Puppet subcommand/ }
+        commandline.expects(:puts).with text
 
-        lambda{ commandline.execute }.should raise_error('stubbed abort')
+        commandline.execute
       end
     end
   end
@@ -111,6 +112,11 @@ describe Puppet::Util::CommandLine do
       @core_apps = %w{describe filebucket kick queue resource agent cert apply doc master}
       @command_line = Puppet::Util::CommandLine.new("foo", %w{ client --help whatever.pp }, @tty )
     end
+    it "should expose available_subcommands as a class method" do
+      @core_apps.each do |command|
+        @command_line.available_subcommands.should include command
+      end
+    end
     it 'should be able to find all existing commands' do
       @core_apps.each do |command|
         @command_line.available_subcommands.should include command
diff --git a/spec/watchr.rb b/spec/watchr.rb
index bad89b088..26919d1a1 100644
--- a/spec/watchr.rb
+++ b/spec/watchr.rb
@@ -15,10 +15,10 @@ def run_comp(cmd)
         line << c
         if c == ?\n
           results << if RUBY_VERSION >= "1.9" then
-            line.join
-              else
-                line.pack "c*"
-                  end
+                       line.join
+                     else
+                       line.pack "c*"
+                     end
           line.clear
         end
       end
@@ -80,7 +80,11 @@ end
 def run_spec_files(files)
   files = Array(files)
   return if files.empty?
-  opts = File.readlines('spec/spec.opts').collect { |l| l.chomp }.join(" ")
+  if File.exist?(File.expand_path("~/.rspec")) then
+    opts = ''                   # use the user defaults
+  else
+    opts = File.readlines('spec/spec.opts').collect { |l| l.chomp }.join(" ")
+  end
   run_spec("rspec #{opts} --tty #{files.join(' ')}")
 end
 
@@ -89,12 +93,12 @@ def run_all_tests
 end
 
 def run_all_specs
-  run_test("rake spec")
+  run_spec_files "spec"
 end
 
 def run_suite
-  run_all_tests
   run_all_specs
+  run_all_tests
 end
 
 watch('spec/spec_helper.rb') { run_all_specs }