From 1230168113bd86509112b1509e8fbaea1a250248 Mon Sep 17 00:00:00 2001
From: Colin Surprenant <colin.surprenant@gmail.com>
Date: Wed, 30 Aug 2017 13:19:01 -0400
Subject: [PATCH] fixes for Windows platform tests/specs

use rm_rf to delete dir and shutdown pipeline after run

avoid the use of rescue nil, bad practice

do not mock close as it prevents closing the file an prevents removing it on Windows

cleanup temporary files and relax file name check for Windows

fix paths handling for Windows

flag the read file string as UTF-8 which is what we expect

fix Windows issues

ignore load_average on windows

windows safe URI

cleanup and fix file handling for windows

wait for pipeline shutdown to complete

revert to original puts

cleanups

use environment for windows platform check

fix hash path

wait for pipeline thread to complete after shutdown
---
 lib/pluginmanager/utils/downloader.rb         |  2 +-
 .../lib/logstash/config/source/local.rb       |  3 +-
 logstash-core/spec/logstash/agent_spec.rb     | 49 +++++++++++++------
 .../logstash/api/modules/node_stats_spec.rb   |  6 ++-
 .../spec/logstash/config/source/local_spec.rb |  5 +-
 .../logstash/pipeline_action/create_spec.rb   |  5 +-
 .../logstash/pipeline_action/reload_spec.rb   |  5 +-
 .../spec/logstash/pipeline_dlq_commit_spec.rb |  4 +-
 .../settings/writable_directory_spec.rb       | 23 +++++----
 .../pack_fetch_strategy/uri_spec.rb           |  4 +-
 .../prepare_offline_pack_spec.rb              | 11 ++++-
 .../plugin_manager/utils/downloader_spec.rb   |  2 +-
 12 files changed, 83 insertions(+), 36 deletions(-)

diff --git a/lib/pluginmanager/utils/downloader.rb b/lib/pluginmanager/utils/downloader.rb
index 0d520febf..d2092f221 100644
--- a/lib/pluginmanager/utils/downloader.rb
+++ b/lib/pluginmanager/utils/downloader.rb
@@ -68,7 +68,7 @@ module LogStash module PluginManager module Utils
           downloaded_file.path
         end
       rescue => e
-        downloaded_file.close rescue nil
+        downloaded_file.close unless downloaded_file.closed?
         FileUtils.rm_rf(download_to)
         raise e
       end
diff --git a/logstash-core/lib/logstash/config/source/local.rb b/logstash-core/lib/logstash/config/source/local.rb
index 4dc46414e..897738e12 100644
--- a/logstash-core/lib/logstash/config/source/local.rb
+++ b/logstash-core/lib/logstash/config/source/local.rb
@@ -71,7 +71,8 @@ module LogStash module Config module Source
           end
 
           config_string = ::File.read(file)
-
+          config_string.force_encoding("UTF-8")
+          
           if config_string.valid_encoding?
             part = org.logstash.common.SourceWithMetadata.new("file", file, 0, 0, config_string)
             config_parts << part
diff --git a/logstash-core/spec/logstash/agent_spec.rb b/logstash-core/spec/logstash/agent_spec.rb
index 7cf5c95f2..4b75cecf1 100644
--- a/logstash-core/spec/logstash/agent_spec.rb
+++ b/logstash-core/spec/logstash/agent_spec.rb
@@ -33,7 +33,7 @@ describe LogStash::Agent do
     # This MUST run first, before `subject` is invoked to ensure clean state
     clear_data_dir
 
-    File.open(config_file, "w") { |f| f.puts config_file_txt }
+    File.open(config_file, "w") { |f| f.puts(config_file_txt) }
 
     agent_args.each do |key, value|
       agent_settings.set(key, value)
@@ -50,8 +50,9 @@ describe LogStash::Agent do
   after :each do
     subject.shutdown
     LogStash::SETTINGS.reset
-    File.unlink(config_file)
-    File.unlink(subject.id_path)
+
+    FileUtils.rm(config_file)
+    FileUtils.rm_rf(subject.id_path)
   end
 
   it "fallback to hostname when no name is provided" do
@@ -314,8 +315,10 @@ describe LogStash::Agent do
   context "metrics after config reloading" do
 
     let(:initial_generator_threshold) { 1000 }
