From 20d9ac1a5b71cd52c3edea107c1cef08562641fd Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Sun, 10 Apr 2011 15:57:33 -0700
Subject: maint: fix indentation in the watchr script.

Whitespace changes, no functional changes.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/watchr.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

(limited to 'spec')

diff --git a/spec/watchr.rb b/spec/watchr.rb
index bad89b088..6f952a64f 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
-- 
cgit 


From 6fcf03c52f98c6e4bcce85548d783a832eaa387b Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Sun, 10 Apr 2011 16:47:33 -0700
Subject: maint: added testing for Puppet::Faces#[]

We didn't do much testing here, which was vaguely reasonable when we didn't
distinguish it from the #define method.  Now they are split out we need to be
more careful about testing the right things.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/interface_spec.rb | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

(limited to 'spec')

diff --git a/spec/unit/interface_spec.rb b/spec/unit/interface_spec.rb
index ea11b21ba..b25d06f51 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')
-- 
cgit 


From 8b13e2b586af60bdef73ac8119b19233c60903ac Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Sun, 10 Apr 2011 16:56:59 -0700
Subject: maint: watchr should respect personal account-wide defaults.

Previously the rspec.opts file would override any other options configured for
rspec and, by virtue of being on the command-line, would win against
configuration files.

Now, where a user configuration file exists in ~/.rspec we ignore the shipped
set of options.  This allows advanced users to customize without using the
default (and relatively nasty) default rpsec UI.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/watchr.rb | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

(limited to 'spec')

diff --git a/spec/watchr.rb b/spec/watchr.rb
index 6f952a64f..26919d1a1 100644
--- a/spec/watchr.rb
+++ b/spec/watchr.rb
@@ -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 }
-- 
cgit 


From a20810e6a822dca3a69a1abf903dfea55aa166c8 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Sun, 10 Apr 2011 19:01:02 -0700
Subject: maint: direct people to the expected spec file...

Because the implementation is directly an alias to another file, the spec here
is empty.  Instead, direct people over to the spec file they should be looking
in.  Makes it easier for new developers at pretty much zero cast.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/faces_spec.rb | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 spec/unit/faces_spec.rb

(limited to 'spec')

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
-- 
cgit 


