From 2079bf69f5b144570738397977a4c22b185a7f66 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Wed, 5 Nov 2014 18:26:38 +0100 Subject: [PATCH 1/6] remove ruby-head and jruby-head from travis --- .travis.yml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/.travis.yml b/.travis.yml index 05a336f3..b7d1c40c 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,10 +5,8 @@ rvm: - 1.9.3 - 2.0.0 - 2.1.0 - - ruby-head - jruby-18mode - jruby-19mode - - jruby-head - ree script: - bundle exec image_optim --info @@ -28,6 +26,3 @@ env: - PATH=~/bin:$PATH matrix: fast_finish: true - allow_failures: - - rvm: ruby-head - - rvm: jruby-head From f8c82e0a6d9f626b1b1c14be721c90096da7a8da Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 6 Nov 2014 01:56:32 +0100 Subject: [PATCH 2/6] move all worker initialization to Worker --- lib/image_optim.rb | 21 +---------------- lib/image_optim/worker.rb | 49 ++++++++++++++++++++++++--------------- 2 files changed, 31 insertions(+), 39 deletions(-) diff --git a/lib/image_optim.rb b/lib/image_optim.rb index 6acea6c4..e27eb3c4 100644 --- a/lib/image_optim.rb +++ b/lib/image_optim.rb @@ -64,7 +64,7 @@ def initialize(options = {}) @bin_resolver = BinResolver.new(self) - @workers_by_format = create_workers_by_format do |klass| + @workers_by_format = Worker.create_all_by_format(self) do |klass| config.for_worker(klass) end @@ -192,25 +192,6 @@ def log_workers_by_format end end - # Create hash with format mapped to list of workers sorted by run order - def create_workers_by_format(&options_proc) - by_format = {} - workers = Worker.create_all(self, &options_proc) - if skip_missing_workers - workers = Worker.reject_missing(workers) - else - Worker.resolve_all!(workers) - end - sorted = workers.sort_by.with_index{ |worker, i| [worker.run_order, i] } - sorted.each do |worker| - worker.image_formats.each do |format| - by_format[format] ||= [] - by_format[format] << worker - end - end - by_format - end - # Run method for each item in list # if block given yields item and result for item and returns array of yield # results diff --git a/lib/image_optim/worker.rb b/lib/image_optim/worker.rb index f069baac..f37626f2 100644 --- a/lib/image_optim/worker.rb +++ b/lib/image_optim/worker.rb @@ -1,6 +1,6 @@ # encoding: UTF-8 -require 'image_optim/bin_resolver/error' +require 'image_optim/bin_resolver' require 'image_optim/configuration_error' require 'image_optim/option_definition' require 'image_optim/option_helpers' @@ -42,34 +42,45 @@ def option(name, default, type, description = nil, &proc) OptionDefinition.new(name, default, type, description, &proc) end - # Initialize all workers using options from calling options_proc with - # klass + # Create hash with format mapped to list of workers sorted by run order + def create_all_by_format(image_optim, &options_proc) + by_format = {} + create_all(image_optim, &options_proc).each do |worker| + worker.image_formats.each do |format| + by_format[format] ||= [] + by_format[format] << worker + end + end + by_format + end + + # Create list of workers sorted by run order + # Workers are initialized with options provided through options_proc + # Resolve all bins of all workers, if there are errors and + # skip_missing_workers of image_optim is true - show warnings, otherwise + # fail with one joint exception def create_all(image_optim, &options_proc) - Worker.klasses.map do |klass| + workers = klasses.map do |klass| next unless (options = options_proc[klass]) klass.new(image_optim, options) end.compact - end - - # Resolve all bins of all workers failing with one joint exception - def resolve_all!(workers) - errors = BinResolver.collect_errors(workers) do |worker| - worker.resolve_used_bins! - end - return if errors.empty? - fail BinResolver::Error, ['Bin resolving errors:', *errors].join("\n") - end - # Resolve all bins of all workers showing warning for missing ones and - # returning others - def reject_missing(workers) resolved = [] errors = BinResolver.collect_errors(workers) do |worker| worker.resolve_used_bins! resolved << worker end - errors.each{ |error| warn error } - resolved + + unless errors.empty? + if image_optim.skip_missing_workers + errors.each{ |error| warn error } + else + message = ['Bin resolving errors:', *errors].join("\n") + fail BinResolver::Error, message + end + end + + resolved.sort_by.with_index{ |worker, i| [worker.run_order, i] } end end From 70e4b1c4be0319529b60de385f2f8c5c64edb7fa Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 6 Nov 2014 01:57:05 +0100 Subject: [PATCH 3/6] add specs for worker initialization, move spec for run_order --- spec/image_optim/worker_spec.rb | 118 ++++++++++++++++++++++++++++++++ spec/image_optim_spec.rb | 30 -------- 2 files changed, 118 insertions(+), 30 deletions(-) diff --git a/spec/image_optim/worker_spec.rb b/spec/image_optim/worker_spec.rb index dc255bc9..582fb3de 100644 --- a/spec/image_optim/worker_spec.rb +++ b/spec/image_optim/worker_spec.rb @@ -1,9 +1,11 @@ require 'spec_helper' require 'image_optim/worker' +require 'image_optim/bin_resolver' describe ImageOptim::Worker do before do stub_const('Worker', ImageOptim::Worker) + stub_const('BinResolver', ImageOptim::BinResolver) end describe :optimize do @@ -17,4 +19,120 @@ end end + describe :create_all_by_format do + it 'passes arguments to create_all' do + image_optim = double + options_proc = proc{ true } + + expect(Worker).to receive(:create_all) do |arg, &block| + expect(arg).to eq(image_optim) + expect(block).to eq(options_proc) + [] + end + + Worker.create_all_by_format(image_optim, &options_proc) + end + + it 'create hash by format' do + workers = [ + double(:image_formats => [:a]), + double(:image_formats => [:a, :b]), + double(:image_formats => [:b, :c]), + ] + + expect(Worker).to receive(:create_all).and_return(workers) + + expect(Worker.create_all_by_format(double)).to eq({ + :a => [workers[0], workers[1]], + :b => [workers[1], workers[2]], + :c => [workers[2]], + }) + end + end + + describe :create_all do + let(:image_optim){ double } + + it 'creates all workers for which options_proc returns true' do + workers = Array.new(3) do + double(:resolve_used_bins! => nil, :run_order => 0) + end + klasses = workers.map{ |worker| double(:new => worker) } + options_proc = proc{ |klass| klass != klasses[1] } + + allow(Worker).to receive(:klasses).and_return(klasses) + + expect(Worker.create_all(image_optim, &options_proc)). + to eq([workers[0], workers[2]]) + end + + describe 'with missing workers' do + let(:workers) do + Array.new(3) do |i| + worker = double(:resolve_used_bins! => nil, :run_order => 0) + unless i == 1 + allow(worker).to receive(:resolve_used_bins!). + and_raise(BinResolver::BinNotFound, "not found #{i}") + end + worker + end + end + let(:klasses){ workers.map{ |worker| double(:new => worker) } } + + before do + allow(Worker).to receive(:klasses).and_return(klasses) + end + + describe 'if skip_missing_workers is true' do + define :bin_not_found do |message| + match do |error| + error.is_a?(BinResolver::BinNotFound) && error.message == message + end + end + + it 'shows warnings and returns resolved workers ' do + allow(image_optim).to receive(:skip_missing_workers).and_return(true) + + expect(Worker).to receive(:warn).once.with(bin_not_found('not found 0')) + expect(Worker).to receive(:warn).once.with(bin_not_found('not found 2')) + + expect(Worker.create_all(image_optim){ true }). + to eq([workers[1]]) + end + end + + describe 'if skip_missing_workers is false' do + it 'fails with a joint exception' do + allow(image_optim).to receive(:skip_missing_workers).and_return(false) + + expect do + Worker.create_all(image_optim){ true } + end.to raise_error(BinResolver::Error, /not found 0\nnot found 2/) + end + end + end + + it 'orders workers by run_order' do + image_optim = double + run_orders = [10, -10, 0, 0, 0, 10, -10] + workers = run_orders.map do |run_order| + double(:resolve_used_bins! => nil, :run_order => run_order) + end + klasses = workers.map{ |worker| double(:new => worker) } + + [ + klasses, + klasses.reverse, + klasses.shuffle, + ].each do |klasses| + allow(Worker).to receive(:klasses).and_return(klasses) + + expected_order = klasses.map(&:new).sort_by.with_index do |worker, i| + [worker.run_order, i] + end + + expect(Worker.create_all(image_optim){ true }).to eq(expected_order) + end + end + end end diff --git a/spec/image_optim_spec.rb b/spec/image_optim_spec.rb index bbc4d167..82c1c99a 100644 --- a/spec/image_optim_spec.rb +++ b/spec/image_optim_spec.rb @@ -28,36 +28,6 @@ def temp_copy(image) allow(ImageOptim::Config).to receive(:local).and_return({}) end - describe 'workers' do - they 'are ordered by run_order' do - image_optim = ImageOptim.new - original_klasses = ImageOptim::Worker.klasses - formats = original_klasses.map do |klass| - klass.new(image_optim, {}).image_formats - end.flatten.uniq - - [ - original_klasses, - original_klasses.to_a.reverse, - original_klasses.to_a.shuffle, - ].each do |klasses| - expect(ImageOptim::Worker).to receive(:klasses).and_return(klasses) - - image_optim = ImageOptim.new - - formats.each do |format| - path = ImageOptim::ImagePath.new("test.#{format}") - expect(path).to receive(:format).and_return(format) - - workers = image_optim.workers_for_image(path) - expect(workers).to eq(workers.sort_by.with_index do |worker, i| - [worker.run_order, i] - end) - end - end - end - end - disable_all_workers = Hash[ImageOptim::Worker.klasses.map do |klass| [klass.bin_sym, false] end] From 9fee3f9ae28065d44c0a1422a8baf57e07d8d108 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 6 Nov 2014 05:10:20 +0100 Subject: [PATCH 4/6] fix code style in worker_spec --- spec/image_optim/worker_spec.rb | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/spec/image_optim/worker_spec.rb b/spec/image_optim/worker_spec.rb index 582fb3de..e3db0623 100644 --- a/spec/image_optim/worker_spec.rb +++ b/spec/image_optim/worker_spec.rb @@ -42,11 +42,13 @@ expect(Worker).to receive(:create_all).and_return(workers) - expect(Worker.create_all_by_format(double)).to eq({ + worker_by_format = { :a => [workers[0], workers[1]], :b => [workers[1], workers[2]], :c => [workers[2]], - }) + } + + expect(Worker.create_all_by_format(double)).to eq(worker_by_format) end end @@ -93,8 +95,10 @@ it 'shows warnings and returns resolved workers ' do allow(image_optim).to receive(:skip_missing_workers).and_return(true) - expect(Worker).to receive(:warn).once.with(bin_not_found('not found 0')) - expect(Worker).to receive(:warn).once.with(bin_not_found('not found 2')) + expect(Worker).to receive(:warn). + once.with(bin_not_found('not found 0')) + expect(Worker).to receive(:warn). + once.with(bin_not_found('not found 2')) expect(Worker.create_all(image_optim){ true }). to eq([workers[1]]) @@ -118,12 +122,12 @@ workers = run_orders.map do |run_order| double(:resolve_used_bins! => nil, :run_order => run_order) end - klasses = workers.map{ |worker| double(:new => worker) } + klasses_list = workers.map{ |worker| double(:new => worker) } [ - klasses, - klasses.reverse, - klasses.shuffle, + klasses_list, + klasses_list.reverse, + klasses_list.shuffle, ].each do |klasses| allow(Worker).to receive(:klasses).and_return(klasses) From b313d3cc14eb487d18ba01069b9228cd53abb342 Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 6 Nov 2014 15:48:42 +0100 Subject: [PATCH 5/6] extract worker_double and use instance_double in worker_spec --- spec/image_optim/worker_spec.rb | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/spec/image_optim/worker_spec.rb b/spec/image_optim/worker_spec.rb index e3db0623..7ea692ff 100644 --- a/spec/image_optim/worker_spec.rb +++ b/spec/image_optim/worker_spec.rb @@ -53,12 +53,15 @@ end describe :create_all do + def worker_double(override = {}) + stubs = {:resolve_used_bins! => nil, :run_order => 0}.merge(override) + instance_double(Worker, stubs) + end + let(:image_optim){ double } it 'creates all workers for which options_proc returns true' do - workers = Array.new(3) do - double(:resolve_used_bins! => nil, :run_order => 0) - end + workers = Array.new(3){ worker_double } klasses = workers.map{ |worker| double(:new => worker) } options_proc = proc{ |klass| klass != klasses[1] } @@ -71,7 +74,7 @@ describe 'with missing workers' do let(:workers) do Array.new(3) do |i| - worker = double(:resolve_used_bins! => nil, :run_order => 0) + worker = worker_double unless i == 1 allow(worker).to receive(:resolve_used_bins!). and_raise(BinResolver::BinNotFound, "not found #{i}") @@ -120,7 +123,7 @@ image_optim = double run_orders = [10, -10, 0, 0, 0, 10, -10] workers = run_orders.map do |run_order| - double(:resolve_used_bins! => nil, :run_order => run_order) + worker_double(:run_order => run_order) end klasses_list = workers.map{ |worker| double(:new => worker) } From 13292af2c70357d70a41210f6e0278c86666414e Mon Sep 17 00:00:00 2001 From: Ivan Kuchin Date: Thu, 6 Nov 2014 15:51:47 +0100 Subject: [PATCH 6/6] use init instead of new for initializing workers directly so worker can initialize multiple instances if needed --- CHANGELOG.markdown | 2 ++ lib/image_optim/worker.rb | 7 +++++-- spec/image_optim/worker_spec.rb | 22 ++++++++++++++++++---- 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.markdown b/CHANGELOG.markdown index a4ae6852..6dff9bba 100644 --- a/CHANGELOG.markdown +++ b/CHANGELOG.markdown @@ -2,6 +2,8 @@ ## unreleased +* Worker can change its initialization by overriding `init` and can initialize multiple instances [#70](https://github.com/toy/image_optim/issues/70) [@toy](https://github.com/toy) + ## v0.18.0 (2014-11-01) * Add interface to `image_optim_pack` [@toy](https://github.com/toy) diff --git a/lib/image_optim/worker.rb b/lib/image_optim/worker.rb index f37626f2..ec0c60da 100644 --- a/lib/image_optim/worker.rb +++ b/lib/image_optim/worker.rb @@ -14,6 +14,9 @@ class Worker @klasses = [] class << self + # Default init for worker is new + alias_method :init, :new + # List of available workers def klasses @klasses.to_enum @@ -62,8 +65,8 @@ def create_all_by_format(image_optim, &options_proc) def create_all(image_optim, &options_proc) workers = klasses.map do |klass| next unless (options = options_proc[klass]) - klass.new(image_optim, options) - end.compact + klass.init(image_optim, options) + end.compact.flatten resolved = [] errors = BinResolver.collect_errors(workers) do |worker| diff --git a/spec/image_optim/worker_spec.rb b/spec/image_optim/worker_spec.rb index 7ea692ff..e445fa09 100644 --- a/spec/image_optim/worker_spec.rb +++ b/spec/image_optim/worker_spec.rb @@ -62,7 +62,7 @@ def worker_double(override = {}) it 'creates all workers for which options_proc returns true' do workers = Array.new(3){ worker_double } - klasses = workers.map{ |worker| double(:new => worker) } + klasses = workers.map{ |worker| double(:init => worker) } options_proc = proc{ |klass| klass != klasses[1] } allow(Worker).to receive(:klasses).and_return(klasses) @@ -71,6 +71,20 @@ def worker_double(override = {}) to eq([workers[0], workers[2]]) end + it 'handles workers initializing multiple instances' do + workers = [ + worker_double, + [worker_double, worker_double, worker_double], + worker_double + ] + klasses = workers.map{ |worker| double(:init => worker) } + + allow(Worker).to receive(:klasses).and_return(klasses) + + expect(Worker.create_all(image_optim){ true }). + to eq(workers.flatten) + end + describe 'with missing workers' do let(:workers) do Array.new(3) do |i| @@ -82,7 +96,7 @@ def worker_double(override = {}) worker end end - let(:klasses){ workers.map{ |worker| double(:new => worker) } } + let(:klasses){ workers.map{ |worker| double(:init => worker) } } before do allow(Worker).to receive(:klasses).and_return(klasses) @@ -125,7 +139,7 @@ def worker_double(override = {}) workers = run_orders.map do |run_order| worker_double(:run_order => run_order) end - klasses_list = workers.map{ |worker| double(:new => worker) } + klasses_list = workers.map{ |worker| double(:init => worker) } [ klasses_list, @@ -134,7 +148,7 @@ def worker_double(override = {}) ].each do |klasses| allow(Worker).to receive(:klasses).and_return(klasses) - expected_order = klasses.map(&:new).sort_by.with_index do |worker, i| + expected_order = klasses.map(&:init).sort_by.with_index do |worker, i| [worker.run_order, i] end