-    let(:temporary_file) { Stud::Temporary.file.path }
-    let(:config_file_txt) { "input { generator { count => #{initial_generator_threshold*2} } } output { file { path => '#{temporary_file}'} }" }
+    let(:original_config_output) { Stud::Temporary.pathname }
+    let(:new_config_output) { Stud::Temporary.pathname }
+
+    let(:config_file_txt) { "input { generator { count => #{initial_generator_threshold*2} } } output { file { path => '#{original_config_output}'} }" }
 
     let(:agent_args) do
       {
@@ -326,14 +329,25 @@ describe LogStash::Agent do
 
     subject { described_class.new(agent_settings, default_source_loader) }
 
+    let(:agent_thread) do
+      # subject has to be called for the first time outside the thread because it could create a race condition
+      # with subsequent subject calls
+      s = subject
+      Thread.new { s.execute }
+    end
+
     before(:each) do
       @abort_on_exception = Thread.abort_on_exception
       Thread.abort_on_exception = true
 
-      @t = Thread.new { subject.execute }
+      agent_thread
 
       # wait for some events to reach the dummy_output
-      sleep(0.01) until IO.readlines(temporary_file).size > initial_generator_threshold
+      Timeout.timeout(timeout) do
+        # wait for file existence otherwise it will raise exception on Windows
+        sleep(0.1) until ::File.exist?(original_config_output)
+        sleep(0.1) until IO.readlines(original_config_output).size > initial_generator_threshold
+      end
 
       # write new config
       File.open(config_file, "w") { |f| f.write(new_config) }
@@ -341,10 +355,14 @@ describe LogStash::Agent do
 
     after :each do
       begin
+        Stud.stop!(agent_thread) rescue nil # it may be dead already
+        agent_thread.join
         subject.shutdown
-        Stud.stop!(@t) rescue nil # it may be dead already
-        @t.join
-        File.unlink(temporary_file)
+
+        FileUtils.rm(original_config_output)
+        FileUtils.rm(new_config_output) if File.exist?(new_config_output)
+      rescue
+          #don't care about errors here.
       ensure
         Thread.abort_on_exception = @abort_on_exception
       end
@@ -352,19 +370,20 @@ describe LogStash::Agent do
 
     context "when reloading a good config" do
       let(:new_config_generator_counter) { 500 }
-      let(:new_file) { Stud::Temporary.file.path }
-      let(:new_config) { "input { generator { count => #{new_config_generator_counter} } } output { file { path => '#{new_file}'} }" }
+      let(:new_config) { "input { generator { count => #{new_config_generator_counter} } } output { file { path => '#{new_config_output}'} }" }
 
       before :each do
         subject.converge_state_and_update
-        sleep(0.01) while ::File.read(new_file).chomp.empty?
+        Timeout.timeout(timeout) do
+          # wait for file existence otherwise it will raise exception on Windows
+          sleep(0.1) until ::File.exist?(new_config_output)
+          sleep(0.1) while ::File.read(new_config_output).chomp.empty?
+        end
         # ensure the converge_state_and_update method has updated metrics by
         # invoking the mutex
         subject.running_pipelines?
       end
 
-      after(:each) { File.unlink(new_file) }
-
       it "resets the pipeline metric collector" do
         snapshot = subject.metric.collector.snapshot_metric
         value = snapshot.metric_store.get_with_path("/stats/pipelines")[:stats][:pipelines][:main][:events][:in].value
diff --git a/logstash-core/spec/logstash/api/modules/node_stats_spec.rb b/logstash-core/spec/logstash/api/modules/node_stats_spec.rb
index 340c680f3..729f9aee9 100644
--- a/logstash-core/spec/logstash/api/modules/node_stats_spec.rb
+++ b/logstash-core/spec/logstash/api/modules/node_stats_spec.rb
@@ -69,7 +69,7 @@ describe LogStash::Api::Modules::NodeStats do
       "cpu"=>{
         "total_in_millis"=>Numeric,
         "percent"=>Numeric,
-        "load_average" => { "1m" => Numeric }
+        # load_average is not supported on Windows, set it below
       }
     },
    "pipelines" => {
@@ -89,5 +89,9 @@ describe LogStash::Api::Modules::NodeStats do
    }
   }
 
+  unless LogStash::Environment.windows?
+    root_structure["process"]["cpu"]["load_average"] = { "1m" => Numeric }
+  end
+
   test_api_and_resources(root_structure)
 end
diff --git a/logstash-core/spec/logstash/config/source/local_spec.rb b/logstash-core/spec/logstash/config/source/local_spec.rb
index 9e3bf8659..2d5574839 100644
--- a/logstash-core/spec/logstash/config/source/local_spec.rb
+++ b/logstash-core/spec/logstash/config/source/local_spec.rb
@@ -79,7 +79,7 @@ describe LogStash::Config::Source::Local::ConfigPathLoader do
 
         parts.each do |part|
           basename = ::File.basename(part.id)
-          file_path = ::File.join(directory, basename)
+          file_path = ::File.expand_path(::File.join(directory, basename))
           content = files[basename]
           expect(part).to be_a_source_with_metadata("file", file_path, content)
         end
@@ -99,7 +99,8 @@ describe LogStash::Config::Source::Local::ConfigPathLoader do
       end
 
       it "raises an exception" do
