(GH-768) Fix in_module_root? gives false positives

lib/pdk/util.rb

@@ -21,7 +21,9 @@ module Util
 
     MODULE_FOLDERS = %w[
       manifests
-      lib
+      lib/puppet
+      lib/puppet_x
+      lib/facter
       tasks
       facts.d
       functions
@@ -157,12 +159,13 @@ def module_fixtures_dir
     module_function :module_fixtures_dir
 
     # Returns true or false depending on if any of the common directories in a module
-    # are found in the current directory
+    # are found in the specified directory. If a directory is not specified, the current
+    # working directory is used.
     #
     # @return [boolean] True if any folders from MODULE_FOLDERS are found in the current dir,
     #   false otherwise.
-    def in_module_root?
-      PDK::Util::MODULE_FOLDERS.any? { |dir| File.directory?(dir) }
+    def in_module_root?(path = Dir.pwd)
+      PDK::Util::MODULE_FOLDERS.any? { |dir| PDK::Util::Filesystem.directory?(File.join(path, dir)) }
     end
     module_function :in_module_root?
 

spec/spec_helper.rb

@@ -37,6 +37,7 @@
 analytics_config = nil
 
 FIXTURES_DIR = File.join(__dir__, 'fixtures')
+EMPTY_MODULE_ROOT = File.join(FIXTURES_DIR, 'module_root')
 
 RSpec.shared_context :stubbed_logger do
   let(:logger) { instance_double('PDK::Logger').as_null_object }

spec/unit/pdk/cli/exec/interactive_command_spec.rb

@@ -105,6 +105,7 @@
     context 'when running in the :module context' do
       before(:each) do
         command.context = :module
+        allow(PDK::Util).to receive(:module_root).and_return(EMPTY_MODULE_ROOT)
       end
 
       it 'changes into the modroot path and then returns to original pwd' do
@@ -143,6 +144,7 @@
     context 'when running in the :pwd context' do
       before(:each) do
         command.context = :pwd
+        allow(PDK::Util).to receive(:module_root).and_return(EMPTY_MODULE_ROOT)
       end
 
       it 'does not change out of pwd' do

spec/unit/pdk/cli/test/unit_spec.rb

@@ -283,6 +283,7 @@
       allow(PDK::Util::PuppetVersion).to receive(:fetch_puppet_dev).and_return(nil)
       allow(PDK::Test::Unit).to receive(:invoke).and_return(0)
       allow(PDK::CLI::Util).to receive(:module_version_check)
+      allow(PDK::Util).to receive(:module_root).and_return(EMPTY_MODULE_ROOT)
     end
 
     it 'activates puppet github source' do
@@ -346,6 +347,7 @@
       allow(PDK::CLI::Util).to receive(:puppet_from_opts_or_env).with(hash_including(:'puppet-version' => puppet_version)).and_return(puppet_env)
       allow(PDK::Test::Unit).to receive(:invoke).and_return(0)
       allow(PDK::CLI::Util).to receive(:module_version_check)
+      allow(PDK::Util).to receive(:module_root).and_return(EMPTY_MODULE_ROOT)
     end
 
     it 'activates resolved puppet version' do
@@ -391,6 +393,7 @@
       allow(PDK::CLI::Util).to receive(:puppet_from_opts_or_env).with(hash_including(:'pe-version' => pe_version)).and_return(puppet_env)
       allow(PDK::Test::Unit).to receive(:invoke).and_return(0)
       allow(PDK::CLI::Util).to receive(:module_version_check)
+      allow(PDK::Util).to receive(:module_root).and_return(EMPTY_MODULE_ROOT)
     end
 
     it 'activates resolved puppet version' do

spec/unit/pdk/cli/util_spec.rb

@@ -27,15 +27,10 @@
       context 'when passed :check_module_layout => true' do
         let(:options) { { check_module_layout: true } }
 
-        %w[manifests lib tasks facts.d functions types].each do |dir|
-          context "when the current directory contains a '#{dir}' directory" do
-            before(:each) do
-              allow(File).to receive(:directory?).with(dir).and_return(true)
-            end
-
-            it 'does not raise an error' do
-              expect { ensure_in_module }.not_to raise_error
-            end
+        context 'when the current directory does contain a module layout' do
+          it 'raises an error' do
+            allow(PDK::Util).to receive(:in_module_root?).and_return(true)
+            expect { ensure_in_module }.not_to raise_error
           end
         end
 

spec/unit/pdk/report_spec.rb

@@ -85,6 +85,10 @@
           ]
         end
 
