Skip to content

Commit

Permalink
fixes for Windows platform tests/specs
Browse files Browse the repository at this point in the history
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
  • Loading branch information
colinsurprenant committed Sep 1, 2017
1 parent 8fd291b commit b24b480
Show file tree
Hide file tree
Showing 7 changed files with 36 additions and 16 deletions.
2 changes: 1 addition & 1 deletion lib/pluginmanager/utils/downloader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def fetch(redirect_count = 0)
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
Expand Down
6 changes: 5 additions & 1 deletion logstash-core/spec/logstash/api/modules/node_stats_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@
"cpu"=>{
"total_in_millis"=>Numeric,
"percent"=>Numeric,
"load_average" => { "1m" => Numeric }
# load_average is not supported on Windows, set it below
}
},
"pipeline" => {
Expand All @@ -88,5 +88,9 @@
}
}

unless LogStash::Environment.windows?
root_structure["process"]["cpu"]["load_average"] = { "1m" => Numeric }
end

test_api_and_resources(root_structure)
end
4 changes: 3 additions & 1 deletion logstash-core/spec/logstash/pipeline_dlq_commit_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ def close() end
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
Expand All @@ -85,6 +85,7 @@ def close() end
entry = dlq_reader.pollEntry(40)
expect(entry).to_not be_nil
expect(entry.reason).to eq("my reason")
subject.shutdown
end
end

Expand All @@ -101,6 +102,7 @@ def close() end
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

Expand Down
23 changes: 13 additions & 10 deletions logstash-core/spec/logstash/settings/writable_directory_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,17 @@
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) }
Expand Down Expand Up @@ -44,8 +44,9 @@
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
Expand All @@ -66,7 +67,8 @@
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
Expand All @@ -84,12 +86,13 @@
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
Expand All @@ -114,8 +117,8 @@

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"
Expand Down
4 changes: 3 additions & 1 deletion spec/unit/plugin_manager/pack_fetch_strategy/uri_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,12 @@
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)
Expand Down
11 changes: 10 additions & 1 deletion spec/unit/plugin_manager/prepare_offline_pack_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@
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
Expand All @@ -101,13 +105,18 @@
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

Expand Down
2 changes: 1 addition & 1 deletion spec/unit/plugin_manager/utils/downloader_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
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

Expand Down

0 comments on commit b24b480

Please sign in to comment.