Fixing #3407 Failing tests in spec/unit/node/environment.rb

A naked rescue in Puppet::Node::Environment was hiding expectation
violations from the Mocha mocks.
Specifically, 'modulepath' expectations were failing, as Puppet::Module now calls
Puppet::Node::Environment#modulepath internally.

Signed-off-by: Jesse Wolfe <jes5199@gmail.com>

3 files changed, 21 insertions, 19 deletions

diff --git a/lib/puppet/module.rb b/lib/puppet/module.rb
index b0d80caa6..f30b862ef 100644
--- a/lib/puppet/module.rb
+++ b/lib/puppet/module.rb
@@ -2,20 +2,16 @@ require 'puppet/util/logging'
 
 # Support for modules
 class Puppet::Module
-    class MissingModule < Puppet::Error; end
-    class IncompatibleModule < Puppet::Error; end
-    class UnsupportedPlatform < Puppet::Error; end
-    class IncompatiblePlatform < Puppet::Error; end
-    class MissingMetadata < Puppet::Error; end
+    class Error < Puppet::Error; end
+    class MissingModule < Error; end
+    class IncompatibleModule < Error; end
+    class UnsupportedPlatform < Error; end
+    class IncompatiblePlatform < Error; end
+    class MissingMetadata < Error; end
+    class InvalidName < Error; end
 
     include Puppet::Util::Logging
 
-    class InvalidName < ArgumentError
-        def message
-            "Invalid module name; module names must be alphanumeric (plus '-')"
-        end
-    end
-
     TEMPLATES = "templates"
     FILES = "files"
     MANIFESTS = "manifests"
@@ -215,6 +211,6 @@ class Puppet::Module
     end
 
     def assert_validity
-        raise InvalidName unless name =~ /^[-\w]+$/
+        raise InvalidName, "Invalid module name; module names must be alphanumeric (plus '-')" unless name =~ /^[-\w]+$/
     end
 end
diff --git a/lib/puppet/node/environment.rb b/lib/puppet/node/environment.rb
index 94f899200..395d50657 100644
--- a/lib/puppet/node/environment.rb
+++ b/lib/puppet/node/environment.rb
@@ -62,7 +62,13 @@ class Puppet::Node::Environment
     # Cache the list, because it can be expensive to create.
     cached_attr(:modules, :ttl => Puppet[:filetimeout]) do
         module_names = modulepath.collect { |path| Dir.entries(path) }.flatten.uniq
-        module_names.collect { |path| Puppet::Module.new(path, self) rescue nil }.compact
+        module_names.collect do |path|
+            begin
+                Puppet::Module.new(path, self)
+            rescue Puppet::Module::Error => e
+                nil
+            end
+        end.compact
     end
 
     # Cache the manifestdir, so that we aren't searching through
diff --git a/spec/unit/node/environment.rb b/spec/unit/node/environment.rb
index 21c224f23..b9bfcafc8 100755
--- a/spec/unit/node/environment.rb
+++ b/spec/unit/node/environment.rb
@@ -139,7 +139,7 @@ describe Puppet::Node::Environment do
         describe ".modules" do
             it "should return a module named for every directory in each module path" do
                 env = Puppet::Node::Environment.new("testing")
-                env.expects(:modulepath).returns %w{/a /b}
+                env.expects(:modulepath).at_least_once.returns %w{/a /b}
                 Dir.expects(:entries).with("/a").returns %w{foo bar}
                 Dir.expects(:entries).with("/b").returns %w{bee baz}
 
@@ -148,7 +148,7 @@ describe Puppet::Node::Environment do
 
             it "should remove duplicates" do
                 env = Puppet::Node::Environment.new("testing")
-                env.expects(:modulepath).returns %w{/a /b}
+                env.expects(:modulepath).returns( %w{/a /b} ).at_least_once
                 Dir.expects(:entries).with("/a").returns %w{foo}
                 Dir.expects(:entries).with("/b").returns %w{foo}
 
@@ -157,18 +157,18 @@ describe Puppet::Node::Environment do
 
             it "should ignore invalid modules" do
                 env = Puppet::Node::Environment.new("testing")
-                env.expects(:modulepath).returns %w{/a}
+                env.expects(:modulepath).returns( %w{/a} )
                 Dir.expects(:entries).with("/a").returns %w{foo bar}
 
                 Puppet::Module.expects(:new).with { |name, env| name == "foo" }.returns mock("foomod", :name => "foo")
-                Puppet::Module.expects(:new).with { |name, env| name == "bar" }.raises Puppet::Module::InvalidName
+                Puppet::Module.expects(:new).with { |name, env| name == "bar" }.raises( Puppet::Module::InvalidName, "name is invalid" )
 
                 env.modules.collect{|mod| mod.name}.sort.should == %w{foo}
             end
 
             it "should create modules with the correct environment" do
                 env = Puppet::Node::Environment.new("testing")
-                env.expects(:modulepath).returns %w{/a}
+                env.expects(:modulepath).at_least_once.returns %w{/a}
                 Dir.expects(:entries).with("/a").returns %w{foo}
 
                 env.modules.each {|mod| mod.environment.should == env }
@@ -176,7 +176,7 @@ describe Puppet::Node::Environment do
 
             it "should cache the module list" do
                 env = Puppet::Node::Environment.new("testing")
-                env.expects(:modulepath).once.returns %w{/a}
+                env.expects(:modulepath).at_least_once.returns %w{/a}
                 Dir.expects(:entries).once.with("/a").returns %w{foo}
 
                 env.modules