+        before(:each) do
+          allow(PDK::Util).to receive(:module_root).and_return(EMPTY_MODULE_ROOT)
+        end
+
         it 'prints the coverage report last' do
           expect(text_report.split("\n")[-1]).to eq('coverage report text')
         end

spec/unit/pdk/util_spec.rb

@@ -360,6 +360,51 @@
     end
   end
 
+  describe '.in_module_root?' do
+    subject { described_class.module_root }
+
+    # We use NUL here because that should never be a valid directory name. But it will work with RSpec mocking.
+    let(:test_path) { '\x00path/test' }
+
+    before(:each) do
+      allow(PDK::Util::Filesystem).to receive(:directory?).and_call_original
+    end
+
+    # Directories which indicate a module root
+    %w[
+      manifests
+      lib/puppet
+      lib/puppet_x
+      lib/facter
+      tasks
+      facts.d
+      functions
+      types
+    ].each do |testcase|
+      it "detects #{testcase} as being in the root of a module" do
+        allow(PDK::Util::Filesystem).to receive(:directory?).with(File.join(test_path, testcase)).and_return(true)
+        expect(described_class.in_module_root?(test_path)).to eq(true)
+      end
+    end
+
+    # Directories which do not indicate a module root
+    %w[
+      lib
+      Boltdir
+      puppet
+    ].each do |testcase|
+      it "detects #{testcase} as not being in the root of a module" do
+        allow(PDK::Util::Filesystem).to receive(:directory?).with(File.join(test_path, testcase)).and_return(true)
+        expect(described_class.in_module_root?(test_path)).to eq(false)
+      end
+    end
+
+    it 'uses the current directory if a directory is not specified' do
+      expect(PDK::Util::Filesystem).to receive(:directory?) { |path| expect(path).to start_with(Dir.pwd) }.at_least(:once)
+      described_class.in_module_root?
+    end
+  end
+
   describe '.find_first_json_in' do
     it 'returns JSON from start of a string' do
       text = '{"version":"3.6.0", "examples":[]}bar'
@@ -421,6 +466,7 @@
     subject(:result) { described_class.module_metadata }
 
     before(:each) do
+      allow(described_class).to receive(:module_root).and_return(EMPTY_MODULE_ROOT)
       allow(PDK::Module::Metadata).to receive(:from_file).with(File.join(described_class.module_root, 'metadata.json')).and_return(mock_metadata)
     end
 

spec/unit/pdk/validate/base_validator_spec.rb

@@ -29,6 +29,7 @@
     context 'when validating less than 1000 targets' do
       before(:each) do
         allow(described_class).to receive(:parse_targets).and_return([(1..999).map(&:to_s), [], []])
+        allow(PDK::Util).to receive(:module_root).and_return(EMPTY_MODULE_ROOT)
       end
 
       it 'executes the validator once' do
@@ -51,6 +52,7 @@
     context 'when validating more than 1000 targets' do
       before(:each) do
         allow(described_class).to receive(:parse_targets).and_return([(1..1001).map(&:to_s), [], []])
+        allow(PDK::Util).to receive(:module_root).and_return(EMPTY_MODULE_ROOT)
       end
 
       it 'executes the validator for each block of up to 1000 targets' do

spec/unit/pdk/validate/puppet_validator_spec.rb

@@ -36,6 +36,7 @@
       context 'and the PuppetLint validator succeeds' do
         before(:each) do
           allow(PDK::Validate::PuppetLint).to receive(:invoke).with(report, anything).and_return(0)
+          allow(PDK::Util).to receive(:module_root).and_return(EMPTY_MODULE_ROOT)
         end
 
         it 'returns 0' do

spec/unit/pdk/validate/tasks_validator_spec.rb

@@ -7,6 +7,10 @@
   describe '.invoke' do
     subject(:return_value) { described_class.invoke(report, {}) }
 
+    before(:each) do
+      allow(PDK::Util).to receive(:module_root).and_return(EMPTY_MODULE_ROOT)
+    end
+
     context 'when the metadata validator succeeds' do
       before(:each) do
         allow(PDK::Validate::Tasks::MetadataLint).to receive(:invoke).with(report, anything).and_return(0)