From dc2675df2805d1e2dbf3c50a49152bcfd78f922f Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Sun, 10 Apr 2011 17:50:36 -0700
Subject: (#6770) Improve test robustness against 'require'

We hit another situation where we crudely flushed the internal set of faces,
but require thought we had already loaded the file defining one.  This fooled
our autoloader and raised more annoying problems.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/interface/face_collection_spec.rb | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

(limited to 'spec')

diff --git a/spec/unit/interface/face_collection_spec.rb b/spec/unit/interface/face_collection_spec.rb
index b83bd50d3..e6e03c3d2 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
 
-- 
cgit 


From d8dfb1f143ba3e602bec387508c8dad7aab00062 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Sun, 10 Apr 2011 18:59:23 -0700
Subject: (#6962) Implement 'summary' for faces.

This adds the methods to the summary builder and runtime instance to support
setting and getting a summary of the face.  This is a short description used
to summarize the purpose of the face in help output.

For example, from the help face:

    "Displays help about puppet subcommands"

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/application/indirection_base_spec.rb |  2 +-
 spec/unit/interface_spec.rb                    | 27 ++++++++++++++++++++++++--
 2 files changed, 26 insertions(+), 3 deletions(-)

(limited to 'spec')

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/interface_spec.rb b/spec/unit/interface_spec.rb
index b25d06f51..7e6b7de77 100755
--- a/spec/unit/interface_spec.rb
+++ b/spec/unit/interface_spec.rb
@@ -43,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
-- 
cgit 


From 1b4d7a51d10b217c7f67f3876242fff6dc694faa Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 4 Apr 2011 16:46:09 -0700
Subject: (#6962) Create the basic shape of the help face.

This implements the basic help face, along with the start of the support
structures; we include the basic application, and the default help action that
just emits a listing of faces and other discovered stuff...

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/faces/help_spec.rb | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 spec/unit/faces/help_spec.rb

(limited to 'spec')

diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
new file mode 100644
index 000000000..5b611a0ad
--- /dev/null
+++ b/spec/unit/faces/help_spec.rb
@@ -0,0 +1,44 @@
+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, :current).should ==
+      subject.help(:help, '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, face.version).should == subject.help(:huzzah, :current)
+  end
+end
-- 
cgit 


From 4eccd53da90593fad1b929eeea3b5d7d252c553e Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Sun, 10 Apr 2011 19:58:43 -0700
Subject: (#6962) Implement Face#summary support for the help face.

We now use the summary information available in other faces as part of
emitting a list our list of available subcommands.  This is seldom used in
faces, but enough emit the information to prove the concept.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/lib/puppet/faces/huzzah.rb |  1 +
 spec/spec_helper.rb             |  7 +++++++
 spec/unit/faces/help_spec.rb    | 21 +++++++++++++++++++++
 3 files changed, 29 insertions(+)

(limited to 'spec')

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/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
index 5b611a0ad..ad553dc3a 100644
--- a/spec/unit/faces/help_spec.rb
+++ b/spec/unit/faces/help_spec.rb
@@ -41,4 +41,25 @@ describe Puppet::Faces[:help, '0.0.1'] do
     face = Puppet::Faces[:huzzah, :current]
     subject.help(:huzzah, face.version).should == subject.help(:huzzah, :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 have_matching_element %r{ #{name} } }
+      it { should have_matching_element %r{ #{name} +#{summary}} } if summary
+    end
+  end
 end
-- 
cgit 


From 14b1e008c1368e6c56d68f83999472351fd3327a Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 10:57:06 -0700
Subject: (#6992) Expose available_subcommands as a class method.

We previously treated the list of legacy commands as an instance property of
Puppet::Util::CommandLine.  This made it difficult to obtain that information
outside the context of the instantiated application.

In the longer term this should wither and die as we eliminate the legacy
applications entirely, but until then exposing that on the class is the
lightest mechanism for making that useful.

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/util/command_line_spec.rb | 5 +++++
 1 file changed, 5 insertions(+)

(limited to 'spec')

diff --git a/spec/unit/util/command_line_spec.rb b/spec/unit/util/command_line_spec.rb
index ca5f0540f..c5c6e5f3e 100755
--- a/spec/unit/util/command_line_spec.rb
+++ b/spec/unit/util/command_line_spec.rb
@@ -111,6 +111,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
-- 
cgit 


From 36021021b4cb11aea0a5acd35d051db52d8fc99f Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 11:19:33 -0700
Subject: (#6770) Don't pollute valid face list when #face? is called.

We had two conflicting uses of the list of available faces: in the #face?
method we were perfectly happy to create a top level key on any request, but
didn't populate the version set.

Meanwhile, in #faces we treated the set of top level keys as the absolute and
correct list of all *valid* faces, leading to pain and suffering when people
queried for an invalid face, but then expected to enumerate only valid faces.

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/interface/face_collection_spec.rb | 5 +++++
 1 file changed, 5 insertions(+)

(limited to 'spec')

diff --git a/spec/unit/interface/face_collection_spec.rb b/spec/unit/interface/face_collection_spec.rb
index e6e03c3d2..752871035 100755
--- a/spec/unit/interface/face_collection_spec.rb
+++ b/spec/unit/interface/face_collection_spec.rb
@@ -138,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
-- 
cgit 


From d13a938b0abc604a7802cf41ff75a30ad6ccd192 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 10:46:08 -0700
Subject: (#6962) Initial support for legacy applications in help.

We now enumerate and print the list of legacy applications in the unadorned
help action; this allows us a path to migrating forward smoothly while still
providing a good user experience.

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/faces/help_spec.rb | 4 ++++
 1 file changed, 4 insertions(+)

(limited to 'spec')

diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
index ad553dc3a..87ff67948 100644
--- a/spec/unit/faces/help_spec.rb
+++ b/spec/unit/faces/help_spec.rb
@@ -61,5 +61,9 @@ describe Puppet::Faces[:help, '0.0.1'] do
       it { should have_matching_element %r{ #{name} } }
       it { should have_matching_element %r{ #{name} +#{summary}} } if summary
     end
+
+    Puppet::Util::CommandLine.available_subcommands do |name|
+      it { should have_matching_element %r{ #{name} } }
+    end
   end
 end
-- 
cgit 


From 91c29a72e2b728e2d9ba495cd34b7354faa3852b Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 12:00:06 -0700
Subject: (#6962) Extract summary from legacy applications for help.

We use a dubious, but effective, regexp match on the existing man(1) style
help string for the application to extract the summary data.  This should
properly be implemented as a summary method down in the applications
themselves...

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/faces/help_spec.rb | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

(limited to 'spec')

diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
index 87ff67948..1399abfef 100644
--- a/spec/unit/faces/help_spec.rb
+++ b/spec/unit/faces/help_spec.rb
@@ -66,4 +66,38 @@ describe Puppet::Faces[:help, '0.0.1'] do
       it { should have_matching_element %r{ #{name} } }
     end
   end
+
+  context "when listing legacy applications" do
+    let :help do Puppet::Faces[:help, :current] end
+
+    # 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 one legacy application" do
+      help.legacy_applications.should have_at_least(1).item
+    end
+
+    # 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 list the #{name} application" do
+        help.legacy_applications.should_not include name
+      end
+    end
+
+    Puppet::Faces[:help, :current].legacy_applications.each do |appname|
+      it "should list #{appname} in the help output" do
+        help.help.should have_matching_element %r{ #{appname} }
+      end
+
+      summary = Puppet::Faces[:help, :current].horribly_extract_summary_from(appname)
+      if summary then
+        it "should display the summary of #{appname}" do
+          help.help.should have_matching_element %r{ #{summary}\b}
+        end
+      end
+    end
+  end
 end
-- 
cgit 


From cdc5fec3640108ad01e0285b1039dae222590339 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 15:50:24 -0700
Subject: (#6962) Implement 'summary' for actions.

This extends the summary function down through the actions themselves,
allowing us to display a useful summary to the user.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/interface/action_builder_spec.rb | 11 +++++++++++
 1 file changed, 11 insertions(+)

(limited to 'spec')

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
-- 
cgit 


From 657082755a20da801b4c679eff296053380c61b6 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 16:19:21 -0700
Subject: (#6962) Add summary help for actions on an individual face.

We now emit the summary of actions for an individual face, in the same format
as the summary of available faces.  This moves forward through the feature set
defined for the help subcommand.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/faces/help_spec.rb | 2 ++
 1 file changed, 2 insertions(+)

(limited to 'spec')

diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
index 1399abfef..61f1947f3 100644
--- a/spec/unit/faces/help_spec.rb
+++ b/spec/unit/faces/help_spec.rb
@@ -100,4 +100,6 @@ describe Puppet::Faces[:help, '0.0.1'] do
       end
     end
   end
+
+  
 end
-- 
cgit 


From ec988e24ce3713a4c4a31918489136f88b6945e6 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 16:47:16 -0700
Subject: (#6962) Move the logic for help layout into erb templates.

Rather than hard-coding the layout of help output in the code, push it out
into external templates.  At the moment overriding that would require
changing the ERB code next to puppet/faces/help.rb file on disk.

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/faces/help_spec.rb | 2 --
 1 file changed, 2 deletions(-)

(limited to 'spec')

diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
index 61f1947f3..1399abfef 100644
--- a/spec/unit/faces/help_spec.rb
+++ b/spec/unit/faces/help_spec.rb
@@ -100,6 +100,4 @@ describe Puppet::Faces[:help, '0.0.1'] do
       end
     end
   end
-
-  
 end
-- 
cgit 


From 648e3c0dd0fb671b01e54cc0bca202bafc5ae934 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 17:11:25 -0700
Subject: (#6962) Better argument checking for help.

We used to blink and miss the fact that we failed to load an action or face in
the past; now we test for that, and fail with a clear error message when the
user asks for something we can't deliver.

Additionally, fix a couple of tests that were silently broken because they
passed the wrong arguments, but still got some output.

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/faces/help_spec.rb | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

(limited to 'spec')

diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
index 1399abfef..aa811c4b3 100644
--- a/spec/unit/faces/help_spec.rb
+++ b/spec/unit/faces/help_spec.rb
@@ -28,8 +28,8 @@ describe Puppet::Faces[:help, '0.0.1'] do
   end
 
   it "should treat :current and 'current' identically" do
-    subject.help(:help, :current).should ==
-      subject.help(:help, 'current')
+    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
@@ -39,7 +39,8 @@ describe Puppet::Faces[:help, '0.0.1'] do
 
   it "should find a face by version" do
     face = Puppet::Faces[:huzzah, :current]
-    subject.help(:huzzah, face.version).should == subject.help(:huzzah, :current)
+    subject.help(:huzzah, :version => face.version).
+      should == subject.help(:huzzah, :version => :current)
   end
 
   context "when listing subcommands" do
-- 
cgit 


From acbbd52e0e876afa62f9c6098f0f40504b993dce Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 17:24:38 -0700
Subject: (#6962) Clean up testing further.

This refactors common code in the tests out, and takes advantage of the
implicit and explicit subject support in rspec2 to make the testing more
expressive and more efficient.

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/faces/help_spec.rb | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

(limited to 'spec')

diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
index aa811c4b3..f2aeb44e9 100644
--- a/spec/unit/faces/help_spec.rb
+++ b/spec/unit/faces/help_spec.rb
@@ -59,46 +59,32 @@ describe Puppet::Faces[:help, '0.0.1'] do
       face = Puppet::Faces[name, :current]
       summary = face.summary
 
-      it { should have_matching_element %r{ #{name} } }
-      it { should have_matching_element %r{ #{name} +#{summary}} } if summary
+      it { should =~ %r{ #{name} } }
+      it { should =~ %r{ #{name} +#{summary}} } if summary
     end
 
-    Puppet::Util::CommandLine.available_subcommands do |name|
-      it { should have_matching_element %r{ #{name} } }
+    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 "when listing legacy applications" do
-    let :help do Puppet::Faces[:help, :current] 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 one legacy application" do
-      help.legacy_applications.should have_at_least(1).item
-    end
+    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 list the #{name} application" do
-        help.legacy_applications.should_not include name
-      end
-    end
-
-    Puppet::Faces[:help, :current].legacy_applications.each do |appname|
-      it "should list #{appname} in the help output" do
-        help.help.should have_matching_element %r{ #{appname} }
-      end
-
-      summary = Puppet::Faces[:help, :current].horribly_extract_summary_from(appname)
-      if summary then
-        it "should display the summary of #{appname}" do
-          help.help.should have_matching_element %r{ #{summary}\b}
-        end
-      end
+      it { should_not include name }
     end
   end
 end
-- 
cgit 


From fe6d59528d18e9aa989f48d364868a5a105017a5 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 22:04:28 -0700
Subject: (#6962) Integrate legacy subcommands into the help face.

Previously we would only emit help for a face; now we fully integrate legacy
subcommands in the sense that asking for face-level help will emit their
entire help text.

Asking for any action subsequent will raise an error: this is an annoyingly
inconsistent behaviour, but there isn't a saner definition of the change.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/faces/help_spec.rb | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

(limited to 'spec')

diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
index f2aeb44e9..cd74a5bf1 100644
--- a/spec/unit/faces/help_spec.rb
+++ b/spec/unit/faces/help_spec.rb
@@ -87,4 +87,26 @@ describe Puppet::Faces[:help, '0.0.1'] do
       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
-- 
cgit 


From 9496067f5644972def823cf8c3318aca137b86fe Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 22:25:58 -0700
Subject: maint: avoid making temporary dirs during testing.

We used to create temporary directories to have some support files on disk
during testing of faces; we don't really need that most of the time, and this
updates a test to reflect that reality.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/lib/puppet/faces/basetest.rb        |  1 +
 spec/unit/application/faces_base_spec.rb | 14 +-------------
 2 files changed, 2 insertions(+), 13 deletions(-)
 create mode 100644 spec/lib/puppet/faces/basetest.rb

(limited to 'spec')

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/unit/application/faces_base_spec.rb b/spec/unit/application/faces_base_spec.rb
index b7a11ad56..a6df55aef 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,11 +21,6 @@ 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)
-- 
cgit 


From 826d5dff531eb624fef91a7298932e9ec5a46231 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 23:09:18 -0700
Subject: (#6962) delegate global usage to the help face.

The global --help handler (also invoked when puppet is run without any other
command line options at all) used to spit out a brief and generally not so
helpful message.

Now that we have a help face that can provide the same information in a much
more user-friendly form, we should delegate the function to that when
required.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/application/cert_spec.rb  | 14 +-------------
 spec/unit/util/command_line_spec.rb |  7 ++++---
 2 files changed, 5 insertions(+), 16 deletions(-)

(limited to 'spec')

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/util/command_line_spec.rb b/spec/unit/util/command_line_spec.rb
index c5c6e5f3e..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
-- 
cgit 


From 7b4d9367b391f75983868046d30928ebc8411f50 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Tue, 12 Apr 2011 11:45:05 -0700
Subject: (#6962) Move option handling into #parse_options, not #preinit.

Logically, the extra work around option parsing for faces belongs in the
application parse_options method, not hidden in the step before.  This commit
moves that to the right place and fixes the fallout from that strange early
design decision.

Along the way we unify error reporting for invalid options so that all the
code paths result in the same externally detected failures.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/application/certificate_spec.rb |  5 ++-
 spec/unit/application/faces_base_spec.rb  | 60 +++++++++++++++++--------------
 spec/unit/application_spec.rb             | 10 ------
 3 files changed, 38 insertions(+), 37 deletions(-)

(limited to 'spec')

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 a6df55aef..18bd30295 100755
--- a/spec/unit/application/faces_base_spec.rb
+++ b/spec/unit/application/faces_base_spec.rb
@@ -23,9 +23,7 @@ describe Puppet::Application::FacesBase do
 
   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
@@ -39,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
@@ -56,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
@@ -73,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},
@@ -128,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
@@ -138,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
@@ -148,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
@@ -174,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_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
-- 
cgit