-        expect { subject.read(file_path) }.to raise_error LogStash::ConfigLoadingError, /#{file_path}/
+        # check against base name because on Windows long paths are shrinked in the exception message
+        expect { subject.read(file_path) }.to raise_error LogStash::ConfigLoadingError, /.+#{::File.basename(file_path)}/
       end
     end
 
diff --git a/logstash-core/spec/logstash/pipeline_action/create_spec.rb b/logstash-core/spec/logstash/pipeline_action/create_spec.rb
index 90627a478..b59188130 100644
--- a/logstash-core/spec/logstash/pipeline_action/create_spec.rb
+++ b/logstash-core/spec/logstash/pipeline_action/create_spec.rb
@@ -19,7 +19,10 @@ describe LogStash::PipelineAction::Create do
   subject { described_class.new(pipeline_config, metric) }
 
   after do
-    pipelines.each { |_, pipeline| pipeline.shutdown }
+    pipelines.each do |_, pipeline| 
+      pipeline.shutdown 
+      pipeline.thread.join
+    end
   end
 
   it "returns the pipeline_id" do
diff --git a/logstash-core/spec/logstash/pipeline_action/reload_spec.rb b/logstash-core/spec/logstash/pipeline_action/reload_spec.rb
index fc2db33bb..60bb59686 100644
--- a/logstash-core/spec/logstash/pipeline_action/reload_spec.rb
+++ b/logstash-core/spec/logstash/pipeline_action/reload_spec.rb
@@ -22,7 +22,10 @@ describe LogStash::PipelineAction::Reload do
   end
 
   after do
-    pipelines.each { |_, pipeline| pipeline.shutdown }
+    pipelines.each do |_, pipeline| 
+      pipeline.shutdown
+      pipeline.thread.join
+    end
   end
 
   it "returns the pipeline_id" do
diff --git a/logstash-core/spec/logstash/pipeline_dlq_commit_spec.rb b/logstash-core/spec/logstash/pipeline_dlq_commit_spec.rb
index 83d1e5d16..c8643c528 100644
--- a/logstash-core/spec/logstash/pipeline_dlq_commit_spec.rb
+++ b/logstash-core/spec/logstash/pipeline_dlq_commit_spec.rb
@@ -67,7 +67,7 @@ describe LogStash::Pipeline do
   end
 
   after(:each) do
-    FileUtils.remove_entry pipeline_settings["path.dead_letter_queue"]
+    FileUtils.rm_rf(pipeline_settings["path.dead_letter_queue"])
   end
 
   context "dlq is enabled" do
@@ -85,6 +85,7 @@ describe LogStash::Pipeline do
       entry = dlq_reader.pollEntry(40)
       expect(entry).to_not be_nil
       expect(entry.reason).to eq("my reason")
+      subject.shutdown
     end
   end
 
@@ -101,6 +102,7 @@ describe LogStash::Pipeline do
       subject.run
       dlq_path = java.nio.file.Paths.get(pipeline_settings_obj.get("path.dead_letter_queue"), pipeline_id)
       expect(java.nio.file.Files.exists(dlq_path)).to eq(false)
+      subject.shutdown
     end
   end
 
diff --git a/logstash-core/spec/logstash/settings/writable_directory_spec.rb b/logstash-core/spec/logstash/settings/writable_directory_spec.rb
index 4463ca82d..be00ce9f0 100644
--- a/logstash-core/spec/logstash/settings/writable_directory_spec.rb
+++ b/logstash-core/spec/logstash/settings/writable_directory_spec.rb
@@ -3,17 +3,17 @@ require "spec_helper"
 require "logstash/settings"
 require "tmpdir"
 require "socket" # for UNIXSocket
+require "fileutils"
 
 describe LogStash::Setting::WritableDirectory do
-  let(:mode_rx) { 0555 }
   # linux is 108, Macos is 104, so use a safe value
   # Stud::Temporary.pathname, will exceed that size without adding anything
   let(:parent) { File.join(Dir.tmpdir, Time.now.to_f.to_s) }
   let(:path) { File.join(parent, "fancy") }
 
   before { Dir.mkdir(parent) }
-  after { Dir.exist?(path) && Dir.unlink(path) rescue nil }
-  after { Dir.unlink(parent) }
+  after { Dir.exist?(path) && FileUtils.rm_rf(path)}
+  after { FileUtils.rm_rf(parent) }
 
   shared_examples "failure" do
     before { subject.set(path) }
@@ -44,8 +44,9 @@ describe LogStash::Setting::WritableDirectory do
       end
 
       context "and the directory cannot be created" do
-        before { File.chmod(mode_rx, parent) }
         it "should fail" do
+          # using chmod does not work on Windows better mock and_raise("message")
+          expect(FileUtils).to receive(:mkdir_p).and_raise("foobar")
           expect { subject.value }.to raise_error
         end
       end
