From 746c3d163bd44752237e3ddbf5b50e2c8c7b39d2 Mon Sep 17 00:00:00 2001 From: Colin Surprenant 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 +- .../logstash/api/modules/node_stats_spec.rb | 6 ++++- .../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 +- 7 files changed, 36 insertions(+), 16 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/spec/logstash/api/modules/node_stats_spec.rb b/logstash-core/spec/logstash/api/modules/node_stats_spec.rb index 2250e885e..922190383 100644 --- a/logstash-core/spec/logstash/api/modules/node_stats_spec.rb +++ b/logstash-core/spec/logstash/api/modules/node_stats_spec.rb @@ -70,7 +70,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 } }, "pipeline" => { @@ -88,5 +88,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/pipeline_dlq_commit_spec.rb b/logstash-core/spec/logstash/pipeline_dlq_commit_spec.rb index 530f27f74..093517090 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