Merge branch 'pr/3261'

* pr/3261:
  (maint) Refactor tests to use helper to create module directory
  (maint) Verify module exception is logged
  (maint) Avoid checking for strings in module_references
  (maint) Log invalid modules

Author: joshcooper

lib/puppet/node/environment.rb

@@ -343,8 +343,8 @@ def module_by_forge_name(forge_name)
     seen_modules = {}
     modulepath.each do |path|
       Dir.entries(path).each do |name|
+        next if name == "." || name == ".."
         warn_about_mistaken_path(path, name)
-        next if module_references.include?(name)
         if not seen_modules[name]
           module_references << {:name => name, :path => File.join(path, name)}
           seen_modules[name] = true
@@ -355,7 +355,8 @@ def module_by_forge_name(forge_name)
     module_references.collect do |reference|
       begin
         Puppet::Module.new(reference[:name], reference[:path], self)
-      rescue Puppet::Module::Error
+      rescue Puppet::Module::Error => e
+        Puppet.log_exception(e)
         nil
       end
     end.compact

spec/lib/puppet_spec/modules.rb

@@ -22,5 +22,16 @@ def create(name, dir, options = {})
 
       Puppet::Module.new(name, module_dir, environment)
     end
+
+    def generate_files(name, dir, options = {})
+      module_dir = File.join(dir, name)
+      FileUtils.mkdir_p(module_dir)
+
+      if metadata = options[:metadata]
+        File.open(File.join(module_dir, 'metadata.json'), 'w') do |f|
+          f.write(metadata.to_pson)
+        end
+      end
+    end
   end
 end

spec/unit/node/environment_spec.rb

@@ -331,16 +331,16 @@ def assert_manifest_conflict(expectation, envconf_manifest_value)
           end
 
           it "should ignore modules with invalid names" do
-            FileUtils.mkdir_p(File.join(@first, 'foo'))
-            FileUtils.mkdir_p(File.join(@first, 'foo2'))
-            FileUtils.mkdir_p(File.join(@first, 'foo-bar'))
-            FileUtils.mkdir_p(File.join(@first, 'foo_bar'))
-            FileUtils.mkdir_p(File.join(@first, 'foo=bar'))
-            FileUtils.mkdir_p(File.join(@first, 'foo bar'))
-            FileUtils.mkdir_p(File.join(@first, 'foo.bar'))
-            FileUtils.mkdir_p(File.join(@first, '-foo'))
-            FileUtils.mkdir_p(File.join(@first, 'foo-'))
-            FileUtils.mkdir_p(File.join(@first, 'foo--bar'))
+            PuppetSpec::Modules.generate_files('foo', @first)
+            PuppetSpec::Modules.generate_files('foo2', @first)
+            PuppetSpec::Modules.generate_files('foo-bar', @first)
+            PuppetSpec::Modules.generate_files('foo_bar', @first)
+            PuppetSpec::Modules.generate_files('foo=bar', @first)
+            PuppetSpec::Modules.generate_files('foo bar', @first)
+            PuppetSpec::Modules.generate_files('foo.bar', @first)
+            PuppetSpec::Modules.generate_files('-foo', @first)
+            PuppetSpec::Modules.generate_files('foo-', @first)
+            PuppetSpec::Modules.generate_files('foo--bar', @first)
 
             env.modules_by_path[@first].collect{|mod| mod.name}.sort.should == %w{foo foo-bar foo2 foo_bar}
           end
@@ -446,37 +446,52 @@ def assert_manifest_conflict(expectation, envconf_manifest_value)
 
           it "should return a module named for every directory in each module path" do
             %w{foo bar}.each do |mod_name|
-              FileUtils.mkdir_p(File.join(@first, mod_name))
+              PuppetSpec::Modules.generate_files(mod_name, @first)
             end
             %w{bee baz}.each do |mod_name|
-              FileUtils.mkdir_p(File.join(@second, mod_name))
+              PuppetSpec::Modules.generate_files(mod_name, @second)
             end
             env.modules.collect{|mod| mod.name}.sort.should == %w{foo bar bee baz}.sort
           end
 
           it "should remove duplicates" do
-            FileUtils.mkdir_p(File.join(@first,  'foo'))
-            FileUtils.mkdir_p(File.join(@second, 'foo'))
+            PuppetSpec::Modules.generate_files('foo', @first)
+            PuppetSpec::Modules.generate_files('foo', @second)
 
             env.modules.collect{|mod| mod.name}.sort.should == %w{foo}
           end
 
           it "should ignore modules with invalid names" do
-            FileUtils.mkdir_p(File.join(@first, 'foo'))
-            FileUtils.mkdir_p(File.join(@first, 'foo2'))
-            FileUtils.mkdir_p(File.join(@first, 'foo-bar'))
-            FileUtils.mkdir_p(File.join(@first, 'foo_bar'))
-            FileUtils.mkdir_p(File.join(@first, 'foo=bar'))
-            FileUtils.mkdir_p(File.join(@first, 'foo bar'))
+            PuppetSpec::Modules.generate_files('foo', @first)
+            PuppetSpec::Modules.generate_files('foo2', @first)
+            PuppetSpec::Modules.generate_files('foo-bar', @first)
+            PuppetSpec::Modules.generate_files('foo_bar', @first)
+            PuppetSpec::Modules.generate_files('foo=bar', @first)
+            PuppetSpec::Modules.generate_files('foo bar', @first)
 
             env.modules.collect{|mod| mod.name}.sort.should == %w{foo foo-bar foo2 foo_bar}
           end
 
           it "should create modules with the correct environment" do
-            FileUtils.mkdir_p(File.join(@first, 'foo'))
+            PuppetSpec::Modules.generate_files('foo', @first)
+
             env.modules.each {|mod| mod.environment.should == env }
           end
 
+          it "should log an exception if a module contains invalid metadata" do
+            PuppetSpec::Modules.generate_files(
+              'foo',
+              @first,
+              :metadata => {
+                :author       => 'puppetlabs'
+                # missing source, version, etc
+              }
+            )
+
+            Puppet.expects(:log_exception).with(is_a(Puppet::Module::MissingMetadata))
+
+            env.modules
+          end
         end
       end
     end