@@ -66,7 +67,8 @@ describe LogStash::Setting::WritableDirectory do
       end
 
       context "but is not writable" do
-        before { File.chmod(0, path) }
+        # chmod does not work on Windows, mock writable? instead
+        before { expect(File).to receive(:writable?).and_return(false) }
         it_behaves_like "failure"
       end
     end
@@ -84,12 +86,13 @@ describe LogStash::Setting::WritableDirectory do
         before { socket } # realize `socket` value
         after { socket.close }
         it_behaves_like "failure"
-      end
+      end unless LogStash::Environment.windows?
 
+      
       context "but is a symlink" do
-        before { File::symlink("whatever", path) }
+        before { FileUtils.symlink("whatever", path) }
         it_behaves_like "failure"
-      end
+      end unless LogStash::Environment.windows?
     end
 
     context "when the directory is missing" do
@@ -114,8 +117,8 @@ describe LogStash::Setting::WritableDirectory do
 
       context "and cannot be created" do
         before do
-          # Remove write permission on the parent
-          File.chmod(mode_rx, parent)
+          # chmod does not work on Windows, mock writable? instead
+          expect(File).to receive(:writable?).and_return(false)
         end
 
         it_behaves_like "failure"
diff --git a/spec/unit/plugin_manager/pack_fetch_strategy/uri_spec.rb b/spec/unit/plugin_manager/pack_fetch_strategy/uri_spec.rb
index 09effdb90..210b32d5b 100644
--- a/spec/unit/plugin_manager/pack_fetch_strategy/uri_spec.rb
+++ b/spec/unit/plugin_manager/pack_fetch_strategy/uri_spec.rb
@@ -32,10 +32,12 @@ describe LogStash::PluginManager::PackFetchStrategy::Uri do
     let(:temporary_file) do
       f = Stud::Temporary.file
       f.write("hola")
+      f.close
       f.path
     end
 
-    let(:plugin_path) { "file://#{temporary_file}" }
+    # Windows safe way to produce a file: URI.
+    let(:plugin_path) { URI.join("file:///" + File.absolute_path(temporary_file)).to_s }
 
     it "returns a `LocalInstaller`" do
       expect(subject.get_installer_for(plugin_path)).to be_kind_of(LogStash::PluginManager::PackInstaller::Local)
diff --git a/spec/unit/plugin_manager/prepare_offline_pack_spec.rb b/spec/unit/plugin_manager/prepare_offline_pack_spec.rb
index aa9376c91..9c55457d0 100644
--- a/spec/unit/plugin_manager/prepare_offline_pack_spec.rb
+++ b/spec/unit/plugin_manager/prepare_offline_pack_spec.rb
@@ -79,6 +79,10 @@ describe LogStash::PluginManager::PrepareOfflinePack do
         expect(LogStash::PluginManager::OfflinePluginPackager).not_to receive(:package).with(anything)
       end
 
+      after do
+        FileUtils.rm_rf(tmp_zip_file)
+      end
+
       it "fails to do any action" do
         expect { subject.run(cmd_args) }.to raise_error Clamp::ExecutionError, /you must specify a filename/
       end
@@ -101,13 +105,18 @@ describe LogStash::PluginManager::PrepareOfflinePack do
         FileUtils.touch(tmp_zip_file)
       end
 
+      after do
+        FileUtils.rm_f(tmp_zip_file)
+      end
+
       context "without `--overwrite`" do
         before do
           expect(LogStash::PluginManager::OfflinePluginPackager).not_to receive(:package).with(anything)
         end
 
         it "should fails" do
-          expect { subject.run(cmd_args) }.to raise_error Clamp::ExecutionError, /output file destination #{tmp_zip_file} already exist/
+          # ignore the first path part of tmp_zip_file because on Windows the long path is shrinked in the exception message 
+          expect { subject.run(cmd_args) }.to raise_error Clamp::ExecutionError, /output file destination .+#{::File.basename(tmp_zip_file)} already exist/
         end
       end
 
diff --git a/spec/unit/plugin_manager/utils/downloader_spec.rb b/spec/unit/plugin_manager/utils/downloader_spec.rb
index e08e731af..2f7a105eb 100644
--- a/spec/unit/plugin_manager/utils/downloader_spec.rb
+++ b/spec/unit/plugin_manager/utils/downloader_spec.rb
@@ -56,7 +56,7 @@ describe LogStash::PluginManager::Utils::Downloader do
       let(:temporary_path) { Stud::Temporary.pathname }
 
       before do
-        expect_any_instance_of(::File).to receive(:close).at_least(:twice).and_raise("Didn't work")
+        expect(Net::HTTP::Get).to receive(:new).once.and_raise("Didn't work")
         expect(Stud::Temporary).to receive(:pathname).and_return(temporary_path)
       end